Signature help improvements for C# tuples (renamed to work around jenkins issues) #14289
Conversation
rchande
commented
Oct 5, 2016
rchande
commented
- The invocation/initializer/objectcreation sighelp providers no longer dismiss sighelp if the user starts typing a tuple
- When the user starts typing a tuple, show signature help for the tuple member types/names if the type of the tuple can be inferred
|
Tag @dotnet/roslyn-ide for review |
|
Fixes #14138 |
|
@dotnet-bot test windows_release_unit64_prtest please |
|
@dotnet-bot test windows_eta_open_prtest please |
| var expectedOrderedItems = new List<SignatureHelpTestItem>(); | ||
| expectedOrderedItems.Add(new SignatureHelpTestItem("int C.Foo(object x)", currentParameterIndex: 0)); | ||
|
|
||
| await TestAsync(markup, expectedOrderedItems, usePreviousCharAsTrigger: true); |
There was a problem hiding this comment.
To verify dismissal locgic, you need to use the CommandHandler test suite. This test framework cannot do it properly.
There was a problem hiding this comment.
In this case, dismissal happens because the user types a '(' or ',' and the system asks the provider to trigger on that typed char. We used to dismiss because we wouldn't trigger on the start of a tuple. I'll rename the tests to indicate this.
| class C | ||
| { | ||
| (int, int) y = [|($$) | ||
| |]}"; |
There was a problem hiding this comment.
what is the [||] for? Does it indicate the span of sig help? If so, why is the span here going out of the bounds of hte tuple?
There was a problem hiding this comment.
Fixed, needed to use the expression's Span, not FullSpan.
| var expectedOrderedItems = new List<SignatureHelpTestItem>(); | ||
| expectedOrderedItems.Add(new SignatureHelpTestItem("(int, int)", currentParameterIndex: 1)); | ||
|
|
||
| await TestAsync(markup, expectedOrderedItems, usePreviousCharAsTrigger: true); |
There was a problem hiding this comment.
typing space triggers sig help?
| var expectedOrderedItems = new List<SignatureHelpTestItem>(); | ||
| expectedOrderedItems.Add(new SignatureHelpTestItem("(int, int)", currentParameterIndex: 1)); | ||
|
|
||
| await TestAsync(markup, expectedOrderedItems, usePreviousCharAsTrigger: true); |
There was a problem hiding this comment.
previous char is space, not a comma.
| </Document>) | ||
|
|
||
| state.SendInvokeSignatureHelp() | ||
| Await state.AssertSelectedSignatureHelpItem(displayText:="(int a, string b)", selectedParameter:="string b") |
There was a problem hiding this comment.
why is the selected paramter "string b"?
| if (token.IsKind(SyntaxKind.OpenParenToken) && token.Parent.IsKind(SyntaxKind.ParenthesizedExpression)) | ||
| { | ||
| var parenthesizedExpr = ((ParenthesizedExpressionSyntax)token.Parent).WalkUpParentheses(); | ||
| return parenthesizedExpr.Parent is ArgumentSyntax && |
There was a problem hiding this comment.
this is expression is too complex and confusing. Can you break it out into simpler cases?
| @@ -0,0 +1,192 @@ | |||
| using System; | |||
| if (TryGetTupleExpression(SignatureHelpTriggerReason.InvokeSignatureHelpCommand, | ||
| root, position, syntaxFacts, cancellationToken, out expression) && | ||
| currentSpan.Start == expression.SpanStart) | ||
| { |
There was a problem hiding this comment.
too complex. Just have nested if statements.
| // This could only have parsed as a parenthesized expression in these two cases: | ||
| // ($$) | ||
| // (name$$) | ||
| string name = 0.ToString(); // This causes the controller to match against the 0th tuple member |
| string name = 0.ToString(); // This causes the controller to match against the 0th tuple member | ||
| if (parenthesizedExpression.Expression is IdentifierNameSyntax) | ||
| { | ||
| name = ((IdentifierNameSyntax)parenthesizedExpression.Expression).Identifier.ValueText; |
There was a problem hiding this comment.
I don't understsand what this is for.
| return new SignatureHelpState( | ||
| argumentIndex: 0, | ||
| argumentCount: 0, | ||
| argumentName: name, |
There was a problem hiding this comment.
tuples don't have reorderable named arguments. i.e. if i have:
(int a, int b) = (b: 0, a: 0);
Then even in teh "b:0" case, we're still in the 0th argument that corresponds to 'int a'.
| typeParts.Add(new SymbolDisplayPart(SymbolDisplayPartKind.PropertyName, null, elementName)); | ||
| } | ||
|
|
||
| result.Add(new SignatureHelpParameter(parameterItemName, false, c => null, typeParts)); |
|
|
||
| // The name used by the controller when selecting parameters | ||
| // Each element needs a unique name to make selection work property | ||
| private string GetParameterName(ImmutableArray<string> tupleElementNames, int i) |
There was a problem hiding this comment.
Won't these be one-off? Tuple item names start at Item1 not Item0.
| return tupleElementNames[i] ?? string.Empty; | ||
| } | ||
|
|
||
| private bool TryGetTupleExpression(SignatureHelpTriggerReason triggerReason, SyntaxNode root, int position, ISyntaxFactsService syntaxFacts, CancellationToken cancellationToken, out TupleExpressionSyntax tupleExpression) |
There was a problem hiding this comment.
wrap long lines that don't fit in github PR.
|
@333fred tests failed with: hudson.util.HudsonFailedToLoad: java.io.IOException: Failed to create a temporary file in /jenkins Another known issue? |
|
test closed-perf please |
|
test perf-closed |
|
test perf-closed please |
2 similar comments
|
test perf-closed please |
|
test perf-closed please |
|
retest this please |
|
test closed-perf please |
|
@rchande do you know which test is the one that failed? I'm attempting to find the build, but I don't know which one broke. |
|
Sorry, @333fred, I didn't record which build that was. |
|
@333fred FWIW, it looks like the tests are actually getting run now (not that that helps your investigation). |
|
@dotnet-bot test perf please |
|
test perf-closed please |
|
retest vsi please |
rchande
changed the title
|
@333fred Looks like a bunch of unit tests are failing with: Known issue? |
|
test internal-perf please |
|
@dotnet-bot test internal-perf please |
|
@rchande, that's a not a known issue. |
|
Actually, @rchande, I believe they ran on one of the nodes that we just deleted as it had the wrong image. I'd try running it again and see if it's still failing. |
|
Thanks @333fred will retest |
|
retest this please |
|
retest eta please |
Signature help improvements for C# tuples (renamed to work around jenkins issues)
