Combine deconstruction assignment and declaration, and support discards. #15548

gafter · 2016-11-27T22:47:56Z

Combine deconstruction assignment and declaration, and support discards.
Because tuples may contain declarations, adjust lambda disambiguation
and adjust parsing of argument lists.
Diagnose tuple element names on the left of a deconstruction.
Add relational operators to disambiguating tokens in 7.5.4.2
Remove BoundVoid

Update (from jcouv): the bug numbers seem wrong. I think the correct ones were #14794
and #14832

/cc @jcouv @AlekseyTs @VSadov @dotnet/roslyn-compiler

- Combine deconstruction assignment and declaration, and support discards. - Add wildcards.work.md to track outstanding work. - Bind each type syntax once in a deconstruction. - Because tuples may contain declarations, adjust lambda disambiguation and adjust parsing of argument lists. - Diagnose tuple element names on the left of a deconstruction. - Add relational operators to disambiguating tokens in 7.5.4.2

gafter · 2016-11-28T17:38:19Z

src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs

+                    {
+                        var pending = (BoundDiscardedExpression)expression;
+                        if ((object)pending.Type == null)
+                        {


Since the caller has already checked this, I will remove the if and add an assertion that the type is null. #Resolved

cston · 2016-11-28T18:07:37Z

src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs

@@ -719,14 +744,20 @@ private DeconstructionVariable BindDeconstructionVariables(ExpressionSyntax node
            }
        }

+        private BoundDiscardedExpression BindDiscardedExpression(
+            DiscardedDesignationSyntax designation,
+            TypeSymbol declType)


static #Closed

cston · 2016-11-28T18:11:39Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+        }
+
+        // Bind a declaration variable where it isn't permitted.
+        private BoundExpression BindDeclarationVariables(TypeSymbol declType, VariableDesignationSyntax node, DiagnosticBag diagnostics)


Is this comment correct? From the body of the method, it's not clear that the declaration variable isn't permitted. #ByDesign

While it is true that the code implements them, this is used only to bind them in contexts in which they are not currently permitted, via the method above which gives an error message. As you can see, the type inference (line 735 below) is short-circuited to produce an error type.

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

cston · 2016-11-28T18:17:54Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+                            builder.Add(BindDeclarationVariables(declType, n, diagnostics));
+                        }
+                        var subExpressions = builder.ToImmutableAndFree();
+                        var tupleType = (TypeSymbol)this.Compilation.CreateTupleTypeSymbol(subExpressions.Select<BoundExpression, ITypeSymbol>(e => e.Type).ToImmutableArray());


Consider using TupleTypeSymbol directly rather than ITypeSymbol, etc.:

