Hold Receiver directly in bound node for implicit indexer access by AlekseyTs · Pull Request #58009 · dotnet/roslyn

AlekseyTs · 2021-11-29T15:08:53Z

Related to #57855.

Relates to test plan #51289

Related to dotnet#57855.

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

333fred

LGTM (commit 1), just a minor spelling touchup.

AlekseyTs · 2021-11-29T17:41:09Z

@jcouv, @dotnet/roslyn-compiler Please review.

jcouv · 2021-11-29T19:55:45Z

src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs

+
+    internal partial class BoundDeconstructValuePlaceholder
+    {
+        public sealed override bool IsEquivalentToThisReference => false; // Preserving old behavior


nit: I found the comment confusing. If the behavior is incorrect, should we file an issue instead?
Same for BoundAwaitableValuePlaceholder

If the behavior is incorrect, should we file an issue instead?

That is not something that I intend to pursue, given that this only affects warnings. If you think we have to follow up anyway, I can open an issue. Let me know.

jcouv · 2021-11-29T19:58:09Z

            if (!(receiver is BoundThisReference))

~~Should we check IsEquivalentToThisReference instead?~~
Never mind, I can't think of a way of using a placeholder receiver to access a field.

In reply to: 981967541

Refers to: src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs:858 in 455e6af. [](commit_id = 455e6af, deletion_comment = False)

jcouv · 2021-11-29T20:04:35Z

                ReportDiagnosticsIfObsolete(diagnostics, setMethod, node, receiver?.Kind == BoundKind.BaseReference);

Do we need some abstraction for placeholder receivers here? (there's a couple more places that check for BoundKind.BaseReference below to consider as well)

In reply to: 981972509

Refers to: src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs:1138 in 455e6af. [](commit_id = 455e6af, deletion_comment = False)

jcouv · 2021-11-29T20:08:13Z

            case BoundKind.ThisReference:

I'm assuming we never need to get the ref-escape scope for a placeholder. If that's correct, could we assert that?

In reply to: 981975586

Refers to: src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs:2047 in 455e6af. [](commit_id = 455e6af, deletion_comment = False)

jcouv · 2021-11-29T20:10:54Z

            case BoundKind.ThisReference:

Should placeholders that are equivalent to this fall into this case?
Same question applies to CheckValEscape

Refers to: src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs:2588 in 455e6af. [](commit_id = 455e6af, deletion_comment = False)

jcouv · 2021-11-29T20:15:59Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            if (expression is BoundValuePlaceholderBase placeholder)
            {
-                if (_awaitablePlaceholdersOpt != null && _awaitablePlaceholdersOpt.TryGetValue((BoundAwaitableValuePlaceholder)expression, out var value))
+                if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(placeholder, out var value))


nit: Thanks for renaming the map to something more general. Along those lines, we may want to add a helper methods for placeholder replacement (here, VisitPlaceholderWithReplacement for replacements, and in VisitAwaitExpression and other places for registering replacements) #WontFix

jcouv · 2021-11-29T20:17:21Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            VisitRvalue(node.Argument);
+
+            EnsurePlaceholdersToResultMap();
+            _resultForPlaceholdersOpt.Add(node.ReceiverPlaceholder, (node.Receiver, receiverResult));


Consider extracting to NullableWalker.AddPlaceholderReplacement and RemovePlaceholderReplacement #WontFix

Consider extracting to NullableWalker.AddPlaceholderReplacement and RemovePlaceholderReplacement

I prefer to keep the churn to a minimum.

jcouv · 2021-11-29T20:23:39Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            _resultForPlaceholdersOpt.Add(node.ReceiverPlaceholder, (node.Receiver, receiverResult));
            VisitRvalue(node.IndexerOrSliceAccess);
-            RemovePlaceholder(node.ArgumentPlaceholders[0]);
+            _resultForPlaceholdersOpt.Remove(node.ReceiverPlaceholder);


Not blocking this PR, since it's not the main purpose, but consider adding the visit for LengthAccess too, now that we can. #Resolved

Not blocking this PR, since it's not the main purpose, but consider adding the visit for LengthAccess too, now that we can.

I am not sure if that would be the right thing to do. The length is not always evaluated and I prefer to not deal with the logic to figure that out here. The receiver is checked already, there is pretty much nothing else we can learn.

jcouv · 2021-11-29T20:27:02Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

    <Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow" />

+    <!-- The target of the index operation -->
+    <Field Name="Receiver" Type="BoundExpression" Null="disallow" />


I'm very glad we're able to move back to this design with the confidence that the ValueCheck logic only needs to be abstracted for placeholders that might be "this" (not as bad as we feared).
Thanks a lot! #Resolved

jcouv

Done with review pass (iteration 2)

AlekseyTs · 2021-11-29T22:16:47Z

                ReportDiagnosticsIfObsolete(diagnostics, setMethod, node, receiver?.Kind == BoundKind.BaseReference);

Do we need some abstraction for placeholder receivers here? (there's a couple more places that check for BoundKind.BaseReference below to consider as well)

A base expression cannot be used as a stand-alone expression. So, I do not think there is a scenario that can benefit from this abstraction today.

In reply to: 981972509

Refers to: src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs:1138 in 455e6af. [](commit_id = 455e6af, deletion_comment = False)

AlekseyTs · 2021-11-29T22:23:12Z

            case BoundKind.ThisReference:

I'm assuming we never need to get the ref-escape scope for a placeholder. If that's correct, could we assert that?

Figuring out abstractions necessary for ref-safety checks is out of scope of this PR, including placing asserts. The PR doesn't close the issue.

In reply to: 981975586

Refers to: src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs:2047 in 455e6af. [](commit_id = 455e6af, deletion_comment = False)

AlekseyTs · 2021-11-29T22:31:10Z

            case BoundKind.ThisReference:

Should placeholders that are equivalent to this fall into this case?

This is out of scope as well. The only placeholder that could be affected today is ImplicitIndexerReceiverPlaceholder, which is explicitly handled below

In reply to: 981977750

Refers to: src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs:2588 in 455e6af. [](commit_id = 455e6af, deletion_comment = False)

jcouv

LGTM Thanks (iteration 2)

…rovements * upstream/main: (310 commits) Read SourceLink info and call service to retrieve source from there (dotnet#57978) Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598) (dotnet#58050) Snap 17.1 P2 (dotnet#58041) Make it possible to analyze the dataflow of `ConstructorInitializerSyntax` and `PrimaryConstructorBaseTypeSyntax` (dotnet#57576) Shorten paths in VS installation (dotnet#57726) Add comments Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598) Fix await completion for expression body lambda Add tests Fix comment Honor option, and also improve formatting with comment Skip TestLargeStringConcatenation (dotnet#58035) Log runtime framework of remote host Mark EqualityContract property accessor as not auto-implemented (dotnet#57917) Fix typo in XML doc for GeneratorExtensions (dotnet#58020) Hold Receiver directly in bound node for implicit indexer access (dotnet#58009) Pass AnalysisKind instead of int Enable nullable reference types for TableDataSource Simplify 'interpolation' data, and move to an easier to consume System.Range approach for it (dotnet#57966) Add missing test for CallerArgumentExpression (dotnet#57805) ...

Hold Receiver directly in bound node for implicit indexer access

ed2d70a

Related to dotnet#57855.

AlekseyTs added the Area-Compilers label Nov 29, 2021

AlekseyTs requested a review from jcouv November 29, 2021 15:08

AlekseyTs requested a review from a team as a code owner November 29, 2021 15:08

333fred reviewed Nov 29, 2021

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Show resolved Hide resolved

333fred reviewed Nov 29, 2021

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated Show resolved Hide resolved

333fred approved these changes Nov 29, 2021

View reviewed changes

This was referenced Nov 29, 2021

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

[Flaky Test] VisualBasic.BasicAddMissingReference.InvokeSomeFixesInVisualBasicThenVerifyReferences #57980

Closed

Rename

455e6af

jcouv self-assigned this Nov 29, 2021

jcouv reviewed Nov 29, 2021

View reviewed changes

jcouv approved these changes Nov 30, 2021

View reviewed changes

AlekseyTs merged commit 03f6adf into dotnet:main Nov 30, 2021

ghost added this to the Next milestone Nov 30, 2021

allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021

Conversation

AlekseyTs commented Nov 29, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 29, 2021

Uh oh!

jcouv Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 29, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Nov 29, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Nov 29, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Nov 29, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 29, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 29, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 29, 2021 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 29, 2021

Uh oh!

AlekseyTs commented Nov 29, 2021

Uh oh!

AlekseyTs commented Nov 29, 2021

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AlekseyTs commented Nov 29, 2021 •

edited by jcouv

Loading

jcouv commented Nov 29, 2021 •

edited by AlekseyTs

Loading

jcouv commented Nov 29, 2021 •

edited by AlekseyTs

Loading

jcouv commented Nov 29, 2021 •

edited by AlekseyTs

Loading

jcouv commented Nov 29, 2021 •

edited

Loading

jcouv Nov 29, 2021 •

edited by AlekseyTs

Loading

jcouv Nov 29, 2021 •

edited by AlekseyTs

Loading

jcouv Nov 29, 2021 •

edited by AlekseyTs

Loading

jcouv Nov 29, 2021 •

edited by AlekseyTs

Loading