Enable out variables in indexers, and implement lazy inference of pattern variables. #13274

gafter · 2016-08-19T23:50:11Z

Also eliminates some cases of cascaded definite assignment errors.

Fixes #13219
Fixes #13009
Fixes #12996
Fixes #10446

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

…tern variables. Also eliminates some cases of cascaded definite assignment errors. Fixes dotnet#13219 Fixes dotnet#13009 Fixes dotnet#12996 Fixes dotnet#10446

…e used to initialize its storage.

cston · 2016-08-22T15:48:25Z

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

+ var x = d[out int z];
+ }
+}";
+ // the C# dynamic binder does not support ref or out indexers, so we don't run this


If the dynamic binder doesn't support ref or out indexers, why doesn't the compiler report an error? #Resolved

Because the C# (static) compiler doesn't know if a variable of type dynamic uses the C# dynamic binder or a custom binder. #Resolved

cston · 2016-08-22T20:03:55Z

LGTM

Plus a couple of other minor changes per code review.

AlekseyTs · 2016-08-22T20:52:52Z

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

+ /// <param name="declarationKind"></param>
+ /// <param name="context">The expression to be bound, which will result in the variable receiving a type</param>
+ /// <returns></returns>
+ public static SourceLocalSymbol MakeVariableDeclaredInExpression(


MakeVariableDeclaredInExpression [](start = 40, length = 32)

The doc comment says this function is specific for "out" locals, but the name doesn't reflect this. Is there a particular reason for this? #Closed

I will update the comment. It is for any variable whose type is inferred as a side-effect of binding an expression.

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

AlekseyTs · 2016-08-22T21:25:22Z

    Test2(x[out var x1], x1);

Please add a test that verifies that an attempt to infer type of this local without binding this statement first produces expected result. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/OutVarTests.cs:14321 in c3d5c38. [](commit_id = c3d5c38, deletion_comment = False)

AlekseyTs · 2016-08-22T21:34:17Z

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

@@ -14591,5 +14575,199 @@ static int Test1(out int x)

 Assert.Equal("System.Int32", model.GetTypeInfo(yRef).Type.ToTestDisplayString());
 }
+
+ [Fact, WorkItem(13219, "https://github.com/dotnet/roslyn/issues/13219")]
+ public void IndexingDynamic()


IndexingDynamic [](start = 20, length = 15)

Please add test for scenario that uses var as type.
Please add verification for SemanticModel behavior.
Please add specific test for type inference before the indexing is bound. #Closed

The thread is marked as resolved, but it doesn't look like there is a "specific test for type inference before the indexing is bound". What am I missing?

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

I've marked the place you were missing.

In reply to: 75921906 [](ancestors = 75921906,75763457)

gafter · 2016-08-26T22:35:28Z

I am going to add tests for fixed field initializers, and any code required to make that work. #Resolved

gafter · 2016-08-26T22:53:45Z

Actually, I just filed an issue for it. #13417

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

gafter · 2016-08-29T18:07:23Z

@dotnet/roslyn-compiler May I please have a second review? #Resolved

VSadov · 2016-08-29T21:51:08Z

agocke · 2016-08-30T17:20:51Z

LGTM

AlekseyTs · 2016-08-30T17:33:49Z

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

