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

Fix trim analyzer warning for inferred type arguments #87156

Merged
merged 6 commits into from
Jun 12, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Jun 5, 2023

Fixes #86032.

This replaces the syntax-based logic with:

  • ISymbol-based logic that looks at generic instantiations in parameter types, return types, and base/interface types
  • IOperation-based logic that looks at invocations

The syntax-based logic used to also warn for the types of local variables, but SymbolKind.Local isn't supported for RegisterSymbolAction. Since this falls into the category of warnings that isn't strictly necessary (based on nativeaot logic which only warns on generic instantiations that can actually lead to code execution), I just left out local variables.

I'm slightly concerned that this could be missing some cases, but at least it works on our existing testcases.

There are two slight differences in warning locations, which I don't expect to cause issues for warning suppressions:

  • For invocations, the warning location used to be the generic method name, without the argument list. Using the IOperation-based approach, we use the invocation's location that includes the argument list.
  • For properties that use arrow syntax (RequireMethods<TFields> Property => null), the ISymbol-based approach warns on the underlying getter method, whose location seems to be the body of the accessor. We used to warn on the return type specifically, but I couldn't find a better way to get the location.

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

ghost commented Jun 5, 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 #86032.

This replaces the syntax-based logic with:

  • ISymbol-based logic that looks at generic instantiations in parameter types, return types, and base/interface types
  • IOperation-based logic that looks at invocations

The syntax-based logic used to also warn for the types of local variables, but SymbolKind.Local isn't supported for RegisterSymbolAction. Since this falls into the category of warnings that isn't strictly necessary (based on nativeaot logic which only warns on generic instantiations that can actually lead to code execution), I just left out local variables.

I'm slightly concerned that this could be missing some cases, but at least it works on our existing testcases.

There are two slight differences in warning locations, which I don't expect to cause issues for warning suppressions:

  • For invocations, the warning location used to be the generic method name, without the argument list. Using the IOperation-based approach, we use the invocation's location that includes the argument list.
  • For properties that use arrow syntax (RequireMethods<TFields> Property => null), the ISymbol-based approach warns on the underlying getter method, whose location seems to be the body of the accessor. We used to warn on the return type specifically, but I couldn't find a better way to get the location.
Author: sbomer
Assignees: -
Labels:

area-Tools-ILLink

Milestone: -

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

In general looks good.

- Allow illink to match ExpectedWarning on property against a
  warning produced from getter
- Add back NativeAOT_StorageSpaceType for illink warning
- Add comment about analyzer local variable behavior
@sbomer sbomer requested a review from vitek-karas June 9, 2023 20:47
@sbomer sbomer merged commit 91cc34c into dotnet:main Jun 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 13, 2023
@sbomer sbomer deleted the fixInferredGeneric 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
Development

Successfully merging this pull request may close these issues.

Trim Analyzer doesn't handle annotations on inferred generic arguments
2 participants