Change local function definite assignment #23749

agocke · 2017-12-12T22:36:25Z

Customer scenario

The LDM has decided that the following rules should be in effect when
calculating definite assignment for local functions:

The entry point to a local function is always reachable.
Variables captured in local functions are definitely assigned if
they are definitely assigned in all branches into the local function.

It turns out these rules were almost exactly what the compiler already
implemented, but there was a bug in captured variable detection that
meant that variables captured in lambdas within local functions were
sometimes not counted as captured. This change fixes the bug around
capturing, which should cause the compiler to conform to this
specification.

Bugs this fixes

Fixes #17829

Workarounds, if any

Language change. If new errors are reported due to the language change, the containing local function can simply be deleted since it must be unreachable for this code to run. In addition, a warning about an unused local function should already be produced, so diagnostics are already produced for this code.

Risk

This is a simple change to capturing, closer to a bug fix than a design change.

Performance impact

Low. This is a change from a constant-time check to a linear check of containing symbols, but
the level of nesting of local functions should be small enough that performance should not
matter.

Is this a regression from a previous update?

No, this is a design change/bug fix to something that has been present since VS2017 shipped.

Root cause analysis

This is a very subtle case that can only happen with a nested lambda inside a local function
where that local function is unreachable.

How was the bug found?

Customer reported.

The LDM has decided that the following rules should be in effect when calculating definite assignment for local functions: 1. The entry point to a local function is always reachable. 2. Variables captured in local functions are definitely assigned if they are definitely assigned in all branches into the local function. It turns out these rules were almost exactly what the compiler already implemented, but there was a bug in captured variable detection that meant that variables captured in lambdas within local functions were sometimes not counted as captured. This change fixes the bug around capturing, which should cause the compiler to conform to this specification. Fixes dotnet#17829

agocke · 2017-12-12T22:36:38Z

cc @jaredpar

jcouv · 2018-01-02T17:27:06Z

Please document the breaking change (new diagnostics can be produced).

gafter · 2018-01-02T19:49:57Z

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

+            // Walk up the containing symbols until we find the target function, in which
+            // case the variable is not captured by the target function, or null, in which 
+            // case it is.
+            var currentFunction = variable.ContainingSymbol;


This could be

for (var currentFunction = variable.ContainingSymbol; currentFunction != null; currentFunction = currentFunction.ContainingSymbol) { if (currentFunction == containingMethodOrLambda) { return false; } }

gafter · 2018-01-02T20:01:01Z

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

+            // on a particular branch.
+            // Assignments to captured variables are also recorded, as a local function
+            // definitely assigns captured variables on a call to a local function
+            // if that variable is definitely assigned at all branches out of the


branches out of [](start = 62, length = 15)

"branches out of" should be "returns from". #Resolved

Yield return, await, and the implicit return before the first statement in an iterator all count -- would you classify those as return statements?

gafter · 2018-01-02T20:01:23Z

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

-            // Local functions don't affect outer state and are analyzed
-            // with everything unassigned and reachable
+            // SPEC: The entry point to a local function is always reachable.
+            // Captured variables are assigned if they are assigned on all


assigned [](start = 38, length = 8)

"assigned" should be "definitely assigned" (twice). #Resolved

gafter

agocke · 2018-01-03T18:28:51Z

@jcouv Where do we document breaking changes now?

agocke · 2018-01-04T21:35:27Z

ping @dotnet/roslyn-compiler for a second review

@jaredpar @MeiChin-Tsai for ask-mode

jcouv · 2018-01-04T23:18:03Z

Breaking changes go here: https://github.com/dotnet/roslyn/blob/master/docs/compilers/CSharp/Compiler%20Breaking%20Changes%20-%20post%20VS2017.md

MeiChin-Tsai · 2018-01-05T00:19:18Z

Approved. Is there a way to assess how breaking this is? thx.

jcouv · 2018-01-05T00:21:01Z

docs/compilers/CSharp/Compiler Breaking Changes - VS2017.md

@@ -4,3 +4,19 @@
 Each entry should include a short description of the break, followed by either a link to the issue describing the full details of the break or the full details of the break inline.*

 1. Previously, we would not find a best type among multiple types (for ternary expressions and method type inference) when the types differed only in dynamic-ness and would involve some nesting. For example, we could not find a best type between `KeyValuePair<dynamic, object>` and `KeyValuePair<object, dynamic>`. Starting with VS 2017, we can find a best type (we merge dynamic-ness). In this example, `KeyValuePair<dynamic, dynamic>` is the best type. This can affect overload resolution. For instance, allowing a better overload to be picked, which was previously discarded.   See issues [#12585](https://github.com/dotnet/roslyn/issues/12585), [#14247](https://github.com/dotnet/roslyn/issues/14247), [#14213](https://github.com/dotnet/roslyn/issues/14213), and their corresponding PRs for more details.
+
+2. VS 2017 15.0-15.5 shipped with a bug around definite assignment of local functions that did not produce definite assignment errors when an uncalled local function contains a nested lambda which captures a variable. For example:


VS 2017 15.0-15.5 [](start = 2, length = 18)

Branding guidelines: Visual Studio 2017 version 15.0-15.5

jcouv · 2018-01-05T00:21:56Z

docs/compilers/CSharp/Compiler Breaking Changes - VS2017.md

+        }
+    }
+    ```
+    This is changed to now produce an error that the variable is not definitely assigned.


This is the wrong file. Look at the one called "post 2017"

jcouv · 2018-01-05T00:28:57Z

docs/compilers/CSharp/Compiler Breaking Changes - post VS2017.md

+        }
+    }
+    ```
+    This is changed to now produce an error that the variable is not definitely assigned.


