UseExplicitType for var in deconstruction #23975

jcouv · 2017-12-30T07:01:24Z

Customer scenario

UseExplicitType should trigger on var in a deconstruction. For instance: var (x, y) = new C(); should trigger. A proper fix should be offered (something like (int x, string y) = new C();).

Bugs this fixes

Fixes #23752

Workarounds, if any

You can fix the code manually when the code fix doesn't trigger.

Risk

There are two parts of this change, both which are low risk:

the GetTypeInfo returned by the semantic model on var (x, y) in a deconstruction foreach should have proper ConvertedType.
the UseExplicitType code fixer needs to generate proper replacement code.

Performance impact

Low. The analyzer only performs one additional and cheap check to see if "var" is a declaration expression with parenthesized designation.

Is this a regression from a previous update?

No

Root cause analysis

For the code fix, I didn't realize we should expect that code fix to trigger in this scenario.

How was the bug found?

Reported internally.

Question for IDE team: should short discards should also trigger this code fix (since the type isn't apparent)?

jcouv · 2017-12-30T07:05:28Z

src/EditorFeatures/CSharpTest/Diagnostics/UseImplicitOrExplicitType/UseExplicitTypeTests.cs

+/*x1*/int x/*x2*/,
+/*yz1*/(
+/*y1*/int y/*y2*/,
+/*z1*/Program z/*z2*/)/*yz2*/) /*after*/ = new Program();


I'm not sure what causes this weird formatting. I'll check with @DustinCampbell for any tips before debugging in depth.

Thanks @DustinCampbell for the pointer.
If I understood properly, the approach is to drop some trivia that would cause formatting problems. I'm not sure I like this approach for this PR.
I'd rather either: (1) leave the odd formatting for the edge case of comments in such positions in a deconstruction, or (2) fix the formatter (it should not introduce newlines in certain positions).

If the IDE team is ok, I'll leave the current behavior (odd formatting).

This looks pretty broken if you ask me...

@jasonmalinowski I spent the afternoon deep in trivia formatter and could not figure it out.
As far as I can tell the root cause of the newlines getting added is AbstractTriviaFormatter.GetLineColumnOfWhitespace which returns a value with lines=1. This causes AbstractTriviaFormatter.AddWhitespaceTextChange to add a newline.

I filed a follow-up issue: #24060

Never mind. I finally understood how Dustin's workaround works and applied a similar trick ;-)

The key is that parens and commas introduced by the codefix come with elastic whitespace trivia which causes the formatter to misbehave. Removing those avoids the problem.

@jasonmalinowski Any other review feedback?

jcouv · 2018-01-02T19:49:14Z

I need to revise this. From discussion with @gafter, he'd rather the semantic model not expose a type for the var in such a deconstruction.
After a recent reframing (due to patterns), var now means "declare something", rather than being a placeholder for a type.

Update: done. Most of the work is in the IDE layer now, with just one small fix in the compiler.

jcouv · 2018-01-03T23:27:37Z

@dotnet/roslyn-ide for review. Thanks

jcouv · 2018-01-05T22:14:32Z

@dotnet/roslyn-ide for review. Thanks

jcouv · 2018-01-08T17:33:33Z

@Pilchie for ask-mode approval. Thanks

Pilchie · 2018-01-08T17:50:10Z

@jcouv I don't see appropriate compiler reviews for the compiler portions.

jcouv · 2018-01-08T18:43:16Z

Good call. I forgot there were there.
@dotnet/roslyn-compiler for review. Thanks

cston · 2018-01-08T19:00:39Z

src/Features/CSharp/Portable/TypeStyle/UseExplicitTypeCodeFixProvider.cs

+            for (int i = 1; i < builder.Count; i++)
+            {
+                separatorBuilder.Add(SyntaxFactory.Token(SyntaxKind.CommaToken).WithoutTrivia());
+            }


Perhaps: var separatorBuilder = ArrayBuilder<SyntaxToken>.GetInstance(builder.Count - 1, SyntaxFactory.Token(...));

Done. Thanks!

jcouv · 2018-01-08T19:09:48Z

@dotnet/roslyn-compiler for a second review. Thanks

gafter · 2018-01-08T20:03:20Z

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

@@ -277,7 +277,7 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
                            hasErrors = true;
                        }

-                        boundIterationVariableType = new BoundTypeExpression(variables, aliasOpt: null, type: iterationVariableType);
+                        boundIterationVariableType = new BoundTypeExpression(variables, aliasOpt: null, type: iterationVariableType).MakeCompilerGenerated();


boundIterationVariableType [](start = 24, length = 26)

As we discussed, bound types are supposed to reflect types in source; you're filing a follow-up issue to separate the representation of the type in source from the type of the iterator.

Filed #24105 from our discussion. Thanks

gafter

Compiler code looks correct to me, modulo a new issue you filed. IDE code looks plausible, but I assume you're getting review from the IDE team.

jcouv · 2018-01-08T20:09:46Z

@Pilchie Thanks for reminding me on compiler review. That's now done. Please approve for ask-mode. Thanks

Pilchie · 2018-01-09T00:06:52Z

