Fixes #18311 - Incorrect quick info for ValueTuple<T> #18489

d0pare · 2017-04-06T03:23:02Z

Hovering var will show System.ValueTuple<System.Int32>
Hovering y will show (local variable) ValueTuple<int> y

CyrusNajmabadi · 2017-04-06T04:13:48Z

Need tests :)

They can be added to: https://github.com/dotnet/roslyn/blob/master/src/EditorFeatures/CSharpTest/QuickInfo/SemanticQuickInfoSourceTests.cs

Also, tagging @dotnet/roslyn-compiler as this changes compiler code.

d0pare · 2017-04-06T04:15:33Z

Fixing tests

CyrusNajmabadi · 2017-04-06T04:16:58Z

Also, does the same issue for VB? If so, we should fix that as well.

d0pare · 2017-04-06T04:33:53Z

VB is ok. Working on new tests.

jcouv · 2017-04-06T04:49:07Z

If there is any chance this same method is invoked again on the "rest" of the tuple (which nests the elements after the 7th), I'd recommend testing the case of an 8-tuple: (1, 2, ..., 8) or ValueTuple.Create(1, 2, ..., 7, ValueTuple.Create(8)).

d0pare · 2017-04-06T04:59:22Z

There are 2 tests for long tuples here https://github.com/dopare/roslyn/blob/f7c0a5a260c0011e6817db9d0d4d8204d951e1b2/src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/TupleTests.cs#L285-L353

jcouv · 2017-04-06T05:01:49Z

Nice. LGTM and thanks!

CyrusNajmabadi · 2017-04-06T05:04:33Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

@@ -381,6 +381,11 @@ private bool CanUseTupleTypeName(INamedTypeSymbol tupleSymbol)
        {
            INamedTypeSymbol currentUnderlying = tupleSymbol.TupleUnderlyingType;

+            if (currentUnderlying.Arity == 1)


Does this need to be <= 1? Is there a similar issue with zero arity value tuples?

For < 1 case CanUseTupleTypeName method is not called. For zero arity everything works fine.

CyrusNajmabadi · 2017-04-06T05:05:28Z

Still need quick info tests. Thanks! :)

d0pare · 2017-04-06T05:06:17Z

Need help. Cannot make unit test to work properly.
var y$$ = ValueTuple.Create(1); generates (local variable) ? y as actual description

CyrusNajmabadi · 2017-04-06T05:07:25Z

Do you have the right usings in place in the test?

CyrusNajmabadi · 2017-04-06T05:08:06Z

Note: look at TestResources.NetFX.ValueTuple.tuplelib_cs as well. It appears as if some tests manually concat that in to get the definition of ValueTuple, since it's not part of mscorlib.

d0pare · 2017-04-06T05:08:38Z

I am using

await TestInMethodAsync(
@"
var y$$ = ValueTuple.Create(1);
",
                MainDescription("(local variable) ValueTuple<int> y"));

TestInMethodAsync imports System

jcouv · 2017-04-06T05:16:18Z

You'll need to manually include the source for ValueTuple into the test. The mscorlib used in tests doesn't have ValueTuple yet.
Cyrus pointed out the resource string that will help.
Or you can just inline a small implementation (example).

d0pare · 2017-04-06T05:27:57Z

Thanks could fix the issue. Working on new tests.

CyrusNajmabadi · 2017-04-06T06:25:45Z

Your "Adding new tests" commit doesn't seem to be adding any new tests :)

d0pare · 2017-04-06T06:31:47Z

Sorry, committed wrong file. Fixed.

CyrusNajmabadi

IDE side lgtm. Still needs compiler review.

jcouv

LGTM

jcouv · 2017-04-06T07:00:47Z

@dotnet/roslyn-compiler for a second review of the compiler change.

d0pare · 2017-04-06T13:20:08Z

Squashed commits for next review

cston · 2017-04-06T16:17:03Z

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/TupleTests.cs

  IL_0006:  ldloc.0
-  IL_0007:  ldfld      ""(int) System.ValueTuple<int, int, int, int, int, int, int, (int)>.Rest""
+  IL_0007:  ldfld      ""ValueTuple<int> System.ValueTuple<int, int, int, int, int, int, int, ValueTuple<int>>.Rest""


The corresponding VB test should have required the same change.

➡️ As an update, this is now corrected.

cston · 2017-04-06T16:20:38Z

Consider adding a simple SymbolDisplay test for both C# and for VB. See SymbolDisplayTests.Tuple.

d0pare · 2017-04-06T18:58:28Z

Updated VB logic and tests. Working on new symbol display tests.

sharwell · 2017-04-06T21:02:51Z

Hovering var will show System.ValueTuple<System.Int32>