+
+ public override void VisitDeconstructionDeclarationStatement(DeconstructionDeclarationStatementSyntax node)
+ {
+ VisitNodeToBind(node.Assignment.Value);


VisitNodeToBind(node.Assignment.Value); [](start = 12, length = 39)

Is this code really used?

If not I'll remove it in a subsequent PR.

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

AlekseyTs · 2016-08-30T17:44:52Z

                        enclosingBinder = enclosingBinder.GetBinder(innerStatement) ?? enclosingBinder;

This change doesn't look right, we should not be overriding enclosingBinder. #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/LocalScopeBinder.cs:184 in e4b4af9. [](commit_id = e4b4af9, deletion_comment = False)

gafter · 2016-08-30T17:48:05Z

                        enclosingBinder = enclosingBinder.GetBinder(innerStatement) ?? enclosingBinder;

This isn't a change in this PR.

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

Refers to: src/Compilers/CSharp/Portable/Binder/LocalScopeBinder.cs:184 in e4b4af9. [](commit_id = e4b4af9, deletion_comment = False)

AlekseyTs · 2016-08-30T17:52:51Z

                        enclosingBinder = enclosingBinder.GetBinder(innerStatement) ?? enclosingBinder;

I guess tools are confused.

In reply to: 243521804 [](ancestors = 243521804,243520839)

Refers to: src/Compilers/CSharp/Portable/Binder/LocalScopeBinder.cs:184 in e4b4af9. [](commit_id = e4b4af9, deletion_comment = False)

AlekseyTs · 2016-08-30T18:13:11Z

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

 break;

 case SyntaxKind.ForStatement:
 var forStatement = (ForStatementSyntax)_deconstruction;
- var forBinder = this.ScopeBinder.GetBinder(forStatement);
- forBinder.BindDeconstructionDeclaration(forStatement, forStatement.Deconstruction.VariableComponent, forStatement.Deconstruction.Value, diagnostics);
+ Debug.Assert(this.ScopeBinder.GetBinder(forStatement) == _nodeBinder);


Debug.Assert(this.ScopeBinder.GetBinder(forStatement) == _nodeBinder); [](start = 24, length = 70)

Do we have a test hitting this code path? #Resolved

The same tests that hit this switch branch previously.

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

Are there any?

In reply to: 76852705 [](ancestors = 76852705,76849261)

Good question. Since you don't know, I'll open an issue about test coverage for this case.

Filed as #13458

In reply to: 76854336 [](ancestors = 76854336,76852705,76849261)

AlekseyTs · 2016-08-30T18:43:55Z

        var model = compilation.GetSemanticModel(tree);

It looks like the local is not used.

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

gafter · 2016-08-30T18:49:57Z

        var model = compilation.GetSemanticModel(tree);

I'll remove it in some future PR.

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

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

gafter added 3 commits August 19, 2016 14:40

Enable out variables in indexers, and implement lazy inference of pat…

38e9fdf

…tern variables. Also eliminates some cases of cascaded definite assignment errors. Fixes dotnet#13219 Fixes dotnet#13009 Fixes dotnet#12996 Fixes dotnet#10446

Make it clear that a cast is valid by tightening the type of the valu…

ed3a62c

…e used to initialize its storage.

Merge master into gafter:master-13219

c3d5c38

gafter added Bug Area-Compilers New Language Feature - Pattern Matching Pattern Matching New Language Feature - Out Variable Declaration Out Variable Declaration labels Aug 19, 2016

gafter added this to the 2.0 (Preview 5) milestone Aug 19, 2016

gafter assigned agocke, gafter and AlekseyTs Aug 19, 2016

dnfclas added the cla-already-signed label Aug 19, 2016

cston reviewed Aug 22, 2016
View reviewed changes

Use a Kind test instead of a type test.

6c56e52

Plus a couple of other minor changes per code review.

AlekseyTs reviewed Aug 22, 2016
View reviewed changes

gafter added 3 commits August 26, 2016 10:46

Bind the entire enclosing expression for expression variable inference.

e4b4af9

Merge dotnet:master into gafter:master-13219

594a961

Changes to local variable inference requested in code review.

1f0e5f8

Add a test for a know broken scenario in pattern variable scope.

e170abf

Merge dotnet:master into gafter:master-13219

a938f65

gafter unassigned agocke and AlekseyTs Aug 30, 2016

AlekseyTs reviewed Aug 30, 2016
View reviewed changes

This was referenced Aug 30, 2016

Test coverage for SourceLocalSymbol binding a for statement #13458

Closed

Tests needed for out variables in declarators in for loop. #13459

Closed

Issue binding (erroneous) variables in a declarator argument list. #13460

Closed

gafter merged commit c484138 into dotnet:master Aug 30, 2016

markwilkie removed the Bug label Aug 30, 2016

gafter added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Aug 30, 2016

gafter deleted the master-13219 branch May 24, 2018 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable out variables in indexers, and implement lazy inference of pattern variables. #13274

Enable out variables in indexers, and implement lazy inference of pattern variables. #13274

gafter commented Aug 19, 2016

cston Aug 22, 2016 •

edited by gafter

Loading

gafter Aug 22, 2016 •

edited

Loading

cston commented Aug 22, 2016

AlekseyTs Aug 22, 2016 •

edited

Loading

gafter Aug 22, 2016

AlekseyTs commented Aug 22, 2016 •

edited by OmarTawfik

Loading

AlekseyTs Aug 22, 2016 •

edited

Loading

AlekseyTs Aug 23, 2016

gafter Aug 23, 2016

gafter commented Aug 26, 2016 •

edited

Loading

gafter commented Aug 26, 2016

gafter commented Aug 29, 2016 •

edited

Loading

VSadov commented Aug 29, 2016

agocke commented Aug 30, 2016

AlekseyTs Aug 30, 2016

gafter Aug 30, 2016

AlekseyTs commented Aug 30, 2016 •

edited

Loading

gafter commented Aug 30, 2016

AlekseyTs commented Aug 30, 2016

AlekseyTs Aug 30, 2016 •

edited by gafter

Loading

gafter Aug 30, 2016

AlekseyTs Aug 30, 2016

gafter Aug 30, 2016 •

edited

Loading

AlekseyTs commented Aug 30, 2016

gafter commented Aug 30, 2016

Enable out variables in indexers, and implement lazy inference of pattern variables. #13274

Enable out variables in indexers, and implement lazy inference of pattern variables. #13274

Conversation

gafter commented Aug 19, 2016

cston Aug 22, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

cston commented Aug 22, 2016

AlekseyTs Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

gafter Aug 22, 2016

Choose a reason for hiding this comment

AlekseyTs commented Aug 22, 2016 • edited by OmarTawfik Loading

AlekseyTs Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Aug 23, 2016

Choose a reason for hiding this comment

gafter Aug 23, 2016

Choose a reason for hiding this comment

gafter commented Aug 26, 2016 • edited Loading

gafter commented Aug 26, 2016

gafter commented Aug 29, 2016 • edited Loading

VSadov commented Aug 29, 2016

agocke commented Aug 30, 2016

AlekseyTs Aug 30, 2016

Choose a reason for hiding this comment

gafter Aug 30, 2016

Choose a reason for hiding this comment

AlekseyTs commented Aug 30, 2016 • edited Loading

gafter commented Aug 30, 2016

AlekseyTs commented Aug 30, 2016

AlekseyTs Aug 30, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Aug 30, 2016

Choose a reason for hiding this comment

AlekseyTs Aug 30, 2016

Choose a reason for hiding this comment

gafter Aug 30, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Aug 30, 2016

gafter commented Aug 30, 2016

cston Aug 22, 2016 •

edited by gafter

Loading

gafter Aug 22, 2016 •

edited

Loading

AlekseyTs Aug 22, 2016 •

edited

Loading

AlekseyTs commented Aug 22, 2016 •

edited by OmarTawfik

Loading

AlekseyTs Aug 22, 2016 •

edited

Loading

gafter commented Aug 26, 2016 •

edited

Loading

gafter commented Aug 29, 2016 •

edited

Loading

AlekseyTs commented Aug 30, 2016 •

edited

Loading

AlekseyTs Aug 30, 2016 •

edited by gafter

Loading

gafter Aug 30, 2016 •

edited

Loading