Expression variables declared in a local function parameter default are given a scope #16315

gafter · 2017-01-07T00:09:03Z

Customer scenario

If you declare an expression variable (e.g. out variable) inside the default value
expression of a parameter to a local function, the IDE crashes.

Bugs this fixes:

Fixes #16167

Workarounds, if any

Don't make that mistake.

Risk

Small. Corrects a small oversight in the implementation of the interaction of new language features.

Performance impact

Trivial.

Is this a regression from a previous update?

No.

Root cause analysis:

Missing test for interaction between multiple new language features. Lack of dedicated testing team. Lack of coordinated testing role for new features and interactions between new features.

How was the bug found?

Customer reported.

@AlekseyTs @agocke @dotnet/roslyn-compiler May I please have a couple of reviews for this very small bug fix?

…re given a scope Fixes dotnet#16167

jcouv · 2017-01-07T00:50:37Z

src/Compilers/CSharp/Test/Semantic/Semantics/FuzzTests.cs

+        }
+
+        [Fact, WorkItem(16167, "https://github.com/dotnet/roslyn/issues/16167")]
+        public void Bug16167()


I'd consider moving this test to another test file (with out vars or local functions maybe). It is not "random trash" ;-)

Yes, we should stress semantic model and OutVarTests has good helpers for that. Please use the helpers.

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

Unfortunately, those helpers are specialized so that they do not work for pattern variables or deconstruction variables.

Because we do not preserve bound nodes for these contexts, the "usual" verifications we do in OutVarTests, PatternTests, etc will not pass. Fixing that will be a relatively much larger undertaking with marginal customer value. I'd prefer to keep this test as it is an file a bug for the (less important) issue of the general semantic model APIs not working well in this context.

Filed #16374 for these.

jcouv

LGTM

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

src/Compilers/CSharp/Test/Semantic/Semantics/FuzzTests.cs

+";
+            // the scope of an expression variable introduced in the default expression
+            // of a local function parameter is that default expression.
+            var compilation = CreateCompilationWithMscorlib45(text);


compilation [](start = 16, length = 11)

Please assert diagnostics for the compilation.

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

src/Compilers/CSharp/Test/Semantic/Semantics/FuzzTests.cs

+{
+    public static void Main(int arg)
+    {
+        void Local1(bool b = M(arg is int z1, z1), int s1 = z1) {}


{} [](start = 64, length = 2)

Please add references to the declared locals into the body of the owning local function. Here and below. We should confirm that they are not in scope, etc.

AlekseyTs · 2017-01-07T01:00:28Z

src/Compilers/CSharp/Test/Semantic/Semantics/FuzzTests.cs

+        void Local4(bool b = M(arg is var z4, z4), int s1 = z4) {}
+        void Local5(bool b = M(M(out var z5), z5)), int s2 = z5) {}
+        void Local6(bool b = M(M((var z6, int a2) = (1, 2)), z6), int a3 = z6) {}
+    }


} [](start = 4, length = 1)

Please add reference to all declared locals at the end of the method body. We should confirm that they are not in scope, etc.

AlekseyTs · 2017-01-07T01:03:51Z

src/Compilers/CSharp/Test/Semantic/Semantics/FuzzTests.cs

