Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve detection of receiver cloning in ref safety analysis #67736

Merged
merged 29 commits into from
Aug 18, 2023
Merged

Improve detection of receiver cloning in ref safety analysis #67736

merged 29 commits into from
Aug 18, 2023

Conversation

jjonescz
Copy link
Member

Fixes #67697.
Fixes #67626.

The regression was likely introduced in #64890.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 11, 2023
@jjonescz jjonescz marked this pull request as ready for review April 11, 2023 15:11
@jjonescz jjonescz requested a review from a team as a code owner April 11, 2023 15:11
@jjonescz jjonescz marked this pull request as draft April 12, 2023 11:38
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 12, 2023 •
edited
Loading 

                if (!Binder.HasHome(receiver,

After learning more about behavior of HasHome API after this call site was introduced, I am leaning towards thinking that using this API here was a mistake. It is supposed to be used only on lowered trees and its behavior is very IL gen specific. I think that instead of trying to make it work for initial binding, we should look for an alternative way to check for what we want. Perhaps we can use CheckValueKind instead? With BindValueKind.RefersToLocation for effectively readonly method and BindValueKind.RefersToLocation | BindValueKind.Assignable otherwise.

In reply to: 1505177626

Refers to: src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs:2091 in 42498e4. [](commit_id = 42498e4, deletion_comment = False)

@jaredpar
Copy link
Member

any particular reason this is in draft?

@jjonescz
Copy link
Member Author

any particular reason this is in draft?

yes, it's broken, need to refactor slightly

@jjonescz jjonescz changed the title Consider ref property and indexer accesses to have home Improve detection of receiver cloning in ref safety analysis Apr 17, 2023
@jjonescz jjonescz marked this pull request as ready for review April 17, 2023 14:53
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 18, 2023 •
edited
Loading

Does this PR address #67435?

In reply to: 1513588074

AlekseyTs
AlekseyTs approved these changes Apr 21, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 23)

@jjonescz
Copy link
Member Author

@dotnet/roslyn-compiler for a second review, thanks

@jjonescz
Copy link
Member Author

jjonescz commented May 3, 2023

@dotnet/roslyn-compiler for a second review, thanks

@build-analysis build-analysis bot mentioned this pull request May 3, 2023
Closed
5 tasks
@jjonescz
Copy link
Member Author

@dotnet/roslyn-compiler for a second review please

@jjonescz
Copy link
Member Author

@dotnet/roslyn-compiler for the second review, thanks

@jaredpar
Copy link
Member

@jcouv PTAL

@jcouv jcouv self-assigned this Jun 15, 2023
@jaredpar jaredpar added this to the 17.8 milestone Jul 18, 2023
@RikkiGibson
Copy link
Contributor

Is this PR still planned for 17.8?

@jjonescz
Copy link
Member Author

jjonescz commented Aug 8, 2023 •
edited
Loading

@RikkiGibson Yes, this bug fix is still planned for 17.8. I have merged main here, so it's up to date.

@jaredpar jaredpar assigned RikkiGibson and unassigned jcouv Aug 9, 2023
RikkiGibson
RikkiGibson approved these changes Aug 17, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs
return BoundCall.Synthesized(
syntax,
expression,
initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method ever called with a value besides Unknown here? Could we eliminate a lot of the churn by removing the parameter and possibly adding an overload to synthesize a call with non-Unknown value if needed?

Not blocking this PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method ever called with a value besides Unknown here?

It is, for example:

roslyn/src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs

Line 714 in 6db323e

initialBindingReceiverIsSubjectToCloning: ReceiverIsSubjectToCloning(collectionExpr, nullableValueGetter),

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs
@@ -25672,6 +25672,225 @@ class B2 : A<string>
Diagnostic(ErrorCode.ERR_UnscopedRefAttributeUnsupportedMemberTarget, "UnscopedRef").WithLocation(17, 6));
}