In ... 15.6, this changed to produce...

I think we should make the specific version change later. There's no guarantee this actually ships in 15.6.

This PR is for master branch, which is 15.6, and you are about to merge...

agocke · 2018-01-05T00:31:14Z

@MeiChin-Tsai This change is only breaking if:

The user has a local function which
Is not called and
Contains a lambda expression which
Contains a variable which is not definitely assigned and
That variable is read in the lambda.

We think this is a very rare case and if it does occur, the fix is simple (delete the local function, since it is never called).

MeiChin-Tsai · 2018-01-05T00:32:24Z

@agocke got it. Thank you!

jcouv

LGTM

AlekseyTs · 2018-01-12T18:27:16Z

docs/compilers/CSharp/Compiler Breaking Changes - post VS2017.md

@@ -79,3 +79,19 @@ Example: `Func<int> f = default(TypedReference).GetHashCode; // new error CS0123
 - https://github.com/dotnet/roslyn/pull/23416 Before Visual Studio 2017 version 15.6 (Roslyn version 2.8) the compiler accepted `__arglist(...)` expressions with void-typed arguments. For instance, `__arglist(Console.WriteLine())`. But such program would fail at runtime. In Visual Studio 2017 version 15.6, this causes a compile-time error.

 - https://github.com/dotnet/roslyn/pull/24023 In Visual Studio 2017 version 15.6, Microsoft.CodeAnalysis.CSharp.Syntax.CrefParameterSyntax constructor and Update(), the parameter refOrOutKeyword was renamed to refKindKeyword (source breaking change if you're using named arguments).
+
+- Visual Studio 2017 15.0-15.5 shipped with a bug around definite assignment of local functions that did not produce definite assignment errors when an uncalled local function contains a nested lambda which captures a variable. For example:


which captures a variable [](start = 201, length = 25)

Is the wording here accurate? It doesn't look like the lambda in example captures a variable, it declares one and uses its value before the variable is assigned.

AlekseyTs · 2018-01-12T18:34:16Z

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

+            // case it is.
+            for (var currentFunction = variable.ContainingSymbol;
+                 currentFunction != null;
+                 currentFunction = currentFunction.ContainingSymbol)


currentFunction = currentFunction.ContainingSymbol [](start = 17, length = 50)

Is there a reason to keep climbing through types and namespaces?

agocke added the Area-Compilers label Dec 12, 2017

agocke added this to the 15.6 milestone Dec 12, 2017

agocke requested a review from a team December 12, 2017 22:36

Change baseline for IDE test related to this bug

e3cca99

agocke requested a review from a team as a code owner December 13, 2017 00:11

jcouv assigned agocke Dec 15, 2017

gafter reviewed Jan 2, 2018

View reviewed changes

gafter approved these changes Jan 2, 2018

View reviewed changes

Respond to PR comments

ab07b36

Add documentation to the breaking change document

6d3694c

MeiChin-Tsai added the Approved to merge label Jan 5, 2018

jcouv reviewed Jan 5, 2018

View reviewed changes

Move to proper docs file

d3669d7

jcouv reviewed Jan 5, 2018

View reviewed changes

jcouv approved these changes Jan 5, 2018

View reviewed changes

Add VS version for breaking change.

162e52e

agocke changed the base branch from master to dev15.6.x January 10, 2018 20:53

Merge branch 'dev15.6.x' into localfunc-definite-assignment

7e9381e

agocke merged commit 1484806 into dotnet:dev15.6.x Jan 11, 2018

agocke deleted the localfunc-definite-assignment branch January 11, 2018 02:16

AlekseyTs reviewed Jan 12, 2018

View reviewed changes

jcouv mentioned this pull request Jan 16, 2018

Definite assignment errors are sometimes reported in unreachable code in local functions #17829

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change local function definite assignment #23749

Change local function definite assignment #23749

agocke commented Dec 12, 2017

agocke commented Dec 12, 2017

jcouv commented Jan 2, 2018

gafter Jan 2, 2018 •

edited

Loading

gafter Jan 2, 2018 •

edited by jcouv

Loading

agocke Jan 3, 2018

gafter Jan 2, 2018 •

edited by jcouv

Loading

gafter left a comment

agocke commented Jan 3, 2018

agocke commented Jan 4, 2018

jcouv commented Jan 4, 2018

MeiChin-Tsai commented Jan 5, 2018

jcouv Jan 5, 2018

jcouv Jan 5, 2018

jcouv Jan 5, 2018

agocke Jan 5, 2018

jcouv Jan 5, 2018

agocke commented Jan 5, 2018

MeiChin-Tsai commented Jan 5, 2018

jcouv left a comment

AlekseyTs Jan 12, 2018

AlekseyTs Jan 12, 2018

Change local function definite assignment #23749

Change local function definite assignment #23749

Conversation

agocke commented Dec 12, 2017

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?

agocke commented Dec 12, 2017

jcouv commented Jan 2, 2018

gafter Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Jan 2, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jan 2, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

agocke commented Jan 3, 2018

agocke commented Jan 4, 2018

jcouv commented Jan 4, 2018

MeiChin-Tsai commented Jan 5, 2018

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

agocke commented Jan 5, 2018

MeiChin-Tsai commented Jan 5, 2018

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jan 2, 2018 •

edited

Loading

gafter Jan 2, 2018 •

edited by jcouv

Loading

gafter Jan 2, 2018 •

edited by jcouv

Loading