+{
+    public static void Main(int arg)
+    {
+        void Local1(bool b = M(arg is int z1, z1), int s1 = z1) {}


z1 [](start = 60, length = 2)

We should test that locals and local functions declared within containing block are in scope inside default values. Semantic model should be tested for this as well: Lookup, GetSymbolInfo, etc.

AlekseyTs · 2017-01-07T01:08:47Z

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

@@ -224,6 +224,11 @@ public override void VisitLocalFunctionStatement(LocalFunctionStatementSyntax no
                var oldMethod = _containingMemberOrLambda;
                _containingMemberOrLambda = match;

+                foreach (var parameter in node.ParameterList.Parameters)
+                {
+                    Visit(parameter.Default);


Visit(parameter.Default); [](start = 20, length = 25)

It looks like BindParameterDefaultValue would always crash without this code. This indicates that we probably do not have any tests that verify that default parameter values are handled properly for local functions. Please add some of the success tests that verify that it is possible to take advantage of the default values. Should also check if values make it to metadata.

I'll add an issue for that to be done separately.

Filed #16352 for this.

…-16167

gafter · 2017-01-10T01:36:27Z

tagging @jaredpar by his request.

cston · 2017-01-10T15:49:58Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

@@ -2159,5 +2161,163 @@ public unsafe void M2()
                //             await Task.Delay(2);
                Diagnostic(ErrorCode.ERR_AwaitInUnsafeContext, "await Task.Delay(2)").WithLocation(33, 13));
        }
+
+        [Fact, WorkItem(16167, "https://github.com/dotnet/roslyn/issues/16167")]
+        public void Bug16167()


Consider using a descriptive name, perhaps ExpressionVariablesDeclaredInDefaultValue.

cston · 2017-01-10T15:53:25Z

LGTM

AlekseyTs · 2017-01-10T18:23:54Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

@@ -2159,5 +2161,163 @@ public unsafe void M2()
                //             await Task.Delay(2);
                Diagnostic(ErrorCode.ERR_AwaitInUnsafeContext, "await Task.Delay(2)").WithLocation(33, 13));
        }
+
+        [Fact, WorkItem(16167, "https://github.com/dotnet/roslyn/issues/16167")]
+        public void Bug16167()


It doesn't look like the PR adds any success tests. I think we should know if scenario like this is going to work correctly:

const int x =1; void Local(int p = x) { ... }

