Track enclosing symbol in data flow analysis, which might be a field or property. by gafter · Pull Request #28252 · dotnet/roslyn

gafter · 2018-07-03T00:23:40Z

Code used to assume it was (and typed it as) a method.
Fixes #19845
Fixes #27969

@dotnet/roslyn-compiler May I please have a couple of reviews?

Details

Customer scenario

Code containing a reference to a property in an initializer undergoes region analysis. The compiler crashes.

Bugs this fixes

Fixes #19845
Fixes #27969

Workarounds, if any

None known.

Risk

Low. This is a localized correction that widens the type of a field so that we can track the enclosing symbol when it is not a method.

Performance impact

None expected.

Is this a regression from a previous update?

Probably.

Root cause analysis

How was the bug found?

Customer reported.

Test documentation updated?

N/A

…or property. Code used to assume it was (and typed it as) a method. Fixes dotnet#19845 Fixes dotnet#27969

cston · 2018-07-03T14:27:23Z

src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs

+
+    static object Create(object name, Func<string, bool> f) => throw null;
+}
+");


Is this test distinct from 01? #Resolved

Yes, it uses P instead of nameof(P).

In reply to: 199827816 [](ancestors = 199827816)

agocke · 2018-07-03T18:10:21Z

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

+        /// Reflects the enclosing member or lambda at the current location (in the bound tree).
        /// </summary>
-        protected MethodSymbol currentMethodOrLambda { get; private set; }
+        protected Symbol currentMember { get; private set; }


Is it worth it to just call this CurrentSymbol? It's not necessarily a member (possibly a lambda or local function) and now it may not even be a function. This name just seems confusing.

Also, I believe our style guidelines require protected properties to be capitalized. #Closed

We are more consistent in this area using lowercase spelling (e.g. topLevelMethod, nextVariableSlot). If we're going to make a change, I'd rather we not do it piecemeal.

In reply to: 199903748 [](ancestors = 199903748)

Sounds good. #Closed

agocke · 2018-07-03T18:11:34Z

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

        protected virtual void ReportUnassignedOutParameter(ParameterSymbol parameter, SyntaxNode node, Location location)
        {
-            if (!_requireOutParamsAssigned && topLevelMethod == currentMethodOrLambda)
+            if (!_requireOutParamsAssigned && topLevelMethod == (object)currentMember)


ReferenceEquals seems clearer here. I thought our (object) pattern was only for comparing to null. #Closed

The cast to object can be used whenever needed to avoid the use of the declared operator ==, but I'm happy to use ReferenceEquals here.

In reply to: 199904132 [](ancestors = 199904132)

agocke · 2018-07-03T18:12:12Z

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

@@ -439,7 +438,7 @@ private static bool IsCaptured(Symbol variable, MethodSymbol containingMethodOrL
                 currentFunction != null;


May want to add an object cast here too. #Resolved

gafter · 2018-07-03T19:48:45Z

@agocke I think the changes I've just pushed address your comments.

agocke

LGTM

jcouv · 2018-07-03T21:57:03Z

src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs

+}
+");
+            var dataFlowAnalysisResults = analysisResults;
+            Assert.Equal("x", GetSymbolNamesJoined(dataFlowAnalysisResults.VariablesDeclared));


Assert.Equal [](start = 12, length = 12)

Not related to this PR: consider adding a helper method in this test file to do the assert plus the GetSymbolNamesJoined, so that GetSymbolNamesJoined doesn't have to be repeated.

It could look like: ValidateSymbolNames("x", dataFlowAnalysisResults.VariablesDeclared)

Filed #28273

In reply to: 199961497 [](ancestors = 199961497)

jcouv · 2018-07-03T22:22:03Z

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

        protected virtual void EnterParameter(ParameterSymbol parameter)
        {
-            if (parameter.RefKind == RefKind.Out && !this.currentMethodOrLambda.IsAsync) // out parameters not allowed in async
+            if (parameter.RefKind == RefKind.Out && !(this.currentSymbol is MethodSymbol currentMethod && currentMethod.IsAsync)) // out parameters not allowed in async


is [](start = 73, length = 2)

recursive pattern? ;-) #Resolved

jcouv

LGTM Thanks (iteration 2)

jcouv · 2018-07-03T22:22:41Z

@jaredpar for ask-mode approval. Thanks

jaredpar · 2018-07-03T22:40:17Z

approved

…atures/compiler * dotnet/features/compiler: (137 commits) Actually fix tabs. Added back breaking changes doc. Addressed PR feedback. Track enclosing symbol in data flow analysis, which might be a field or property. (dotnet#28252) Change text from 'Spell check' to 'Fix typo'. Implement FAR for GetAwaiter methods (dotnet#28230) Fix typo Fix typo Fix import. Address PR feedback and fix failing test. Lower expressions of in arguments to correct temp locals (dotnet#28181) Move to 2.0.7 runtime Produce errors on ref-assignment to non-ref parameters Fix spelling. Preserve left-to-right evaluation order for dynamic compound addition and subtraction. inline. Provide a spellchecking suggestion when someone mispells a constructor. Typo (dotnet#28177) PR Comments Fix regression in bitness of Interactive Window host (dotnet#28006) ...

Track enclosing symbol in data flow analysis, which might be a field …

a3f149c

…or property. Code used to assume it was (and typed it as) a method. Fixes dotnet#19845 Fixes dotnet#27969

gafter added Bug Area-Compilers labels Jul 3, 2018

gafter added this to the 15.8 milestone Jul 3, 2018

gafter self-assigned this Jul 3, 2018

gafter requested review from a team and agocke July 3, 2018 00:23

cston reviewed Jul 3, 2018

View reviewed changes

cston approved these changes Jul 3, 2018

View reviewed changes

agocke reviewed Jul 3, 2018

View reviewed changes

Minor changes per code review.

9648253

agocke approved these changes Jul 3, 2018

View reviewed changes

jcouv reviewed Jul 3, 2018

View reviewed changes

jcouv approved these changes Jul 3, 2018

View reviewed changes

jaredpar added the Approved to merge label Jul 3, 2018

gafter merged commit 2868849 into dotnet:master Jul 3, 2018

		@@ -439,7 +438,7 @@ private static bool IsCaptured(Symbol variable, MethodSymbol containingMethodOrL
		currentFunction != null;

Conversation

gafter commented Jul 3, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

cston Jul 3, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

agocke Jul 3, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

agocke Jul 3, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke Jul 3, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

agocke Jul 3, 2018 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter commented Jul 3, 2018

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

gafter Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 3, 2018 • edited 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!

jcouv commented Jul 3, 2018

Uh oh!

jaredpar commented Jul 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cston Jul 3, 2018 •

edited by gafter

Loading

agocke Jul 3, 2018 •

edited by jcouv

Loading

agocke Jul 3, 2018 •

edited by jcouv

Loading

agocke Jul 3, 2018 •

edited by jcouv

Loading

agocke Jul 3, 2018 •

edited by gafter

Loading

jcouv Jul 3, 2018 •

edited

Loading