Skip to content

Conversation

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Jul 23, 2024

Fixes #72012
Fixes #74500
Fixes #74383

@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 23, 2024
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review July 23, 2024 19:54
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner July 23, 2024 19:54
@CyrusNajmabadi CyrusNajmabadi requested a review from dibarbet July 23, 2024 19:54
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
Contributor 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
Contributor 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.

items.SyntacticArgumentCount,
items.ArgumentName,
selectedParameter: 0,
userSelected);
Copy link
Contributor 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.

model.SemanticParameterIndex,
model.SyntacticArgumentCount,
model.ArgumentName,
isCaseSensitive);
Copy link
Contributor 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.

int semanticParameterIndex,
int syntacticArgumentCount,
string name,
bool isCaseSensitive)
Copy link
Contributor 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.

public int SemanticParameterIndex { get; }

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

Choose a reason for hiding this comment

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

just renames.

if (argumentCount < argumentIndex)
{
throw new ArgumentException($"{nameof(argumentCount)} < {nameof(argumentIndex)}. {argumentCount} < {argumentIndex}", nameof(argumentIndex));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the crux of hte bug. a check that literally made no sense. the number of syntactic arguments provided has no basis on which semantic parameter we think is teh right one to select. It's just information about the syntax tree, that's all.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge July 23, 2024 20:00
@CyrusNajmabadi CyrusNajmabadi merged commit 415cea3 into dotnet:main Jul 23, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 23, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the sigHelpCrash branch July 23, 2024 23:50
@KirillOsenkov
Copy link
Member

Thanks for fixing, this was for me the #1 most annoying/visible Roslyn bug for my usage in the past year or maybe more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

4 participants