AlekseyTs · 2017-01-10T18:29:15Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+{
+    public static void Main(int arg)
+    {
+        void Local1(bool b = M(arg is int z1, z1), int s1 = z1) {}


{} [](start = 64, length = 2)

Please add references to the declared locals into the body of the owning local function. Here and below. We should confirm that they are not in scope, etc.

AlekseyTs · 2017-01-10T18:31:26Z

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs

+{
+    public static void Main(int arg)
+    {
+        void Local1(bool b = M(arg is int z1, z1), int s1 = z1) {}


M(arg is int z1, z1) [](start = 29, length = 20)

We should test that locals and local functions declared within containing block are in scope inside default values. Semantic model should be tested for this as well: Lookup, GetSymbolInfo, etc.

AlekseyTs

I think we need to have more tests especially around semantic model behavior within the default values.

AlekseyTs · 2017-01-10T18:45:51Z

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

@@ -224,6 +224,11 @@ public override void VisitLocalFunctionStatement(LocalFunctionStatementSyntax no
                var oldMethod = _containingMemberOrLambda;
                _containingMemberOrLambda = match;

+                foreach (var parameter in node.ParameterList.Parameters)
+                {
+                    Visit(parameter.Default);


Visit(parameter.Default); [](start = 20, length = 25)

Looking at SourceComplexParameterSymbol.MakeDefaultExpression:

var convertedExpression = binder.CreateBinderForParameterDefaultValue(this, defaultSyntax).BindParameterDefaultValue(defaultSyntax, parameterType, diagnostics, out valueBeforeConversion);

It feels like we need to make sure that whatever we do here is consistent (for example, BinderFlags.ParameterDefaultValue flag is set appropriately). Also, SourceComplexParameterSymbol.MakeDefaultExpression is going to create its own binder instead of using the one created by LocalBinderFactory. This looks suspicious.

It does look like we have a potential inconsistency here with respect to this flag.

I've added a comment to #16451 and #16352 noting this.

@gafter this isn't something we should delay. There is an incossitsency here we need to undercstand before we can merge a fix.

Default expressions are never bound directly with a binder from the factory or the cache. They are always wrapped by a binder in MakeDefaultExpression before the expression is bound. So it appears to be a consistency rather than an inconsistency.

You make it sound like the binders that are created in LocalBinderFactory.cs are never used to bind default parameters, at the same time creating the binders avoids a crash. So, to me it looks like, we are binding default parameters with binders that differ in flags, i.e. inconsistently.

AlekseyTs · 2017-01-10T18:48:31Z

Because we do not preserve bound nodes for these contexts, the "usual" verifications we do in OutVarTests, PatternTests, etc will not pass. Fixing that will be a relatively much larger undertaking with marginal customer value. I'd prefer to keep this test as it is an file a bug for the (less important) issue of the general semantic model APIs not working well in this context.

I would like to get a better understanding what exactly is not working and how. Let's sync-up offline.

gafter · 2017-01-12T19:07:24Z

Will create a new PR for rc3 branch

Expression variables declared in a local function parameter default a…

583c9cf

…re given a scope Fixes dotnet#16167

gafter added Area-Compilers Bug labels Jan 7, 2017

gafter added this to the 2.0 (RC.3) milestone Jan 7, 2017

gafter self-assigned this Jan 7, 2017

dnfclas added the cla-already-signed label Jan 7, 2017

jcouv reviewed Jan 7, 2017

View reviewed changes

jcouv approved these changes Jan 7, 2017

View reviewed changes

AlekseyTs reviewed Jan 7, 2017

View reviewed changes

gafter mentioned this pull request Jan 9, 2017

Add tests for default parameter values for local functions #16352

Closed

gafter added the New Language Feature - Local Functions Local Functions label Jan 9, 2017

gafter added 2 commits January 9, 2017 16:57

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

674182e

…-16167

Some test changes recommended by code review.

f0d55ff

Work toward addressing pattern-matching crashes.

5d668d3

cston reviewed Jan 10, 2017

View reviewed changes

gafter changed the base branch from master to dev15-rc3 January 10, 2017 17:54

gafter changed the base branch from dev15-rc3 to master January 10, 2017 17:54

AlekseyTs reviewed Jan 10, 2017

View reviewed changes

AlekseyTs requested changes Jan 10, 2017

View reviewed changes

AlekseyTs reviewed Jan 10, 2017

View reviewed changes

Test changes per code review.

e520c9c

gafter closed this Jan 12, 2017

gafter mentioned this pull request Jan 12, 2017

Expression variables declared in a local function parameter default are given a scope #16467

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expression variables declared in a local function parameter default are given a scope #16315

Expression variables declared in a local function parameter default are given a scope #16315

gafter commented Jan 7, 2017

jcouv Jan 7, 2017

AlekseyTs Jan 7, 2017

gafter Jan 9, 2017

gafter Jan 10, 2017

gafter Jan 10, 2017

jcouv left a comment

AlekseyTs Jan 7, 2017

AlekseyTs Jan 7, 2017 •

edited

Loading

AlekseyTs Jan 7, 2017

AlekseyTs Jan 7, 2017 •

edited

Loading

AlekseyTs Jan 7, 2017

gafter Jan 9, 2017

gafter Jan 9, 2017

gafter commented Jan 10, 2017

cston Jan 10, 2017

cston commented Jan 10, 2017

AlekseyTs Jan 10, 2017

AlekseyTs Jan 10, 2017

AlekseyTs Jan 10, 2017

AlekseyTs left a comment

AlekseyTs Jan 10, 2017

jaredpar Jan 12, 2017

gafter Jan 12, 2017

jaredpar Jan 12, 2017

gafter Jan 12, 2017

AlekseyTs Jan 12, 2017

AlekseyTs commented Jan 10, 2017

gafter commented Jan 12, 2017

Expression variables declared in a local function parameter default are given a scope #16315

Expression variables declared in a local function parameter default are given a scope #16315

Conversation

gafter commented Jan 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Jan 10, 2017

Choose a reason for hiding this comment

cston commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jan 10, 2017

gafter commented Jan 12, 2017

AlekseyTs Jan 7, 2017 •

edited

Loading

AlekseyTs Jan 7, 2017 •

edited

Loading