Approved - thanks @jcouv

jcouv · 2018-01-09T03:59:58Z

Retargetd to dev15.6.x branch.

jcouv · 2018-01-09T06:19:46Z

test ubuntu_14_debug_prtest please

AlekseyTs · 2018-01-09T22:56:39Z

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

@@ -1227,6 +1227,7 @@ public void Deconstruct(out int a, out int b)
            Assert.Equal("(System.Int32 a, System.Int32 b)", tuple2.ToTestDisplayString());
            var underlying2 = tuple2.TupleUnderlyingType;
            Assert.Equal("System.ValueTuple<System.Int32, System.Int32>[missing]", underlying2.ToTestDisplayString());
+            Assert.Equal("(System.Int32 a, System.Int32 b)", model.GetTypeInfo(ab).ConvertedType.ToTestDisplayString());


Assert.Equal("(System.Int32 a, System.Int32 b)", model.GetTypeInfo(ab).ConvertedType.ToTestDisplayString()); [](start = 12, length = 108)

Do we have similar test for a regular deconstruction declaration?

@AlekseyTs I can't find one. I'll start a PR to add such test. Thanks

jcouv added the Area-IDE label Dec 30, 2017

jcouv added this to the 15.6 milestone Dec 30, 2017

jcouv self-assigned this Dec 30, 2017

jcouv requested a review from a team as a code owner December 30, 2017 07:01

jcouv commented Dec 30, 2017

View reviewed changes

SemanticModel and UseExplicitType for var in deconstruction

37742fb

jcouv force-pushed the var-decon branch from c9bd18a to 37742fb Compare December 30, 2017 09:12

jcouv added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Dec 30, 2017

jcouv added PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jan 3, 2018

jcouv changed the title ~~SemanticModel and UseExplicitType for var in deconstruction~~ [WIP] SemanticModel and UseExplicitType for var in deconstruction Jan 3, 2018

Back off semantic model change on "var" in deconstruction

c310873

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 3, 2018

jcouv changed the title ~~[WIP] SemanticModel and UseExplicitType for var in deconstruction~~ UseExplicitType for var in deconstruction Jan 3, 2018

jcouv added 2 commits January 4, 2018 18:10

Capture an extra bit of trivia

9d118e6

Work around newlines introduced by formatting

96d7564

jcouv force-pushed the var-decon branch from 97a608f to 96d7564 Compare January 5, 2018 02:52

jasonmalinowski approved these changes Jan 5, 2018

View reviewed changes

cston reviewed Jan 8, 2018

View reviewed changes

cston approved these changes Jan 8, 2018

View reviewed changes

Simplify initialization of ArrayBuilder with identical tokens

5250abd

jcouv force-pushed the var-decon branch from 3060675 to 5250abd Compare January 8, 2018 19:23

gafter reviewed Jan 8, 2018

View reviewed changes

gafter approved these changes Jan 8, 2018

View reviewed changes

Pilchie added the Approved to merge label Jan 9, 2018

jcouv changed the base branch from master to dev15.6.x January 9, 2018 03:59

AlekseyTs reviewed Jan 9, 2018

View reviewed changes

jcouv merged commit 6dc3522 into dotnet:dev15.6.x Jan 10, 2018

jcouv deleted the var-decon branch January 10, 2018 19:18

jcouv mentioned this pull request Jan 10, 2018

Test ConvertedType on LHS of deconstruction-assignment #24158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UseExplicitType for var in deconstruction #23975

UseExplicitType for var in deconstruction #23975

jcouv commented Dec 30, 2017 •

edited

Loading

jcouv Dec 30, 2017

jcouv Jan 3, 2018

jasonmalinowski Jan 4, 2018

jcouv Jan 5, 2018

jcouv Jan 5, 2018

jcouv Jan 5, 2018

jcouv commented Jan 2, 2018 •

edited

Loading

jcouv commented Jan 3, 2018

jcouv commented Jan 5, 2018

jcouv commented Jan 8, 2018

Pilchie commented Jan 8, 2018

jcouv commented Jan 8, 2018

cston Jan 8, 2018

jcouv Jan 8, 2018

jcouv commented Jan 8, 2018

gafter Jan 8, 2018

jcouv Jan 8, 2018

gafter left a comment

jcouv commented Jan 8, 2018

Pilchie commented Jan 9, 2018

jcouv commented Jan 9, 2018

jcouv commented Jan 9, 2018

AlekseyTs Jan 9, 2018

jcouv Jan 10, 2018

UseExplicitType for var in deconstruction #23975

UseExplicitType for var in deconstruction #23975

Conversation

jcouv commented Dec 30, 2017 • edited Loading

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?

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

jcouv commented Jan 2, 2018 • edited Loading

jcouv commented Jan 3, 2018

jcouv commented Jan 5, 2018

jcouv commented Jan 8, 2018

Pilchie commented Jan 8, 2018

jcouv commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

jcouv commented Jan 8, 2018

Pilchie commented Jan 9, 2018

jcouv commented Jan 9, 2018

jcouv commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Dec 30, 2017 •

edited

Loading

jcouv commented Jan 2, 2018 •

edited

Loading