[Theory, CombinatorialData, WorkItem("https://github.com/dotnet/roslyn/issues/67697")]
public void UnscopedRefAttribute_NestedAccess_MethodOrProperty(bool firstMethod, bool secondMethod)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider naming parameters something like firstIsMethod, secondIsMethod

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
@@ -1982,6 +1988,7 @@ bool isRefEscape
GetInvocationArgumentsForEscape(
symbol,
receiver: null, // receiver handled explicitly below
receiverIsSubjectToCloning: ThreeState.Unknown,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, passing Unknown is fine here only because receiver is null.

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs Outdated
@@ -363,7 +363,7 @@ private bool IsCountableAndIndexable(SyntaxNode node, TypeSymbol inputType, out
hasErrors |= !TryGetSpecialTypeMember(Compilation, SpecialMember.System_Array__Length, node, diagnostics, out PropertySymbol lengthProperty);
if (lengthProperty is not null)
{
lengthAccess = new BoundPropertyAccess(node, receiverPlaceholder, lengthProperty, LookupResultKind.Viable, lengthProperty.Type) { WasCompilerGenerated = true };
lengthAccess = new BoundPropertyAccess(node, receiverPlaceholder, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, lengthProperty, LookupResultKind.Viable, lengthProperty.Type) { WasCompilerGenerated = true };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we should pass False when receiver is a placeholder.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me, will fix.

src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs
@@ -634,6 +634,7 @@ private void VisitArgumentsAndGetArgumentPlaceholders(BoundExpression? receiverO

protected override void VisitArguments(BoundCall node)
{
Debug.Assert(node.InitialBindingReceiverIsSubjectToCloning != ThreeState.Unknown);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's def a part of me that feels like, man, instead of this flag I wish we could just wrap the receiver expression into the BoundCapturedReceiverPlaceholder node at initial binding time. After all, what we are doing in ref safety analysis is simply to introduce this node late into the tree and then analyze that.

I know that change has the tendency for unpleasant ripple effects through other areas of the compiler. Flow analysis does already have mechanisms for "seeing through" placeholders and so on, but they can be unpleasant to use, and I'm sure more than that would be impacted. Also, I think we've solved the problem with the current change and I don't want to belabor that unnecessarily.

Copy link
Member Author

@jjonescz jjonescz Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea, I haven't tried that, but I can imagine it could be an even more invasive change.

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
@@ -9234,7 +9234,7 @@ private void VisitThisOrBaseReference(BoundExpression node)
// when binding initializers, we treat assignments to auto-properties or field-like events as direct assignments to the underlying field.
// in order to track member state based on these initializers, we need to see the assignment in terms of the associated member
case BoundFieldAccess { ExpressionSymbol: FieldSymbol { AssociatedSymbol: PropertySymbol autoProperty } } fieldAccess:
left = new BoundPropertyAccess(fieldAccess.Syntax, fieldAccess.ReceiverOpt, autoProperty, LookupResultKind.Viable, autoProperty.Type, fieldAccess.HasErrors);
left = new BoundPropertyAccess(fieldAccess.Syntax, fieldAccess.ReceiverOpt, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, autoProperty, LookupResultKind.Viable, autoProperty.Type, fieldAccess.HasErrors);
break;
case BoundFieldAccess { ExpressionSymbol: FieldSymbol { AssociatedSymbol: EventSymbol @event } } fieldAccess:
left = new BoundEventAccess(fieldAccess.Syntax, fieldAccess.ReceiverOpt, @event, isUsableAsField: true, LookupResultKind.Viable, @event.Type, fieldAccess.HasErrors);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do events also need this new field? Or no, because we don't allow implicit cloning for the receiver of an event anyway?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not, because events need to be of a delegate type, and implicit cloning happens only for structs.

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
@@ -9234,7 +9234,7 @@ private void VisitThisOrBaseReference(BoundExpression node)
// when binding initializers, we treat assignments to auto-properties or field-like events as direct assignments to the underlying field.
// in order to track member state based on these initializers, we need to see the assignment in terms of the associated member
case BoundFieldAccess { ExpressionSymbol: FieldSymbol { AssociatedSymbol: PropertySymbol autoProperty } } fieldAccess:
left = new BoundPropertyAccess(fieldAccess.Syntax, fieldAccess.ReceiverOpt, autoProperty, LookupResultKind.Viable, autoProperty.Type, fieldAccess.HasErrors);
left = new BoundPropertyAccess(fieldAccess.Syntax, fieldAccess.ReceiverOpt, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, autoProperty, LookupResultKind.Viable, autoProperty.Type, fieldAccess.HasErrors);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you can see, rewriting the bound tree while we are visiting it is kind of a tradition :P

@jjonescz jjonescz merged commit 3e516fc into dotnet:main Aug 18, 2023
27 checks passed
@jjonescz jjonescz deleted the 67697-UnscopedRef-Property branch August 18, 2023 16:35
@ghost ghost modified the milestones: 17.8, Next Aug 18, 2023
@allisonchou allisonchou mentioned this pull request Aug 22, 2023
Closed
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
@jjonescz jjonescz mentioned this pull request Nov 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

Assignees

@RikkiGibson RikkiGibson

Labels
Area-Compilers New Language Feature - Ref Fields untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.8 P2
6 participants
@jjonescz @AlekseyTs @jaredpar @RikkiGibson @dibarbet @jcouv