Extract method refactoring should handle tuples in VB #13278
Conversation
| @@ -2412,6 +2412,20 @@ System.ValueTuple`2[System.ValueTuple`2[System.Int32,System.Object],System.Value | |||
|
|
|||
| ]]>) | |||
| End Sub | |||
|
|
|||
| <Fact(Skip:="https://github.com/dotnet/roslyn/issues/13277"), WorkItem(13277, "https://github.com/dotnet/roslyn/issues/13277")> | |||
There was a problem hiding this comment.
we put workitem on the next line. Also, generally try to avoid lines that are too long for github.
|
LGTM, but i think you could really simplify the construction of the node. |
jcouv
force-pushed
the
vb-extract-method
branch
2 times, most recently
from
21cf0d5
to
9d70d38
Compare
| Public Async Function TestTuples() As Task | ||
|
|
||
| Await TestAsync( | ||
| NewLines("Class Program \n Sub Main(args As String()) \n [|Dim x = (1, 2)|] \n M(x) \n End Sub |
There was a problem hiding this comment.
you don't need this either. VB natively supports multiline string literals now :) SO just make a single string literal that contains the test you want.
There was a problem hiding this comment.
Oh, I see what you mean now. Fixed
|
👍 |
|
@VSadov Could you review this small change? Thanks |
jcouv
force-pushed
the
vb-extract-method
branch
from
ec41e56
to
2164545
Compare
|
@VSadov Please review when you have some time. Thanks |
|
LGTM |
|
Thanks Vlad. |
|
@dotnet/roslyn-compiler for a second compiler review. Thanks |
| TypeSymbol intType = comp.GetSpecialType(SpecialType.System_Int32); | ||
| var vt2 = comp.CreateErrorTypeSymbol(null, "ValueTuple", 2).Construct(intType, intType); | ||
|
|
||
| Assert.Throws<ArgumentException>(() => comp.CreateTupleTypeSymbol(vt2, default(ImmutableArray<string>))); |
There was a problem hiding this comment.
Minor point: Since elementNames argument is optional, consider dropping that parameter so it's clear the ArgumentException is from the vt2 argument.
|
LGTM |
|
Thanks! I'll wait for all-green, then merge. |
Refactorings involving a VB tuple type would result in
Objectinstead of the proper tuple type.That is fixed by the change in
CreateSimpleTypeSyntax(matching the corresponding C# code).Beyond that, there was also a (sneaky) issue with the simplification code. It would leave
(System.Int32, System.Int32), instead of simplifying to(Integer, Integer).This was caused by
ExpressionSyntaxExpressions.TryReducelooking up the type forSystem.Int32(see call toSimplificationHelpers.GetOriginalSymbolInfo) but finding no symbol. The fix is inSyntaxFacts.IsInTypeOnlyContext. It turns out the same fix was already introduced in C# for some other scenario...@CyrusNajmabadi @VSadov @dotnet/roslyn-compiler for review.
Fixes #13042