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.

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

Check Locations length before accessing #87659

Merged
merged 5 commits into from
Jun 17, 2023
Merged

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Jun 15, 2023

Fixes an IndexOutOfRangeException thrown by the analyzer when analyzing method parameters for the implicitly generated Main method when using top-level statements. These method parameters don't have location info because they are not in user code.

I'm also adding checks in the other places this analyzer accesses Locations[0], to be safe. But only one of these accesses was causing the crash reported in #87647. @vitek-karas @agocke please let me know if you have opinions on whether we should do a more targeted fix.

Fixes #87647

@sbomer sbomer requested review from agocke and vitek-karas June 15, 2023 22:11
@sbomer sbomer requested a review from marek-safar as a code owner June 15, 2023 22:11
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Jun 15, 2023
@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 15, 2023
@ghost ghost assigned sbomer Jun 15, 2023
@ghost
Copy link

ghost commented Jun 15, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes an IndexOutOfRangeException thrown by the analyzer when analyzing method parameters for the implicitly generated Main method when using top-level statements. These method parameters don't have location info because they are not in user code.

I'm also adding checks in the other places this analyzer accesses Locations[0], to be safe. But only one of these accesses was causing the crash reported in #87647. @vitek-karas @agocke please let me know if you have opinions on whether we should do a more targeted fix.

Author: sbomer
Assignees: -
Labels:

area-Tools-ILLink

Milestone: -


foreach (var parameter in method.Parameters) {
foreach (var diagnostic in ProcessGenericParameters (parameter.Type, parameter.Locations[0]))
context.ReportDiagnostic (diagnostic);
if (parameter.Locations.Length > 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This check is the only one necessary to fix the reported issue.

@radical
Copy link
Member

radical commented Jun 16, 2023

Wasm tests are still failing with the same error:
error AD0001: Analyzer 'ILLink.RoslynAnalyzer.DynamicallyAccessedMembersAnalyzer' threw an exception of type 'System.IndexOutOfRangeException' with message 'Index was outside the bounds of the array.'.

We use the latest 8.0 sdk to run the tests, so we would need to wait for this change to show up in a new sdk.
They can be ignored here.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 16, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 16, 2023
@sbomer sbomer requested a review from vitek-karas June 16, 2023 17:00
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

public static async Task VerifyAnalyzerAsync (string src, (string, string)[]? analyzerOptions = null, IEnumerable<MetadataReference>? additionalReferences = null, params DiagnosticResult[] expected)
public static async Task VerifyAnalyzerAsync (
string src,
bool consoleApplication,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Make it false by default to avoid changing so many lines - and also to simplify the most common usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find a good way to make it optional - the second argument seems to be always treated as part of the params[] args: https://sharplab.io/#v2:C4LgTgrgdgNAJiA1AHwMIDoCyAKARACwFMAbYge1xgAIBGamgSgG4BYAKHYAEAmK1KgN7sqIqpwDMYmgDYxAFio5ONAAxUAztQBGZMsSpaqAXioAzAIbF1hagAdzYcwFt1VAJZRgAbQC6VBwDm6gyCAL7soUA===.

@sbomer sbomer merged commit fe88013 into dotnet:main Jun 17, 2023
SteveSandersonMS added a commit to dotnet/aspnetcore that referenced this pull request Jun 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 17, 2023
@sbomer sbomer deleted the locationFix branch November 3, 2023 18:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
4 participants