Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix crash in sighelp #74510

Merged
merged 6 commits into from
Jul 23, 2024
Merged

Fix crash in sighelp #74510

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
deb8e23
Initial renames
CyrusNajmabadi Jul 23, 2024
96e5501
renames
CyrusNajmabadi Jul 23, 2024
e90aad6
Downstream
CyrusNajmabadi Jul 23, 2024
e46eb4a
test fallout
CyrusNajmabadi Jul 23, 2024
0863c70
Add test
CyrusNajmabadi Jul 23, 2024
62680ba
Fix
CyrusNajmabadi Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/EditorFeatures/Core.Wpf/SignatureHelp/Controller.Session_ComputeModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ private async Task<Model> ComputeModelInBackgroundAsync(
currentModel.Provider == provider &&
currentModel.GetCurrentSpanInSubjectBuffer(disconnectedBufferGraph.SubjectBufferSnapshot).Span.Start == items.ApplicableSpan.Start &&
currentModel.Items.IndexOf(currentModel.SelectedItem) == items.SelectedItemIndex &&
currentModel.ArgumentIndex == items.ArgumentIndex &&
currentModel.ArgumentCount == items.ArgumentCount &&
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed these to make the distinction clear. 'argument count' refferred to how many explicit argument syntax nodes are present in the code. 'argument index' was very badly named. It indicated 'this is the parameter in the actual symcol we think the user is currently on'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: the pr is almost entirely this rename. i call out anything that isn't just that.

currentModel.SemanticParameterIndex == items.SemanticParameterIndex &&
currentModel.SyntacticArgumentCount == items.SyntacticArgumentCount &&
currentModel.ArgumentName == items.ArgumentName)
{
// The new model is the same as the current model. Return the currentModel
Expand All @@ -112,14 +112,28 @@ private async Task<Model> ComputeModelInBackgroundAsync(

var selectedItem = GetSelectedItem(currentModel, items, provider, out var userSelected);

var model = new Model(disconnectedBufferGraph, items.ApplicableSpan, provider,
items.Items, selectedItem, items.ArgumentIndex, items.ArgumentCount, items.ArgumentName,
selectedParameter: 0, userSelected);
var model = new Model(
disconnectedBufferGraph,
items.ApplicableSpan,
provider,
items.Items,
selectedItem,
items.SemanticParameterIndex,
items.SyntacticArgumentCount,
items.ArgumentName,
selectedParameter: 0,
userSelected);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just renames + wrapping. no other change.


var syntaxFactsService = document.GetLanguageService<ISyntaxFactsService>();
var isCaseSensitive = syntaxFactsService == null || syntaxFactsService.IsCaseSensitive;
var selection = DefaultSignatureHelpSelector.GetSelection(model.Items,
model.SelectedItem, model.UserSelected, model.ArgumentIndex, model.ArgumentCount, model.ArgumentName, isCaseSensitive);
var selection = DefaultSignatureHelpSelector.GetSelection(
model.Items,
model.SelectedItem,
model.UserSelected,
model.SemanticParameterIndex,
model.SyntacticArgumentCount,
model.ArgumentName,
isCaseSensitive);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just renames + wrapping. no other change.


return model.WithSelectedItem(selection.SelectedItem, selection.UserSelected)
.WithSelectedParameter(selection.SelectedParameter);
Expand Down
30 changes: 18 additions & 12 deletions src/EditorFeatures/Core.Wpf/SignatureHelp/Controller.Session_UpdateModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@ public static SignatureHelpSelection GetSelection(
IList<SignatureHelpItem> items,
SignatureHelpItem selectedItem,
bool userSelected,
int argumentIndex,
int argumentCount,
int semanticParameterIndex,
int syntacticArgumentCount,
string argumentName,
bool isCaseSensitive)
{
SelectBestItem(ref selectedItem, ref userSelected, items, argumentIndex, argumentCount, argumentName, isCaseSensitive);
var selectedParameter = GetSelectedParameter(selectedItem, argumentIndex, argumentName, isCaseSensitive);
SelectBestItem(ref selectedItem, ref userSelected, items, semanticParameterIndex, syntacticArgumentCount, argumentName, isCaseSensitive);
var selectedParameter = GetSelectedParameter(selectedItem, semanticParameterIndex, argumentName, isCaseSensitive);
return new SignatureHelpSelection(selectedItem, userSelected, selectedParameter);
}

Expand All @@ -67,12 +67,18 @@ private static int GetSelectedParameter(SignatureHelpItem bestItem, int paramete
return parameterIndex;
}

private static void SelectBestItem(ref SignatureHelpItem currentItem, ref bool userSelected,
IList<SignatureHelpItem> filteredItems, int selectedParameter, int argumentCount, string name, bool isCaseSensitive)
private static void SelectBestItem(
ref SignatureHelpItem currentItem,
ref bool userSelected,
IList<SignatureHelpItem> filteredItems,
int semanticParameterIndex,
int syntacticArgumentCount,
string name,
bool isCaseSensitive)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just renames + wrapping. no other change.

{
// If the current item is still applicable, then just keep it.
if (filteredItems.Contains(currentItem) &&
IsApplicable(currentItem, argumentCount, name, isCaseSensitive))
IsApplicable(currentItem, syntacticArgumentCount, name, isCaseSensitive))
{
// If the current item was user-selected, we keep it as such.
return;
Expand All @@ -86,7 +92,7 @@ private static void SelectBestItem(ref SignatureHelpItem currentItem, ref bool u
// selected parameter was outside the bounds of all methods. i.e. all methods only
// went up to 3 parameters, and selected parameter is 3 or higher. In that case,
// just pick the very last item as it is closest in parameter count.
var result = filteredItems.FirstOrDefault(i => IsApplicable(i, argumentCount, name, isCaseSensitive));
var result = filteredItems.FirstOrDefault(i => IsApplicable(i, semanticParameterIndex, name, isCaseSensitive));
if (result != null)
{
currentItem = result;
Expand All @@ -97,7 +103,7 @@ private static void SelectBestItem(ref SignatureHelpItem currentItem, ref bool u
// a name.
if (name != null)
{
SelectBestItem(ref currentItem, ref userSelected, filteredItems, selectedParameter, argumentCount, null, isCaseSensitive);
SelectBestItem(ref currentItem, ref userSelected, filteredItems, semanticParameterIndex, syntacticArgumentCount, null, isCaseSensitive);
return;
}

Expand All @@ -112,7 +118,7 @@ private static void SelectBestItem(ref SignatureHelpItem currentItem, ref bool u
currentItem = lastItem;
}

private static bool IsApplicable(SignatureHelpItem item, int argumentCount, string name, bool isCaseSensitive)
private static bool IsApplicable(SignatureHelpItem item, int syntacticArgumentCount, string name, bool isCaseSensitive)
{
// If they provided a name, then the item is only valid if it has a parameter that
// matches that name.
Expand All @@ -126,7 +132,7 @@ private static bool IsApplicable(SignatureHelpItem item, int argumentCount, stri
// parameter index. i.e. if it has 2 parameters and we're at index 0 or 1 then it's
// applicable. However, if it has 2 parameters and we're at index 2, then it's not
// applicable.
if (item.Parameters.Length >= argumentCount)
if (item.Parameters.Length >= syntacticArgumentCount)
{
return true;
}
Expand All @@ -141,7 +147,7 @@ private static bool IsApplicable(SignatureHelpItem item, int argumentCount, stri
// Also, we special case 0. that's because if the user has "Goo(" and goo takes no
// arguments, then we'll see that it's arg count is 0. We still want to consider
// any item applicable here though.
return argumentCount == 0;
return syntacticArgumentCount == 0;
}
}
}
Expand Down
20 changes: 12 additions & 8 deletions src/EditorFeatures/Core.Wpf/SignatureHelp/Model.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,12 @@ internal class Model

/// <summary>UserSelected is true if the SelectedItem is the result of a user selection (up/down arrows).</summary>
public bool UserSelected { get; }
public int ArgumentIndex { get; }
public int ArgumentCount { get; }

/// <inheritdoc cref="SignatureHelpItems.SemanticParameterIndex"/>
public int SemanticParameterIndex { get; }

/// <inheritdoc cref="SignatureHelpItems.SyntacticArgumentCount"/>
public int SyntacticArgumentCount { get; }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just renames.

public string ArgumentName { get; }
public int? SelectedParameter { get; }
public ISignatureHelpProvider Provider { get; }
Expand All @@ -35,8 +39,8 @@ public Model(
ISignatureHelpProvider provider,
IList<SignatureHelpItem> items,
SignatureHelpItem selectedItem,
int argumentIndex,
int argumentCount,
int semanticParameterIndex,
int syntacticArgumentCount,
string argumentName,
int? selectedParameter,
bool userSelected)
Expand All @@ -51,8 +55,8 @@ public Model(
this.Provider = provider;
this.SelectedItem = selectedItem;
this.UserSelected = userSelected;
this.ArgumentIndex = argumentIndex;
this.ArgumentCount = argumentCount;
this.SemanticParameterIndex = semanticParameterIndex;
this.SyntacticArgumentCount = syntacticArgumentCount;
this.ArgumentName = argumentName;
this.SelectedParameter = selectedParameter;
}
Expand All @@ -61,14 +65,14 @@ public Model WithSelectedItem(SignatureHelpItem selectedItem, bool userSelected)
{
return selectedItem == this.SelectedItem && userSelected == this.UserSelected
? this
: new Model(_disconnectedBufferGraph, TextSpan, Provider, Items, selectedItem, ArgumentIndex, ArgumentCount, ArgumentName, SelectedParameter, userSelected);
: new Model(_disconnectedBufferGraph, TextSpan, Provider, Items, selectedItem, SemanticParameterIndex, SyntacticArgumentCount, ArgumentName, SelectedParameter, userSelected);
}

public Model WithSelectedParameter(int? selectedParameter)
{
return selectedParameter == this.SelectedParameter
? this
: new Model(_disconnectedBufferGraph, TextSpan, Provider, Items, SelectedItem, ArgumentIndex, ArgumentCount, ArgumentName, selectedParameter, UserSelected);
: new Model(_disconnectedBufferGraph, TextSpan, Provider, Items, SelectedItem, SemanticParameterIndex, SyntacticArgumentCount, ArgumentName, selectedParameter, UserSelected);
}

public SnapshotSpan GetCurrentSpanInSubjectBuffer(ITextSnapshot bufferSnapshot)
Expand Down
12 changes: 6 additions & 6 deletions src/EditorFeatures/Test2/IntelliSense/SignatureHelpControllerTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense
slowProvider.Setup(Function(p) p.IsTriggerCharacter(" "c)).Returns(True)
slowProvider.Setup(Function(p) p.IsRetriggerCharacter(" "c)).Returns(True)
slowProvider.Setup(Function(p) p.GetItemsAsync(It.IsAny(Of Document), It.IsAny(Of Integer), It.IsAny(Of SignatureHelpTriggerInfo), options, It.IsAny(Of CancellationToken))) _
.Returns(Task.FromResult(New SignatureHelpItems(CreateItems(2), TextSpan.FromBounds(0, 0), selectedItem:=0, argumentIndex:=0, argumentCount:=0, argumentName:=Nothing)))
.Returns(Task.FromResult(New SignatureHelpItems(CreateItems(2), TextSpan.FromBounds(0, 0), selectedItem:=0, semanticParameterIndex:=0, syntacticArgumentCount:=0, argumentName:=Nothing)))
Dim controller = CreateController(CreateWorkspace(), provider:=slowProvider.Object, waitForPresentation:=True)

' Now force an update to the model that will result in stopping the session
Expand Down Expand Up @@ -115,7 +115,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense
slowProvider.Setup(Function(p) p.GetItemsAsync(It.IsAny(Of Document), It.IsAny(Of Integer), It.IsAny(Of SignatureHelpTriggerInfo), options, It.IsAny(Of CancellationToken))) _
.Returns(Function()
mre.WaitOne()
Return Task.FromResult(New SignatureHelpItems(CreateItems(2), TextSpan.FromBounds(0, 0), selectedItem:=0, argumentIndex:=0, argumentCount:=0, argumentName:=Nothing))
Return Task.FromResult(New SignatureHelpItems(CreateItems(2), TextSpan.FromBounds(0, 0), selectedItem:=0, semanticParameterIndex:=0, syntacticArgumentCount:=0, argumentName:=Nothing))
End Function)

GetMocks(controller).PresenterSession.Setup(Sub(p) p.Dismiss())
Expand All @@ -135,7 +135,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense
slowProvider.Setup(Function(p) p.GetItemsAsync(It.IsAny(Of Document), It.IsAny(Of Integer), It.IsAny(Of SignatureHelpTriggerInfo), options, It.IsAny(Of CancellationToken))) _
.Returns(Function()
mre.WaitOne()
Return Task.FromResult(New SignatureHelpItems(CreateItems(2), TextSpan.FromBounds(0, 0), selectedItem:=0, argumentIndex:=0, argumentCount:=0, argumentName:=Nothing))
Return Task.FromResult(New SignatureHelpItems(CreateItems(2), TextSpan.FromBounds(0, 0), selectedItem:=0, semanticParameterIndex:=0, syntacticArgumentCount:=0, argumentName:=Nothing))
End Function)

GetMocks(controller).PresenterSession.Setup(Sub(p) p.Dismiss())
Expand All @@ -157,7 +157,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense
slowProvider.Setup(Function(p) p.IsTriggerCharacter(" "c)).Returns(True)
slowProvider.Setup(Function(p) p.IsRetriggerCharacter(" "c)).Returns(True)
slowProvider.Setup(Function(p) p.GetItemsAsync(It.IsAny(Of Document), It.IsAny(Of Integer), It.IsAny(Of SignatureHelpTriggerInfo), options, It.IsAny(Of CancellationToken))) _
.Returns(Task.FromResult(New SignatureHelpItems(CreateItems(2), TextSpan.FromBounds(0, 0), selectedItem:=0, argumentIndex:=0, argumentCount:=0, argumentName:=Nothing)))
.Returns(Task.FromResult(New SignatureHelpItems(CreateItems(2), TextSpan.FromBounds(0, 0), selectedItem:=0, semanticParameterIndex:=0, syntacticArgumentCount:=0, argumentName:=Nothing)))

Await threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync()
Dim controller = CreateController(workspace, provider:=slowProvider.Object, waitForPresentation:=True)
Expand All @@ -168,7 +168,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense
slowProvider.Setup(Function(p) p.GetItemsAsync(It.IsAny(Of Document), It.IsAny(Of Integer), It.IsAny(Of SignatureHelpTriggerInfo), options, It.IsAny(Of CancellationToken))) _
.Returns(Function()
checkpoint.Task.Wait()
Return Task.FromResult(New SignatureHelpItems(CreateItems(2), TextSpan.FromBounds(0, 2), selectedItem:=0, argumentIndex:=0, argumentCount:=0, argumentName:=Nothing))
Return Task.FromResult(New SignatureHelpItems(CreateItems(2), TextSpan.FromBounds(0, 2), selectedItem:=0, semanticParameterIndex:=0, syntacticArgumentCount:=0, argumentName:=Nothing))
End Function)

Await threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync()
Expand Down Expand Up @@ -355,7 +355,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense
Public Function GetItemsAsync(document As Document, position As Integer, triggerInfo As SignatureHelpTriggerInfo, options As MemberDisplayOptions, cancellationToken As CancellationToken) As Task(Of SignatureHelpItems) Implements ISignatureHelpProvider.GetItemsAsync
GetItemsCount += 1
Return Task.FromResult(If(_items.Any(),
New SignatureHelpItems(_items, TextSpan.FromBounds(position, position), selectedItem:=0, argumentIndex:=0, argumentCount:=0, argumentName:=Nothing),
New SignatureHelpItems(_items, TextSpan.FromBounds(position, position), selectedItem:=0, semanticParameterIndex:=0, syntacticArgumentCount:=0, argumentName:=Nothing),
Nothing))
End Function

Expand Down
4 changes: 2 additions & 2 deletions src/EditorFeatures/TestUtilities/SignatureHelp/AbstractSignatureHelpProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ protected virtual async Task VerifyCurrentParameterNameAsync(string markup, stri
private static async Task<SignatureHelpState?> GetArgumentStateAsync(int cursorPosition, Document document, ISignatureHelpProvider signatureHelpProvider, SignatureHelpTriggerInfo triggerInfo, MemberDisplayOptions options)
{
var items = await signatureHelpProvider.GetItemsAsync(document, cursorPosition, triggerInfo, options, CancellationToken.None);
return items == null ? null : new SignatureHelpState(items.ArgumentIndex, items.ArgumentCount, items.ArgumentName, argumentNames: default);
return items == null ? null : new SignatureHelpState(items.SemanticParameterIndex, items.SyntacticArgumentCount, items.ArgumentName, ArgumentNames: default);
}

private async Task VerifyCurrentParameterNameWorkerAsync(string markup, string expectedParameterName, SourceCodeKind sourceCodeKind)
Expand Down Expand Up @@ -237,7 +237,7 @@ private static void CompareSigHelpItemsAndCurrentPosition(

if (expectedTestItem.CurrentParameterIndex != null)
{
Assert.True(expectedTestItem.CurrentParameterIndex == items.ArgumentIndex, $"The current parameter is {items.ArgumentIndex}, but we expected {expectedTestItem.CurrentParameterIndex}");
Assert.True(expectedTestItem.CurrentParameterIndex == items.SemanticParameterIndex, $"The current parameter is {items.SemanticParameterIndex}, but we expected {expectedTestItem.CurrentParameterIndex}");
}

if (expectedTestItem.Description != null)
Expand Down
8 changes: 4 additions & 4 deletions src/Features/CSharp/Portable/SignatureHelp/TupleConstructionSignatureHelpProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ public TupleConstructionSignatureHelpProvider()
if (currentSpan.Start == parenthesizedExpression.SpanStart)
{
return new SignatureHelpState(
argumentIndex: 0,
argumentCount: 0,
argumentName: string.Empty,
argumentNames: default);
SemanticParameterIndex: 0,
SyntacticArgumentCount: 0,
ArgumentName: string.Empty,
ArgumentNames: default);
}
}

Expand Down
Loading
Loading