Semantic model APIs on declaration expressions and discards by jcouv · Pull Request #16153 · dotnet/roslyn

jcouv · 2016-12-30T08:52:49Z

Customer scenario

Update semantic model APIs for declaration and discard expressions.

Bugs this fixes:

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

No. Previously the semantic model would return no information for declaration expressions and discards. Now it does.

Root cause analysis:

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

Tracked work item.

jcouv · 2017-01-05T20:09:48Z

@dotnet/roslyn-compiler This is ready for review. Thanks #Resolved

gafter

LGTM

jcouv · 2017-01-06T00:47:19Z

@AlekseyTs If you can find some time, could you do the second review too? Thanks #Resolved

AlekseyTs · 2017-01-06T18:57:53Z

+        {
+            get
+            {
+                return new DiscardSymbol(this.Type);


return new DiscardSymbol(this.Type); [](start = 16, length = 36)

Consider adding an assert that this.Type is not null. #Closed

AlekseyTs · 2017-01-06T19:19:01Z

+
+            var type = TupleTypeSymbol.Create(syntax.Location,
+                elementsBuilder.ToImmutableAndFree(), locationsBuilder.ToImmutableAndFree(),
+                elementNames: default(ImmutableArray<string>), compilation: this.Compilation, shouldCheckConstraints: false);


shouldCheckConstraints: false [](start = 94, length = 29)

Consider adding a comment why constraints shouldn't be checked. #Closed

I'll add a comment.
This makes me think we could construct this final tuple only once. I'll file an issue. #Closed

Filed #16311 #Resolved

AlekseyTs · 2017-01-06T19:20:50Z

+        private BoundTupleLiteral DeconstructVariablesAsTuple(SyntaxNode syntax, ArrayBuilder<DeconstructionVariable> variables)
        {
+            int count = variables.Count;
+            var argBuilder = ArrayBuilder<BoundExpression>.GetInstance(count);


argBuilder [](start = 16, length = 10)

elementValues or valuesBuilder? #Closed

AlekseyTs · 2017-01-06T19:21:07Z

        {
+            int count = variables.Count;
+            var argBuilder = ArrayBuilder<BoundExpression>.GetInstance(count);
+            var elementsBuilder = ArrayBuilder<TypeSymbol>.GetInstance(count);


elementsBuilder [](start = 16, length = 15)

typesBuilder? #Closed

AlekseyTs · 2017-01-06T19:26:11Z

@@ -690,7 +699,7 @@ internal BoundDeconstructionAssignmentOperator BindDeconstruction(
        {
            DeconstructionVariable locals = BindDeconstructionVariables(left, diagnostics, ref declaration, ref expression);


BindDeconstructionVariables [](start = 44, length = 27)

Is there still an advantage of using NestedVariables? Could this function produce tuple literal right from the beginning instead of having to translate this structure later on? #Closed

I think we need NestedVariables because the variables need to be updated (by the inference process). NestedVariables uses a builder, but tuple expression is immutable. #Resolved

AlekseyTs · 2017-01-06T20:43:24Z

                        (node is AttributeSyntax) ||
-                        (node is CrefSyntax);
+                        (node is CrefSyntax) ||
+                        (node is DeclarationExpressionSyntax);


(node is DeclarationExpressionSyntax) [](start = 24, length = 37)

Do we really need this extra condition? It feels like the very first condition should be good enough. #Closed

AlekseyTs · 2017-01-06T20:48:22Z

+                }
+
+                var symbol = GetDeclaredSymbol((SingleVariableDesignationSyntax)declaration.Designation, cancellationToken);
+                return new SymbolInfo(symbol);


return new SymbolInfo(symbol) [](start = 16, length = 29)

Do we need to handle null symbol specially here? #Closed

Thanks. I added it, but I'm having trouble to hit it with a test though.

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

AlekseyTs · 2017-01-06T20:53:20Z

                        return GetBindableSyntaxNode(parent);

-                    case SyntaxKind.DeclarationExpression:
-                    case SyntaxKind.TupleExpression:


case SyntaxKind.TupleExpression: [](start = 20, length = 32)

Are we certain that we can always bind these (both) expressions without knowing context? #Closed

Confirmed we need to remove this code. Thanks

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

AlekseyTs · 2017-01-06T21:57:50Z


            var discard1 = GetDiscardDesignations(tree).ElementAt(0);
            Assert.Null(model.GetDeclaredSymbol(discard1));
+            Assert.Null(model.GetTypeInfo(discard1).Type);


model.GetTypeInfo(discard1) [](start = 24, length = 27)

Test GetSymbolInfo too? #Closed

AlekseyTs · 2017-01-06T21:58:10Z

            var declaration1 = (DeclarationExpressionSyntax)discard1.Parent;
            Assert.Equal("int _", declaration1.ToString());
-            //Assert.Equal("", model.GetTypeInfo(declaration1).Type.ToTestDisplayString()); // https://github.com/dotnet/roslyn/issues/15450
+            Assert.Equal("System.Int32", model.GetTypeInfo(declaration1).Type.ToTestDisplayString());


model.GetTypeInfo(declaration1) [](start = 41, length = 31)

Test GetSymbolInfo too? #Closed

AlekseyTs · 2017-01-06T21:58:59Z

            var declaration2 = (DeclarationExpressionSyntax)discard2.Parent;
            Assert.Equal("var _", declaration2.ToString());
-            //Assert.Equal("", model.GetTypeInfo(declaration2).Type.ToTestDisplayString()); // https://github.com/dotnet/roslyn/issues/15450
+            Assert.Equal("System.Int32", model.GetTypeInfo(declaration2).Type.ToTestDisplayString());


model.GetTypeInfo(declaration2) [](start = 41, length = 31)

Test GetSymbolInfo too? #Closed

AlekseyTs · 2017-01-06T21:59:57Z

            var declaration1 = (DeclarationExpressionSyntax)discard1.Parent;
            Assert.Equal("int _", declaration1.ToString());
-            //Assert.Equal("System.Int32", model.GetTypeInfo(declaration1).Type.ToTestDisplayString()); // https://github.com/dotnet/roslyn/issues/15450
+            Assert.Equal("System.Int32", model.GetTypeInfo(declaration1).Type.ToTestDisplayString());


model.GetTypeInfo(declaration1) [](start = 41, length = 31)

Test GetSymbolInfo too? #Closed

AlekseyTs · 2017-01-06T22:00:24Z

            var declaration2 = (DeclarationExpressionSyntax)discard2.Parent;
            Assert.Equal("var _", declaration2.ToString());
-            //Assert.Equal("System.Int32", model.GetTypeInfo(declaration2).Type.ToTestDisplayString()); // https://github.com/dotnet/roslyn/issues/15450
+            Assert.Equal("System.Int32", model.GetTypeInfo(declaration2).Type.ToTestDisplayString());


model.GetTypeInfo(declaration2) [](start = 41, length = 31)

Test GetSymbolInfo too? #Closed

AlekseyTs · 2017-01-06T22:00:52Z

            var discard3 = GetDiscardIdentifiers(tree).First();
-            var symbol = (IDiscardSymbol)model.GetSymbolInfo(discard3).Symbol; // returns null  https://github.com/dotnet/roslyn/issues/15450
-            //Assert.Equal("System.Int32", symbol.Type.ToTestDisplayString());
+            var discard3Symbol = (IDiscardSymbol)model.GetSymbolInfo(discard3).Symbol;


model.GetSymbolInfo(discard3) [](start = 49, length = 29)

Test GetTypeInfo too? #Closed

AlekseyTs · 2017-01-06T22:02:49Z

            var declaration2 = (DeclarationPatternSyntax)discard2.Parent;
            Assert.Equal("var _", declaration2.ToString());
-            //Assert.Equal("", model.GetTypeInfo(declaration2).Type.ToTestDisplayString()); // https://github.com/dotnet/roslyn/issues/15450
+            Assert.Null(model.GetTypeInfo(declaration2).Type);


model.GetTypeInfo(declaration2) [](start = 24, length = 31)

Test GetSymbolInfo too, if applicable? #Closed

AlekseyTs · 2017-01-06T22:05:05Z

Done with review pass. #Closed

AlekseyTs · 2017-01-07T00:16:16Z


            var discard1 = GetDiscardDesignations(tree).ElementAt(0);
            Assert.Null(model.GetDeclaredSymbol(discard1));
+            Assert.Equal(null, model.GetSymbolInfo(discard1).Symbol);


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

Assert.Null? #Closed

AlekseyTs · 2017-01-07T00:17:42Z

-            //Assert.Equal("System.Int32", symbol.Type.ToTestDisplayString());
+            Assert.Equal("System.Int32", model.GetTypeInfo(discard3).Type.ToTestDisplayString());
+            var discard3Symbol = (IDiscardSymbol)model.GetSymbolInfo(discard3).Symbol;
+            Assert.Equal("System.Int32", discard3Symbol.Type.ToTestDisplayString());


discard3Symbol.Type.ToTestDisplayString() [](start = 41, length = 41)

Print symbol instead? #Closed

AlekseyTs · 2017-01-07T00:24:11Z

        {
            var symbolInfo = model.GetSymbolInfo(decl);
-            Assert.Null(symbolInfo.Symbol);
+            if (expectedSymbol != null)


if (expectedSymbol != null) [](start = 11, length = 28)

Please remove the 'if', we should assert that we get null when null is expected. #Closed

AlekseyTs · 2017-01-07T00:25:00Z

            var typeInfo = model.GetTypeInfo(decl);
-            Assert.Null(typeInfo.Type);
-            Assert.Null(typeInfo.ConvertedType);
+            if (expectedType != null)


if (expectedType != null) [](start = 12, length = 25)

Remove if? #Closed

AlekseyTs · 2017-01-07T00:43:23Z

LGTM

… and GlobalExpressionVariable

jcouv · 2017-01-07T07:09:40Z

@srivatsn for ask mode approval. Thanks

dnfclas added the cla-already-signed label Dec 30, 2016

jcouv self-assigned this Dec 30, 2016

jcouv added Area-Compilers Feature - Out Variable Declaration Out Variable Declaration Feature - Tuples Tuples labels Dec 30, 2016

jcouv added this to the 2.0 (RC.3) milestone Dec 30, 2016

jcouv changed the title ~~Decon semantic~~ Semantic model APIs on declaration expressions and discards Dec 30, 2016

jcouv force-pushed the decon-semantic branch 5 times, most recently from 219545b to ce28bd6 Compare January 5, 2017 18:51

jcouv added 3 commits January 5, 2017 10:52

Bound node for deconstruction includes bound tuple expression

9a224ed

Fixing semantic model for d-foreach

ee04d54

Fixing syntax attached to bound nodes

6fd5606

jcouv force-pushed the decon-semantic branch from ce28bd6 to 63f4475 Compare January 5, 2017 19:16

Fixed remaining tests

60b371c

jcouv force-pushed the decon-semantic branch from 63f4475 to 60b371c Compare January 5, 2017 19:52

gafter approved these changes Jan 5, 2017

View reviewed changes

AlekseyTs reviewed Jan 6, 2017

View reviewed changes

PR feedback (1)

e889fec

AlekseyTs reviewed Jan 6, 2017

View reviewed changes

jcouv force-pushed the decon-semantic branch 2 times, most recently from a80e3b8 to af52303 Compare January 6, 2017 23:16

PR feedback (2)

8e0e800

jcouv force-pushed the decon-semantic branch from af52303 to 8e0e800 Compare January 6, 2017 23:25

jcouv added 2 commits January 6, 2017 16:05

Removed custom visit methods

f38d3c3

Stricter checks on type and symbol in out vars

7e7fcdf

AlekseyTs reviewed Jan 7, 2017

View reviewed changes

jcouv mentioned this pull request Jan 7, 2017

Semantic model: Should GetSymbolInfo on var _ return an IDiscardSymbol? #16316

Open

Assert.Null

f9f93a5

jcouv force-pushed the decon-semantic branch from 217c664 to f9f93a5 Compare January 7, 2017 00:23

AlekseyTs reviewed Jan 7, 2017

View reviewed changes

Remove if

6aaf3e9

Avoid divergence of inferred type between OutVariablePendingInference…

62ba60d

… and GlobalExpressionVariable

srivatsn added the Approved to merge label Jan 7, 2017

jcouv merged commit 4305568 into dotnet:master Jan 7, 2017

jcouv deleted the decon-semantic branch January 7, 2017 20:04

This was referenced Jan 7, 2017

Update semantic model APIs for discard and deconstruction #15450

Closed

Semantic model gives incorrect answer on type in out var combined with discard #16361

Closed

		@@ -690,7 +699,7 @@ internal BoundDeconstructionAssignmentOperator BindDeconstruction(
		{
		DeconstructionVariable locals = BindDeconstructionVariables(left, diagnostics, ref declaration, ref expression);

Uh oh!

Conversation

jcouv commented Dec 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jan 6, 2017 • 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!

AlekseyTs Jan 6, 2017 • 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 Jan 6, 2017 • 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 Jan 6, 2017 • 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!

AlekseyTs Jan 6, 2017 • 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!

AlekseyTs Jan 6, 2017 • 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!

AlekseyTs Jan 6, 2017 • 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 Jan 6, 2017 • 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!

AlekseyTs Jan 6, 2017 • 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!

AlekseyTs Jan 6, 2017 • 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 Jan 6, 2017

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 6, 2017 • 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 Jan 6, 2017

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 6, 2017 • 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!

AlekseyTs Jan 6, 2017 • 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!

AlekseyTs Jan 6, 2017 • 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!

AlekseyTs Jan 6, 2017 • 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 commented Dec 30, 2016 •

edited

Loading

jcouv commented Jan 5, 2017 •

edited

Loading

jcouv commented Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

jcouv Jan 6, 2017 •

edited

Loading

jcouv Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

jcouv Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 6, 2017 •

edited

Loading

AlekseyTs commented Jan 6, 2017 •

edited

Loading

AlekseyTs Jan 7, 2017 •

edited

Loading

AlekseyTs Jan 7, 2017 •

edited

Loading

AlekseyTs Jan 7, 2017 •

edited

Loading

AlekseyTs Jan 7, 2017 •

edited

Loading