Implement pattern-matching in top-level scripts #13999

gafter · 2016-09-22T23:14:06Z

in which pattern variables become fields.
Also aligns the overload resolution implementation with the spec for out var.
Fixes #10603

@AlekseyTs @agocke Please review.
/cc @dotnet/roslyn-compiler

…versionFromExpression` (issue ignored during code review)

in which pattern variables become fields. Fixes dotnet#10603

gafter · 2016-09-23T21:07:23Z

@AlekseyTs @agocke Please review.
#Resolved

gafter · 2016-09-26T17:36:39Z

@AlekseyTs @agocke Please review. #Resolved

gafter · 2016-09-26T22:48:10Z

@AlekseyTs @agocke Please review. #Resolved

gafter · 2016-09-27T17:04:19Z

@AlekseyTs @agocke Please review. #Resolved

jcouv · 2016-09-27T17:32:21Z

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

-                        if (written) NoteWrite(symbol, value, read);
+                        var pattern = (BoundDeclarationPattern)node;
+                        Symbol symbol = pattern.Variable as LocalSymbol;
+                        if (symbol != null)


Is it useful to cast to object here for null check?
Does the case were pattern.Variable is a field need to be handled too? If not, consider adding a comment. #Resolved

jcouv · 2016-09-27T17:33:45Z

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

-                        SetSlotState(slot, assigned: written || !this.State.Reachable);
-                        if (written) NoteWrite(symbol, value, read);
+                        var pattern = (BoundDeclarationPattern)node;
+                        Symbol symbol = pattern.Variable as LocalSymbol;


Maybe use var or LocalSymbol instead of Symbol? #Resolved

jcouv · 2016-09-27T17:41:08Z

public class PatternMatchingTests : PatternMatchingTestBase

The tests in this file seem to have been split to multiple files. Is there a way I tell which tests are new or modified with this PR? #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests.cs:16 in 59aca3e. [](commit_id = 59aca3e, deletion_comment = False)

jcouv · 2016-09-27T17:43:17Z

public class PatternMatchingTests : PatternMatchingTestBase

Nevermind. I think it is the test file named "Global" ;-)

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests.cs:16 in 59aca3e. [](commit_id = 59aca3e, deletion_comment = False)

jcouv · 2016-09-27T17:44:48Z

public class PatternMatchingTests_Global : PatternMatchingTestBase

Are those all duplicated tests modified to be scripts? #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_Global.cs:16 in 59aca3e. [](commit_id = 59aca3e, deletion_comment = False)

jcouv · 2016-09-27T17:46:30Z

src/Compilers/CSharp/Portable/CSharpExtensions.cs

@@ -1236,7 +1236,7 @@ public static ISymbol GetDeclaredSymbol(this SemanticModel semanticModel, Variab
        /// <summary>
        /// Given a declaration pattern syntax, get the corresponding symbol.


Consider updating comment: "get the corresponding symbol (local or field)". #WontFix

I'd rather not introduce an inconsistency with all the other places where this comment appears (e.g. for SingleVariableDesignationSyntax and VariableDeclaratorSyntax above)

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

jcouv · 2016-09-27T17:49:48Z

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

@@ -77,7 +77,12 @@ private void NoteDeclaredPatternVariables(BoundPattern pattern)
            if (IsInside && pattern.Kind == BoundKind.DeclarationPattern)
            {
                var decl = (BoundDeclarationPattern)pattern;
-                _variablesDeclared.Add(decl.LocalSymbol);
+                if (decl.Variable.Kind == SymbolKind.Local)


I don't recall modifying this file/walker for d-declaration in script. Should I have? #WontFix

It depends how this walker handles Deconstruction Declaration. It shouldn't try to add fields into _variablesDeclared list. You should definitely consider adding tests for region analysis for Deconstruction Declaration.

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

You didn't modify this file because the relevant APIs were never implemented for deconstruction. I'll submit a bug with a test case.

In reply to: 80756845 [](ancestors = 80756845,80753230)

AlekseyTs · 2016-09-27T18:28:33Z

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

@@ -1562,12 +1562,14 @@
  </AbstractNode>

  <Node Name="BoundDeclarationPattern" Base="BoundPattern">
-    <Field Name="LocalSymbol" Type="LocalSymbol" Null="disallow"/>
+    <Field Name="Variable" Type="Symbol" Null="disallow"/>


[](start = 4, length = 54)

Please add a comment what kind of symbols could end up here. #Closed

gafter · 2016-09-27T19:04:20Z

public class PatternMatchingTests_Global : PatternMatchingTestBase

I'm not sure I understand your question. These tests correspond 1-to-1 with tests for similar cases involving out variables.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_Global.cs:16 in 59aca3e. [](commit_id = 59aca3e, deletion_comment = False)

gafter · 2016-09-27T19:29:01Z

All review comments have been addressed. #Resolved

jcouv

LGTM

AlekseyTs · 2016-09-27T20:48:01Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

@@ -1748,11 +1736,13 @@ private BetterResult BetterConversionFromExpression(BoundExpression node, TypeSy
        {
            okToDowngradeToNeither = false;

-            if (considerRefKinds)
+            var argumentKind = node.Kind;
+            if (considerRefKinds || argumentKind == BoundKind.OutVariablePendingInference || argumentKind == BoundKind.OutDeconstructVarPendingInference)


if (considerRefKinds || argumentKind == BoundKind.OutVariablePendingInference || argumentKind == BoundKind.OutDeconstructVarPendingInference) [](start = 12, length = 141)

I believe merging handling of BoundKind.OutVariablePendingInference and BoundKind.OutDeconstructVarPendingInference into this if complicates the code significantly. The behavior we want for these nodes is very simple and it is better to handle them separately, so that we can quote the spec and have an easy way to match implementation to the spec. It is nearly impossible to match the current implementation to the spec. #Closed

The behavior we want arises from the specification for exactly matching expression. I will implement it that way, now that we have that tentative spec language.

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

The behavior we want arises from the specification for exactly matching expression.

Please quote what you think the tentative specification is. I am not able to verify correctness of the change otherwise.

In reply to: 80793171 [](ancestors = 80793171,80791006)

I will include the quote in a comment.

In reply to: 80794118 [](ancestors = 80794118,80793171,80791006)

There is an open issue for the spec/implementation misalignment in #14177 and an open discussion in the LDM. When it is settled I will implement and document the new spec.

In reply to: 80795408 [](ancestors = 80795408,80794118,80793171,80791006)

Please revert change to this file then.

In reply to: 80800555 [](ancestors = 80800555,80795408,80794118,80793171,80791006)

Closed the issue, as it does not occur in the ECMA draft for our next spec. I've updated the spec and the implementation to align as close as the shape of our implementation allows.

In reply to: 80800555 [](ancestors = 80800555,80795408,80794118,80793171,80791006)

…-10603

AlekseyTs · 2016-09-27T21:15:17Z

src/Compilers/CSharp/Portable/Compilation/SyntaxTreeSemanticModel.cs

        {
            var memberModel = this.GetMemberModel(declarationSyntax);
-            return memberModel == null ? null : memberModel.GetDeclaredSymbol(declarationSyntax, cancellationToken);
+            return memberModel?.GetDeclaredSymbol(declarationSyntax, cancellationToken) ??
+                GetEnclosingBinder(declarationSyntax.Position)?.LookupDeclaredField(declarationSyntax, declarationSyntax.Identifier.ValueText);


, declarationSyntax.Identifier.ValueText [](start = 101, length = 40)

Consider using an overload that doesn't need this argument. #Closed

AlekseyTs · 2016-09-27T21:24:52Z

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

@@ -106,7 +106,7 @@ private Symbol GetNodeSymbol(BoundNode node)
                {
                    case BoundKind.DeclarationPattern:
                        {
-                            return ((BoundDeclarationPattern)node).LocalSymbol;
+                            return ((BoundDeclarationPattern)node).Variable as LocalSymbol;


return ((BoundDeclarationPattern)node).Variable as LocalSymbol; [](start = 28, length = 63)

Consider simply delegating to VariableAccess. #Closed

VariableAccess isn't a symbol, and doesn't represent the logical symbol represented by this node.

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

AlekseyTs · 2016-09-27T21:30:32Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Patterns.cs

@@ -52,12 +52,12 @@ private BoundExpression LowerConstantPattern(BoundConstantPattern pattern, Bound

        private BoundExpression LowerDeclarationPattern(BoundDeclarationPattern pattern, BoundExpression input)
        {
-            Debug.Assert(pattern.IsVar || pattern.LocalSymbol.Type == pattern.DeclaredType.Type);
+            Debug.Assert(pattern.IsVar || pattern.Variable.GetTypeOrReturnType() == pattern.DeclaredType.Type);


pattern.IsVar [](start = 25, length = 14)

What is the purpose of IsVar condition? Shouldn't Variable have type assigned before we get here? #Closed

AlekseyTs · 2016-09-27T21:32:49Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_PatternSwitchStatement.cs

@@ -320,7 +320,7 @@ private void AddBindings(ArrayBuilder<BoundStatement> sectionBuilder, ImmutableA
                {
                    var source = kv.Key;
                    var dest = kv.Value;


Both expressions should be lowered. #Closed

Both are required to be of a form that no further local lowering is possible.

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

We already had this discussion in context of #13535. There is a simple rule that implementation of LocalRewriter should follow - every node that is a part of the input bound tree should be lowered. Following this simple rule doesn't create any correctness problems, but at the same time prevents us from introducing hard to find bugs and makes local rewriter robust to future changes in the pipeline.

In reply to: 80815417 [](ancestors = 80815417,80800519)

Thank you for making this change.

In reply to: 80849000 [](ancestors = 80849000,80815417,80800519)

AlekseyTs · 2016-09-27T22:12:54Z

src/Compilers/CSharp/Portable/Symbols/MemberSymbolExtensions.cs

@@ -453,48 +453,6 @@ internal static ImmutableArray<Symbol> GetExplicitInterfaceImplementations(this
            }
        }

-        internal static TypeSymbol GetTypeOrReturnType(this Symbol member)


internal static TypeSymbol GetTypeOrReturnType(this Symbol member) [](start = 8, length = 66)

Could you please share the rational behind moving these helpers to SymbolExtensions.cs? #Closed

They now act on symbols.

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

Are you referring to the rename of the "this" parameter, which was and still is a Symbol? This is applicable to many other methods in this file. Why stop at this two?

In reply to: 80815690 [](ancestors = 80815690,80807640)

I am referring to the fact that they operate on symbols that are not members as well as symbols that are members.

In reply to: 80849752 [](ancestors = 80849752,80815690,80807640)

…efactoring the whole thing)

AlekseyTs · 2016-09-27T23:08:02Z

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

+        protected override TypeSyntax TypeSyntax => (TypeSyntax)_typeSyntax.GetSyntax();
+        protected override SyntaxTokenList ModifiersTokenList => default(SyntaxTokenList);
+        public override bool HasInitializer => false;
+        protected override ConstantValue MakeConstantValue(HashSet<SourceFieldSymbolWithSyntaxReference> dependencies, bool earlyDecodingWellKnownAttributes, DiagnosticBag diagnostics) => null;


protected override ConstantValue MakeConstantValue(HashSet dependencies, bool earlyDecodingWellKnownAttributes, DiagnosticBag diagnostics) => null; [](start = 8, length = 185)

This line is too long. Please split it. #Closed

AlekseyTs · 2016-09-27T23:12:06Z

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

@@ -323,7 +323,7 @@ public sealed override bool HasInitializer
            get { return _hasInitializer; }
        }

-        public VariableDeclaratorSyntax VariableDeclaratorNode
+        private VariableDeclaratorSyntax VariableDeclaratorNode


private VariableDeclaratorSyntax VariableDeclaratorNode [](start = 8, length = 55)

Could you please explain the rational behind making this field private? #Closed

Generally, as a matter of software engineering hygiene, it is better to keep API surface area between components as small as possible. This is an example of the application of that principle.

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

Generally, as a matter of software engineering hygiene, it is better to keep API surface area between components as small as possible.

I agree with that in general. However, SourceFixedFieldSymbol derives from this type and now it has an implicit contract with the base instead of an explicit one. I believe that an explicit contract is better than an implicit one. It makes dependency clear, code more readable and easier to understand. Consider changing accessibility of this method to protected and reverting the change in SourceFixedFieldSymbol.cs

In reply to: 80816186 [](ancestors = 80816186,80815616)

A private member is an explicit contract with code within its containing type.

In reply to: 80850539 [](ancestors = 80850539,80816186,80815616)

I am talking about the contract between this class and SourceFixedFieldSymbol, which you made implicit by replacing the use of this property with a type cast.

In reply to: 80959907 [](ancestors = 80959907,80850539,80816186,80815616)

Thank you for making the change.

In reply to: 80965746 [](ancestors = 80965746,80959907,80850539,80816186,80815616)

…ion.

gafter · 2016-09-27T23:46:38Z

@dotnet-bot test perf_correctness_prtest please #Closed

AlekseyTs · 2016-09-27T23:49:16Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

+                    // both match exactly, non-lambda case.
+                    // Neither is a better conversion from expression
+                    return BetterResult.Neither;
+                }


I am not comfortable with this change. It also doesn't match the latest specification quoted by Mads at #14117 #Closed

AlekseyTs · 2016-09-27T23:59:36Z

The change in OverloadResolution.cs is way more complicated than the original implementation with no clear benefits. The discussion about the exact spec wording is in flight, there is no good reason to rush making this change right now, you already iterated through at least three different implementations. Overload resolution is a delicate area, let's wait for the dust to settle, i.e. for the LDM to come up with the final wording. Please exclude changes to OverloadResolution.cs and outvar.md from this PR, they are not related to the goal that this PR is trying to achieve. #Closed

gafter · 2016-09-28T00:21:26Z

The change in OverloadResolution.cs now implements what the spec actually says, rather than inlining it into the caller.

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

…-10603

gafter · 2016-09-28T00:46:52Z

I'm not comfortable having the implementation of "better conversion from expression" incorrect and partially inlined into a caller. So lets review the fix for that and move on. #Closed

AlekseyTs · 2016-09-28T05:58:25Z

Given that we are in the middle of an active design discussion about the exact wording that should be used by the language specification to describe interaction of Overload Resolution with Out Variable Declarations and the fact that you haven't demonstrated any correctness issues with the current implementation, I object to making any premature changes both to the outvar.md in that area and to OverloadResolution.cs. Please summarize your concerns in a new, or existing issue (if there is one already) and assign it to me, I will make the necessary changes once design is finalized. Also, please exclude relevant changes from this PR.

AlekseyTs

Please revert premature changes to OverloadResolution.cs and changes related to overload resolution in outvar.md
Please ensure LocalRewriter explicitly lowers bound nodes from the input bound tree.
Please open an issue to close a test gap for Declaration Pattern in Script. All remaining statements (for, foreach, try, etc.) should be covered.

gafter · 2016-09-28T14:16:24Z

Please revert premature changes to OverloadResolution.cs and changes related to overload resolution in outvar.md

Those changes either describe what is implemented, or change the description to align with spec terminology. So unless your preexisting implementation is also premature, those changes are intended to remain in this PR.

You're welcome to make changes to that spec in the future if/when you change the corresponding implementation, and vice versa. As it stands, the spec and implementation are out of alignment, and these changes are necessary to correct that.

Please ensure LocalRewriter explicitly lowers bound nodes from the input bound tree.

The only nodes you asked to be lowered were those in the decision tree. The decision tree is constructed in locally lowered form, but if you need the double-lowering to pass the review, I'll do that.

Please open an issue to close a test gap for Declaration Pattern in Script. All remaining statements (for, foreach, try, etc.) should be covered.

I'll check to see what test gap remains.
#Resolved

agocke · 2016-09-28T15:58:28Z

LGTM, although I know very little about the semantics in scripting.

…-10603

…ignment in switch.

AlekseyTs · 2016-09-28T17:11:32Z

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

@@ -55,8 +55,7 @@ public sealed override int FixedSize
                    DiagnosticBag diagnostics = DiagnosticBag.GetInstance();
                    int size = 0;

-                    VariableDeclaratorSyntax declarator = this.VariableDeclaratorNode;
-
+                    VariableDeclaratorSyntax declarator = VariableDeclaratorNode;


VariableDeclaratorSyntax declarator = VariableDeclaratorNode; [](start = 20, length = 61)

It looks like there is no meaningful change in this file. Consider reverting it to the previous state to reduce unnecessary churn in the code base.

jaredpar

We need to wait for discussions to settle on the spec behavior before merging this change.

gafter · 2016-09-28T19:21:37Z

@jaredpar This is implementing the spec as written before the .md edit here. The spec before this change says that

For the purposes of overload resolution (see sections 7.5.3.2 Better function member and 7.5.3.3 Better conversion from expression),
Overload resolution is modified as follows:
neither conversion is considered better when corresponding argument is an implicitly-typed out variable declaration.

This change modifies BetterConversionFromExpression to implement this. There was a misalignment between the specified "better conversion from expression" and the method BetterConversionFromExpression before this change that hasn't been corrected. The other changes in the spec describe things already implemented (like the already-implemented conversion to which this refers).

I'm fine with the spec and implementation changing in lockstep if there is a proposed revision. Until then I prefer we keep them aligned.

gafter · 2016-09-28T21:08:25Z

@jaredpar I checked with @MadsTorgersen and the LDM does not intend to produce any further specification text for quite some time. He is OK with a spec that aligns with the implementation, the shape of the current language specification, and the intent of the LDM as previously expressed. We are all OK with any further work on the spec and implementation, as long as they are kept in alignment.

gafter added 2 commits September 22, 2016 16:02

Move computation of better conversion from expression into `BetterCon…

12c9bc8

…versionFromExpression` (issue ignored during code review)

Implement pattern-matching in top-level scripts

b5b90cb

in which pattern variables become fields. Fixes dotnet#10603

gafter added Area-Compilers Feature Request 4 - In Review A fix for the issue is submitted for review. labels Sep 22, 2016

gafter added this to the 2.0 (RC) milestone Sep 22, 2016

gafter self-assigned this Sep 22, 2016

dnfclas added the cla-already-signed label Sep 22, 2016

Merge master into master-10603

59aca3e

jcouv reviewed Sep 27, 2016

View reviewed changes