TupleTypeSymbol.Create( null, subExpressions.SelectAsArray(e => e.Type), default(ImmutableArray<Location>), default(ImmutableArray<string>, Compilation); ``` #Resolved

cston · 2016-11-28T18:19:38Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+                        }
+                        var subExpressions = builder.ToImmutableAndFree();
+                        var tupleType = (TypeSymbol)this.Compilation.CreateTupleTypeSymbol(subExpressions.Select<BoundExpression, ITypeSymbol>(e => e.Type).ToImmutableArray());
+                        return new BoundTupleLiteral(node, ImmutableArray<string>.Empty, subExpressions, tupleType);


Should argumentNamesOpt be default(ImmutableArray<string>) rather than Empty? #Resolved

cston · 2016-11-28T18:23:20Z

src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs

+                // We only allow assignment-only or declaration-only deconstructions at this point.
+                // Issue https://github.com/dotnet/roslyn/issues/15050 tracks allowing mixed deconstructions.
+                // For now we give an error when you mix.
+                Error(diagnostics, ErrorCode.ERR_MixedDeconstructionUnsupported, left);


Please add a test for this case. #Resolved

There already is one: MixedDeconstruction_03 in DeconstructionTests.cs

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

cston · 2016-11-28T18:24:48Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

+        ERR_MixedDeconstructionUnsupported = 8184,
+        ERR_DeclarationExpressionNotPermitted = 8185,
+        ERR_MustDeclareForeachIteration = 8186,
+        ERR_TupleElementNamesInDeconstruction = 8187,


Please add tests for each of these diagnostics. #Resolved

For ERR_MixedDeconstructionUnsupported: four tests in Emit\CodeGen\CodeGenDeconstructTests.cs, and two in Syntax\Parsing\DeconstructionTests.cs

for ERR_DeclarationExpressionNotPermitted: many tests in Emit\CodeGen\CodeGenTupleTest.cs, Semantic\Semantics\DeconstructionTests.cs, Semantic\Semantics\LocalFunctionTests.cs, Semantic\Semantics\OutVarTests.cs, and Semantic\Semantics\PatternSwitchTests.cs

for ERR_MustDeclareForeachIteration: two tests in Emit\CodeGen\CodeGenDeconstructTests.cs

for ERR_TupleElementNamesInDeconstruction: two tests in Emit\CodeGen\CodeGenDeconstructTests.cs

Are you saying that you don't think these tests are sufficient?

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

Sorry, I missed those tests since the webpage did not include those in the diff. #Resolved

cston · 2016-11-28T18:28:59Z

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

@@ -255,7 +277,7 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
            hasErrors = hasErrors || iterationVariableType.IsErrorType();

            // Skip the conversion checks and array/enumerator differentiation if we know we have an error (except local name conflicts).
-            if (hasErrors)
+            if (hasErrors || _syntax.HasErrors)


Is the check for _syntax.HasErrors necessary? What if the error is something simple such as missing in token? #Resolved

I'm fine with suppressing semantic errors in the face of syntax errors, given that we've bound all the pieces of the syntax needed for a good IDE/SemanticModel experience.

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

I'll change the syntax errors check to check for syntax errors only on the declaration part. #Resolved

This is now a semantic restriction (until we decide to remove it).

cston · 2016-11-29T17:59:37Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

-            return (deconstruction == null)
-                ? (CommonForEachStatementSyntax)_syntaxFactory.ForEachStatement(@foreach, openParen, type, name, @in, expression, closeParen, statement)
-                : (CommonForEachStatementSyntax)_syntaxFactory.ForEachVariableStatement(@foreach, openParen, deconstruction, @in, expression, closeParen, statement);
+        private bool IsValidForeachVariable(ExpressionSyntax variable)


private [](start = 8, length = 7)

static #Resolved

AlekseyTs · 2016-11-29T19:12:24Z

src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs

+        // differently, as we check for the comma before concluding that the identifier should cause a
+        // disambiguation. For example, for the input `(A < B , C > D)`, we treat this as a tuple with
+        // two elements, because if we considered the `A<B,C>` to be a type, it wouldn't be a tuple at
+        // all. Since we don't have such as thing as a one-element tuple (even for deconstruction), the


such as thing [](start = 36, length = 13)

Typo "such as thing" vs. "such a thing"? #Closed

AlekseyTs · 2016-11-29T19:30:07Z

src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs

+                }
+
+                // check for a designation
+                if (!ScanDesignation(permitTupleDesignation && (lastTokenOfType.IsVar() || IsPredefinedType(lastTokenOfType.Kind))))


lastTokenOfType.IsVar() [](start = 64, length = 23)

Will this condition do the right thing for a type name "A.var"? #Closed

Probably not, as we'll want A.var(x, y) to be an invocation expression. I'll revise it. #Resolved

AlekseyTs · 2016-11-29T19:50:36Z

src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs

+                    return false;
+                case SyntaxKind.IdentifierToken:
+                    var result = this.IsTrueIdentifier();
+                    this.EatToken();


this.EatToken(); [](start = 20, length = 16)

It doesn't feel appropriate to eat the token when we are about to return false. #Closed

When we return false, the caller unwinds the trial parse, so it doesn't matter. #ByDesign

I think it would be cleaner and more intuitive if this function (and the like) wouldn't consume tokens it doesn't accept. #Closed

That isn't possible given the way we scan forward and discard tokens before making a decision. Even if we don't eat the very last token, we will have eaten preceding tokens that we don't accept. Avoiding eating the very last token would be extra work that serves no purpose. #ByDesign

AlekseyTs · 2016-11-29T19:51:24Z

src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs

+                        switch (this.CurrentToken.Kind)
+                        {
+                            case SyntaxKind.CloseParenToken:
+                                this.EatToken();


this.EatToken(); [](start = 32, length = 16)

It doesn't feel appropriate to eat the token when we are about to return false. #Closed

As with other scan methods, it is unspecified what tokens have been eaten when it returns false. #ByDesign

AlekseyTs · 2016-11-29T20:01:38Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -5346,6 +5343,13 @@ private bool IsTrueIdentifier()
            return false;
        }

+        private bool IsTrueIdentifier(SyntaxToken token)


IsTrueIdentifier [](start = 21, length = 16)

Should the method above delegate its work to this method? Otherwise, the methods have the same name, but appear to use different criteria. #Closed

Good idea, but not quite so simple to do, as there are a series of helper methods for the previous method intended to help it disambiguate, implicitly based on the current token in declarations; it peeks forward using PeekToken(int) and even does some trial forward parsing.

This method is for use in executable code (not declarations), so it has a much simpler implementation that depends only on the token passed in and this.IsInQuery. I'll add some clarifying doc comments. #Resolved

AlekseyTs · 2016-11-29T20:43:57Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                    if ((options & NameOptions.AfterIsOrCaseOrOutOrTupleComma) != 0 ||
+                        (options & NameOptions.FirstElementOfPossibleTupleLiteral) != 0 && this.PeekToken(1).Kind == SyntaxKind.CommaToken)
+                    {
+                        // we allow 'G<T,U> x' as a pattern-matching operation and a declaration pattern in a tuple.


declaration pattern [](start = 85, length = 20)

Typo "declaration pattern in a tuple"? #Closed

AlekseyTs · 2016-11-29T21:03:31Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+            var type = this.ParseUnderlyingType(parentIsParameter: mode == ParseTypeMode.Parameter, options: nameOptions);
+
+            if (this.CurrentToken.Kind == SyntaxKind.QuestionToken &&
+                (mode != ParseTypeMode.AfterIsOrCase || !IsTrueIdentifier(this.PeekToken(1))))


(mode != ParseTypeMode.AfterIsOrCase || !IsTrueIdentifier(this.PeekToken(1)))) [](start = 16, length = 78)

Consider adding a comment explaining the rational behind this new condition. #Closed

AlekseyTs · 2016-11-29T21:10:49Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

-            if (tk == SyntaxKind.RefKeyword ||
-                (SyntaxFacts.IsPredefinedType(tk) && this.PeekToken(1).Kind != SyntaxKind.DotToken) || IsDeclarationModifier(tk))
+            if (tk == SyntaxKind.RefKeyword || IsDeclarationModifier(tk) ||
+                (SyntaxFacts.IsPredefinedType(tk) && this.PeekToken(1).Kind != SyntaxKind.DotToken && this.PeekToken(1).Kind != SyntaxKind.OpenParenToken))


this.PeekToken(1).Kind != SyntaxKind.OpenParenToken [](start = 102, length = 51)

Consider adding a clarifying comment for this condition. #Closed

AlekseyTs · 2016-11-29T21:28:30Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

-                        var assignment = (AssignmentExpressionSyntax)expression;
-                        if (assignment.Left.IsDeconstructionDeclarationLeft())
-                        {
-                            statement = this.AddError(statement, ErrorCode.ERR_BadEmbeddedStmt);


statement = this.AddError(statement, ErrorCode.ERR_BadEmbeddedStmt); [](start = 28, length = 68)

It is not clear why this error is no longer reported. Was there an explicit LDM decision? #Closed

Yes, the LDM explicitly decided to remove the language concept of a deconstruction declaration statement, which is where this restriction arises. We will discuss tomorrow if the LDM wants to add back any of these restrictions in C# 7, including the ones among the first changes in Binder_Deconstruct.cs (lines 26-57 in this PR); as of the last LDM they wanted to remove all those restrictions. But when we do we will need additional tests. #ByDesign

The way I read the response, and based on my recollection of the LDM discussion, the answer to my question "Was there an explicit LDM decision?" is No. Meaning the LDM did not make an explicit decision about this error. So, let's hold on on removing it until the decision is made. #Closed

You misread the response. We did discuss the error, I raised the point that it is no longer an error if it is not a declaration when in the position of a controlled statement, and that was OK with Mads and others. If the LDM explicitly decides to change their mind and re-introduce an error here, I will do that. #Resolved

The LDM met yesterday and reconfirmed this. From the raw LDM notes:

Declarations in embedded statements?

Let's not add errors for deconstruction situations. In principle we probably want to remove even the existing limitation, but it's work we won't push for in C# 7.0.
#Resolved

AlekseyTs · 2016-11-29T21:30:27Z

        Debug.Assert(this.CurrentToken.Kind == SyntaxKind.ForKeyword || this.CurrentToken.Kind == SyntaxKind.ForEachKeyword);

Should this condition be now removed? #Closed

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:7832 in 1003470. [](commit_id = 1003470, deletion_comment = False)

AlekseyTs · 2016-11-29T21:33:00Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -7812,28 +7844,22 @@ private StatementSyntax ParseForOrForEachStatement()
            var resetPoint = this.GetResetPoint();
            try
            {
-                if (this.CurrentToken.Kind == SyntaxKind.ForKeyword)
+                Debug.Assert(this.CurrentToken.Kind == SyntaxKind.ForKeyword);


Debug.Assert(this.CurrentToken.Kind == SyntaxKind.ForKeyword); [](start = 16, length = 62)

It looks like we already asserted this at the beginning of this function. #Closed

cston · 2016-11-29T21:55:59Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                    this.CurrentToken.Kind == SyntaxKind.CloseBracketToken)
+                {
+                    // convert `]` into `)` or vice versa for error recovery
+                    closeToken = this.ConsumeOneToken(closeKind);


this.ConsumeOneToken(closeKind) [](start = 33, length = 31)

Why consume the ']' or ')' if it's not the close kind expected? Is it more likely the user closed with the wrong kind or that the expected kind is missing? #Resolved

We currently parse a C++-style declaration
Type x(args, ...)
using the bracketed argument list, which our syntax trees support. If we don't turn the ( into a [, then we get out of sync and will think the argument list is a tuple literal. And then we won't parse out variable declarations properly. Doing the same substitution for the close paren reduces cascaded diagnostics. #Resolved

AlekseyTs · 2016-12-07T20:56:41Z

<Field Name="Variable" Type="ExpressionSyntax">

Should the list of kinds within this element be adjusted? #Closed

Refers to: src/Compilers/CSharp/Portable/Syntax/Syntax.xml:2120 in c4d4991. [](commit_id = c4d4991, deletion_comment = False)

AlekseyTs · 2016-12-07T20:58:57Z

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

@@ -99,7 +99,8 @@ protected override ImmutableArray<LocalSymbol> BuildLocals()
                case SyntaxKind.IdentifierName:
                    break;
                default:
-                    throw ExceptionUtilities.UnexpectedValue(declaration.Kind());
+                    ExpressionVariableFinder.FindExpressionVariables(this, locals, declaration);


ExpressionVariableFinder.FindExpressionVariables(this, locals, declaration); [](start = 20, length = 76)

Could you please add a comment about the scenario when we get here and point me to the test(s) that covers this line of code? #Closed

Sure. The tests would be UnitTests.CodeGen.CodeGenDeconstructTests.MixedDeconstruction_06, UnitTests.CodeGen.CodeGenDeconstructTests.ForeachIntoExpression, and UnitTests.CodeGen.CodeGenDeconstructTests.MixedDeconstruction_05. Comments in the next iteration.

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

AlekseyTs · 2016-12-07T21:24:41Z

            // for error recovery, we parse `int (x, y) = e` as a deconstruction.

Should we keep the comment, is it still valid? #Closed

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:9220 in 1003470. [](commit_id = 1003470, deletion_comment = True)

AlekseyTs · 2016-12-07T21:31:58Z

src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs

+        // absence of the comma after the `D` means we don't treat the `D` as contributing to the
+        // disambiguation of the expression/type. More formally, ...
+        //
+        // If a sequence of tokens can be parsed(in context) as a* simple-name* (§7.6.3), *member-access* (§7.6.5),


member-access [](start = 90, length = 15)

Consider removing the ** annotations throughout this comment. #Closed

I'd prefer to keep the markup from the specification.

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

Is this a* simple-name* intended markup, note position of the first *? There are several similar places throughout this comment.

In reply to: 91453230 [](ancestors = 91453230,91392143)

That is what is in the draft spec. I agree it could be improved ever so slightly. If this is what is holding up your approval of the code review, I think we should hold this set of changes until I am back in the office in January.

In reply to: 91570733 [](ancestors = 91570733,91453230,91392143)

Let's not block on the markup here.

AlekseyTs · 2016-12-07T21:47:08Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

-                return result;
+                if ((object)this._type == null)
+                {
+                    diagnostics.Add(this.ForbiddenDiagnostic, Locations[0], Name);


diagnostics.Add(this.ForbiddenDiagnostic, Locations[0], Name); [](start = 20, length = 62)

It is not clear why ForbiddenDiagnostic is an appropriate diagnostics to use in this case. Also, could you please point me to the test(s) covering this line? #Closed

I don't know of a way to trigger this defensive code. I'll remove it and let the assertion in the caller fail if it (_type == null) occurs.

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

AlekseyTs · 2016-12-07T21:48:47Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

+                {
+                    diagnostics.Add(this.ForbiddenDiagnostic, Locations[0], Name);
+                    SetType(new ExtendedErrorTypeSymbol(
+                        ((SourceModuleSymbol)this.ContainingModule).DeclaringCompilation, name: "var", arity: 0, errorInfo: null, variableUsedBeforeDeclaration: true));


variableUsedBeforeDeclaration: true) [](start = 130, length = 36)

Why does it make sense to set variableUsedBeforeDeclaration to true here and what effect is this going to have? #Closed

It wouldn't have any effect other than recording that the variable's type was required at a location before it was able to be inferred.

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

AlekseyTs · 2016-12-07T21:50:03Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

-                Debug.Assert((object)this._type != null);
+                if ((object)this._type == null)
+                {
+                    diagnostics.Add(this.ForbiddenDiagnostic, Locations[0], Name);


diagnostics.Add(this.ForbiddenDiagnostic, Locations[0], Name); [](start = 20, length = 62)

It is not clear why ForbiddenDiagnostic is an appropriate diagnostics to use in this case. Also, could you please point me to the test(s) covering this line? #Closed

It feels that ERR_TypeInferenceFailedForImplicitlyTypedOutVariable or the like would be more appropriate in this case. Also, it looks like there is no guarantee that this diagnostics is ever going to be surfaced to the user. It would be good to enforce that we are going to get here only for broken code (have syntax errors or something). I would prefer a throw, but an assert should be fine as well.

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

Indeed, diagnostics from this method are always discarded. I'll remove it.

This is covered by OutVarTests.OutVarDeconstruction_02

There isn't a mechanism to "enforce" that we only get here for broken code, because we don't have the necessary compilation diagnostics in hand. Ideas for doing that are welcome.

In reply to: 91397639 [](ancestors = 91397639,91395620)

There isn't a mechanism to "enforce" that we only get here for broken code, because we don't have the necessary compilation diagnostics in hand. Ideas for doing that are welcome.

I believe we should never get here for an Out Var or a pattern variable. It should be fairly easy to detect such cases by examining the syntax. Please add the detection and a throw for them.

In reply to: 91455715 [](ancestors = 91455715,91397639,91395620)

AlekseyTs · 2016-12-08T00:17:05Z

src/EditorFeatures/CSharpTest/Diagnostics/GenerateMethod/GenerateMethodTests.cs

@@ -7246,7 +7246,7 @@ public void M1()
 parseOptions: TestOptions.Regular);
        }

-        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsGenerateMethod)]
+        [Fact(Skip = "https://github.com/dotnet/roslyn/issues/15508"), Trait(Traits.Feature, Traits.Features.CodeActionsGenerateMethod)]


Skip = "#15508" [](start = 14, length = 54)

It is fine to merge with skipped test into a feature branch, but I think we should come up with solution or implement a workaround that prevents the crash before we merge this to master. #Closed

Agree. This is an ask mode change to RC. We can't introduce new IDE regressions here without explicit IDE buy off. Otherwise this will not pass the ask mode review. #Resolved

That (not crashing) was the reason for the addition of "return false" in "CSharpTypeInferenceService.TypeInferrer.cs".

I'll adjust this test so it reflects the "broken" but non-crashing behavior.

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

AlekseyTs · 2016-12-08T00:34:25Z

Done with review pass. A number of active comments remains, I think I reactivated a several existing threads as well. #Closed

gafter · 2016-12-08T07:18:44Z

<Field Name="Variable" Type="ExpressionSyntax">

I will remove the list.

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

Refers to: src/Compilers/CSharp/Portable/Syntax/Syntax.xml:2120 in c4d4991. [](commit_id = c4d4991, deletion_comment = False)

gafter · 2016-12-08T07:20:02Z

            // for error recovery, we parse `int (x, y) = e` as a deconstruction.

Yes.

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

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:9220 in 1003470. [](commit_id = 1003470, deletion_comment = True)

…iew.

gafter · 2016-12-08T07:33:45Z

git repository authentication is broken again, so I can't push to the repository. When I can I'll send the next iteration, which addresses all comments. #Resolved

gafter · 2016-12-08T09:14:56Z

            // for error recovery, we parse `int (x, y) = e` as a deconstruction.

More specifically, this is now the name of a kind of assignment expression in which the left-hand-side is of one of a couple of forms.

In reply to: 265670754 [](ancestors = 265670754,265577755)

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:9220 in 1003470. [](commit_id = 1003470, deletion_comment = True)

gafter · 2016-12-08T09:20:43Z

All comments have been addressed. May I have a second code review, please?

AlekseyTs · 2016-12-08T18:37:07Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

@@ -670,9 +673,7 @@ protected override TypeSymbol InferTypeOfVarVariable(DiagnosticBag diagnostics)
                        throw ExceptionUtilities.UnexpectedValue(_deconstruction.Kind());
                }

-                TypeSymbol result = this._type;
-                Debug.Assert((object)result != null);


Debug.Assert((object)result != null); [](start = 16, length = 37)

Please restore the original assert. The earlier we detect the problem the better. #Closed

AlekseyTs · 2016-12-08T18:58:28Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

+{
+    static void Main(string[] args)
+    {
+        foreach (M(out var x1) in new[] { 1, 2, 3 })


foreach (M(out var x1) in new[] { 1, 2, 3 }) [](start = 8, length = 44)

Please add similar test for a var pattern variable. #Closed

See the very next test here.

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

AlekseyTs · 2016-12-08T19:09:32Z

src/EditorFeatures/CSharpTest/Formatting/Indentation/SmartIndenterTests.cs

@@ -2570,7 +2570,7 @@ class C
                expectedIndentation: 8);
        }

-        [WpfFact, Trait(Traits.Feature, Traits.Features.SmartIndent)]
+        [WpfFact(Skip = "test is no longer correct due to language changes"), Trait(Traits.Feature, Traits.Features.SmartIndent)]


[WpfFact(Skip = "test is no longer correct due to language changes") [](start = 8, length = 68)

Do we have an issue tracking this? #Closed

Tracking what? Language changes? The deletion of this test?

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

If the test is skipped, a follow-up should follow. It can take many forms: remove, adjust, test new scenarios enabled by the change. Deciding what to do will be a part of resolving that issue, unless we can do that in context of this change.

In reply to: 91585635 [](ancestors = 91585635,91584597)

We should not be disabling tests in this manner. If a test is disabled it needs a tracking issue. If it's no longer valid then it should be deleted and we should make the appropriate team (IDE in this case) aware of the change.

AlekseyTs · 2016-12-08T19:53:13Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

-                Debug.Assert((object)this._type != null);
+                if ((object)this._type == null)
+                {
+                    SetType(_nodeBinder.CreateErrorType("var"));


SetType(_nodeBinder.CreateErrorType("var")); [](start = 20, length = 44)

There isn't a mechanism to "enforce" that we only get here for broken code, because we don't have the necessary compilation diagnostics in hand. Ideas for doing that are welcome.

I believe we should never get here for an Out Var or a pattern variable. It should be fairly easy to detect such cases by examining the syntax. Please add the detection and a throw for them. #Closed

AlekseyTs · 2016-12-08T19:55:00Z

I am done with review pass. The remaining issues that I think should be addressed are:

Restoring an original assert lost between iterations in SourceLocalSymbol.cs Combine deconstruction assignment and declaration, and support discards. #15548 (review)
Creating an issue tracking a follow-up for a test disabled in SmartIndenterTests.cs Combine deconstruction assignment and declaration, and support discards. #15548 (review)
Adding an assert or a throw to catch unexpected type inference failure early in SourceLocalSymbol.cs Combine deconstruction assignment and declaration, and support discards. #15548 (review) #Closed

AlekseyTs · 2016-12-11T22:54:06Z

LGTM

jcouv · 2016-12-12T02:36:02Z

@dotnet-bot test linux_debug_prtest please

gafter added Area-Compilers Feature Request New Language Feature - Pattern Matching Pattern Matching New Language Feature - Tuples Tuples labels Nov 27, 2016

gafter added this to the 2.0 (RC.3) milestone Nov 27, 2016

gafter self-assigned this Nov 27, 2016

dnfclas added the cla-already-signed label Nov 27, 2016

gafter commented Nov 28, 2016

View reviewed changes

cston reviewed Nov 28, 2016

View reviewed changes

Disallow deconstruction declarations except at the statement level.

1003470

This is now a semantic restriction (until we decide to remove it).

cston reviewed Nov 29, 2016

View reviewed changes

AlekseyTs reviewed Nov 29, 2016

View reviewed changes

cston reviewed Nov 29, 2016

View reviewed changes

AlekseyTs reviewed Dec 7, 2016

View reviewed changes

AlekseyTs reviewed Dec 8, 2016

View reviewed changes

Minor changes to the handling of declaration expressions per code rev…

c5e4534

…iew.

AlekseyTs reviewed Dec 8, 2016

View reviewed changes

jcouv mentioned this pull request Dec 9, 2016

Confirm expected behavior for DontCreateIndentOperationForBrokenBracketedArgumentList after discard parsing changes #15813

Closed

Addressing blocking feedback while Neal is OOF

3e0d9db

jcouv force-pushed the wildcards-syntax7 branch from f9b33ce to 3e0d9db Compare December 10, 2016 09:31

jcouv merged commit 3806ef8 into dotnet:features/wildcard Dec 12, 2016

This was referenced Dec 12, 2016

Combine deconstruction assignment and declaration, and support discards #15826

Closed

Combine deconstruction assignment and declaration, and support discards (merging to master) #15842

Merged

gafter mentioned this pull request Dec 12, 2016

Remove BoundVoid #13913

Closed

gafter deleted the wildcards-syntax7 branch May 24, 2018 19:08

Combine deconstruction assignment and declaration, and support discards. #15548

Combine deconstruction assignment and declaration, and support discards. #15548

Conversation

gafter commented Nov 27, 2016 • edited by jcouv Loading

gafter Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

cston Nov 28, 2016 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Nov 28, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 28, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

cston Nov 28, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

cston Nov 28, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

cston Nov 28, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 28, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

cston Nov 28, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

cston Nov 29, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

gafter Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

gafter Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

gafter Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

gafter Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

gafter Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

gafter Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

gafter Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

gafter Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Declarations in embedded statements?

AlekseyTs commented Nov 29, 2016 • edited Loading

AlekseyTs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

cston Nov 29, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Dec 7, 2016 • edited Loading

AlekseyTs Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

gafter commented Nov 27, 2016 •

edited by jcouv

Loading

gafter Nov 28, 2016 •

edited

Loading

cston Nov 28, 2016 •

edited by jcouv

Loading

cston Nov 28, 2016 •

edited by gafter

Loading

cston Nov 28, 2016 •

edited by gafter

Loading

cston Nov 28, 2016 •

edited by gafter

Loading

cston Nov 28, 2016 •

edited by gafter

Loading

gafter Nov 28, 2016 •

edited

Loading

cston Nov 28, 2016 •

edited by gafter

Loading

cston Nov 28, 2016 •

edited by gafter

Loading

cston Nov 28, 2016 •

edited by gafter

Loading

gafter Nov 29, 2016 •

edited

Loading

cston Nov 29, 2016 •

edited by gafter

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

gafter Nov 29, 2016 •

edited

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

gafter Nov 29, 2016 •

edited

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

gafter Nov 30, 2016 •

edited

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

gafter Nov 30, 2016 •

edited

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

gafter Nov 30, 2016 •

edited

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

gafter Nov 30, 2016 •

edited

Loading

AlekseyTs Nov 30, 2016 •

edited

Loading

gafter Nov 30, 2016 •

edited

Loading

gafter Dec 1, 2016 •

edited

Loading

AlekseyTs commented Nov 29, 2016 •

edited

Loading

AlekseyTs Nov 29, 2016 •

edited

Loading

cston Nov 29, 2016 •

edited by gafter

Loading

gafter Nov 30, 2016 •

edited

Loading

AlekseyTs commented Dec 7, 2016 •

edited

Loading

AlekseyTs Dec 7, 2016 •

edited

Loading

AlekseyTs commented Dec 7, 2016 •

edited

Loading

AlekseyTs Dec 7, 2016 •

edited by gafter

Loading

AlekseyTs Dec 7, 2016 •

edited

Loading

gafter Dec 8, 2016 •

edited

Loading

AlekseyTs Dec 7, 2016 •

edited by gafter

Loading

AlekseyTs Dec 7, 2016 •

edited

Loading

AlekseyTs Dec 8, 2016 •

edited

Loading

jaredpar Dec 8, 2016 •

edited by gafter

Loading

AlekseyTs commented Dec 8, 2016 •

edited

Loading

gafter commented Dec 8, 2016 •

edited

Loading

AlekseyTs Dec 8, 2016 •

edited

Loading

AlekseyTs Dec 8, 2016 •

edited

Loading

AlekseyTs Dec 8, 2016 •

edited

Loading

AlekseyTs Dec 8, 2016 •

edited

Loading

AlekseyTs commented Dec 8, 2016 •

edited

Loading