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 check for types interesting to dataflow #105642

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Jul 29, 2024

This makes the check IsTypeInterestingForDataflow more consistent across ILLink/ILC/analyzer.

Fixes #104761, and fixes analyzer behavior for byrefs of string.

@dotnet/appmodel PTAL

@sbomer sbomer requested a review from marek-safar as a code owner July 29, 2024 18:28
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Jul 29, 2024
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jul 29, 2024
@sbomer sbomer requested a review from agocke July 30, 2024 15:59
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static unsafe void RequirePublicMethods (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
Type* typePtr)
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure I understand what's going on here: we do not consider pointers of Type to be interesting, right? Or is #101211 saying that we shouldn't be doing that, but we are?

Copy link
Member Author

@sbomer sbomer Jul 31, 2024

Choose a reason for hiding this comment

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

Type* should not be considered for dataflow analysis. All the tools correctly report IL2098 due to invalid annotations on uninteresting Type*.

#101211 is about the analyzer still considering Type* in dataflow analysis, and incorrectly reporting IL2067 about annotations on typePtr mismatching the requirements of RequirePublicFields. Basically the IsTypeInterestingForDataflow check isn't wired up correctly in the analyzer, so it only impacts the IL2098 warnings, not the dataflow warnings.

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!

@sbomer
Copy link
Member Author

sbomer commented Jul 31, 2024

/ba-g "Helix work item dead-lettered"

@sbomer sbomer merged commit ed40e60 into dotnet:main Jul 31, 2024
75 of 77 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 31, 2024
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.

Placing DAM on a Type array crashes illink
2 participants