I believe the conclusion in #18298 was we want to display System.ValueTuple<int> instead of System.ValueTuple<System.Int32> for this case. However, it's possible that the work to implement this behavior will be part of the other pull request.

sharwell · 2017-04-06T21:30:12Z

@dopare It's possible that this pull request fixed the following issue which I found during an investigation of #15508. If that is the case, then this pull request likely fixes the last remaining issue with that issue.

void Foo()
{
    ValueTuple<int> y = ValueTuple.Create(1);
    y.Item1.ToString(); // Extract method for this line crashes the refactoring provider
}

d0pare · 2017-04-07T02:32:17Z

Added simple symbol display tests for C# and VB.
@sharwell Will check it.

d0pare · 2017-04-07T03:38:25Z

@sharwell I have manually tested this in latest Visual Studio 2017 with and without my changes.

Without my changes ExtractMethodCodeRefactoringProvider keeps crashing.

With my changes I could extract following method:

void Foo()
{
      ValueTuple<int> y = ValueTuple.Create(1);
      NewMethod(y);
}

private static void NewMethod(ValueTuple<int> y)
{
      y.Item1.ToString(); // Extract method for this line crashes the refactoring provider
}

cston · 2017-04-07T05:50:47Z

LGTM thanks.

d0pare · 2017-04-07T12:32:55Z

Squashed all commits

sharwell · 2017-04-07T12:57:58Z

@dopare The Extract Method tests are in ExtractMethodTests.cs and ExtractMethodTests.vb. I'm planning to add tests for the case I mentioned after this PR is merged, but if you want to tackle them now that you already fixed the issue feel free to. 😄

d0pare · 2017-04-07T13:53:53Z

@sharwell Will add those tests.

d0pare · 2017-04-07T18:25:18Z

@sharwell Added new extract method tests for C# and VB.

sharwell · 2017-04-07T19:51:44Z

Look at @dopare, doing my work for me. 😎

d0pare · 2017-04-07T19:56:24Z

Wow thanks 👍

dnfclas added the cla-already-signed label Apr 6, 2017

CyrusNajmabadi reviewed Apr 6, 2017

View reviewed changes

CyrusNajmabadi approved these changes Apr 6, 2017

View reviewed changes

jcouv approved these changes Apr 6, 2017

View reviewed changes

cston reviewed Apr 6, 2017

View reviewed changes

sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 6, 2017

Mike Mazmanyan added 2 commits April 7, 2017 14:24

Fixes #18311 - Incorrect quick info for ValueTuple<T>

8191c9c

Adding new extract method test for C# and VB

37ae596

sharwell merged commit b6f7c5c into dotnet:master Apr 7, 2017

d0pare deleted the bugfix/18311 branch April 7, 2017 19:56

Fixes #18311 - Incorrect quick info for ValueTuple<T> #18489

Fixes #18311 - Incorrect quick info for ValueTuple<T> #18489

Conversation

d0pare commented Apr 6, 2017 • edited by sharwell Loading

CyrusNajmabadi commented Apr 6, 2017

d0pare commented Apr 6, 2017

CyrusNajmabadi commented Apr 6, 2017

d0pare commented Apr 6, 2017 • edited Loading

jcouv commented Apr 6, 2017

d0pare commented Apr 6, 2017

jcouv commented Apr 6, 2017

CyrusNajmabadi Apr 6, 2017

Choose a reason for hiding this comment

d0pare Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 6, 2017

d0pare commented Apr 6, 2017

CyrusNajmabadi commented Apr 6, 2017

CyrusNajmabadi commented Apr 6, 2017

d0pare commented Apr 6, 2017

jcouv commented Apr 6, 2017

d0pare commented Apr 6, 2017

CyrusNajmabadi commented Apr 6, 2017

d0pare commented Apr 6, 2017

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Apr 6, 2017

d0pare commented Apr 6, 2017

cston Apr 6, 2017

Choose a reason for hiding this comment

sharwell Apr 7, 2017

Choose a reason for hiding this comment

cston commented Apr 6, 2017

d0pare commented Apr 6, 2017

sharwell commented Apr 6, 2017 • edited Loading

sharwell commented Apr 6, 2017

d0pare commented Apr 7, 2017

d0pare commented Apr 7, 2017

cston commented Apr 7, 2017

d0pare commented Apr 7, 2017

sharwell commented Apr 7, 2017

d0pare commented Apr 7, 2017

d0pare commented Apr 7, 2017 • edited Loading

sharwell commented Apr 7, 2017

d0pare commented Apr 7, 2017

d0pare commented Apr 6, 2017 •

edited by sharwell

Loading

d0pare commented Apr 6, 2017 •

edited

Loading

d0pare Apr 6, 2017 •

edited

Loading

sharwell commented Apr 6, 2017 •

edited

Loading

d0pare commented Apr 7, 2017 •

edited

Loading