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

Report Requires diagnostics from dataflow analyzer #92724

Merged
merged 16 commits into from
Oct 23, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Sep 27, 2023

This moves the logic for generating diagnostics about RequiresUnreferencedCode,
RequiresAssemblyFiles, and RequiresDynamicCode attributes from the separate
Requires* analyzers into the DynamicallyAccessedMembersAnalyzer.

This includes logic for warning on access to attributed members, not the warnings
about mismatching attributes on base/override methods. The override validation
logic is still handled by the respective analyzers.

The DynamicallyAccessedMembersAnalyzer is now turned on by any of the settings
EnableTrimAnalyzer, EnableAotAnalyzer, or EnableSingleFileAnalyzer.

A new type RequiresAnalyzerContext is used to cache per-compilation state of
the analyzers, to avoid recomputing it for each trim analysis pattern that
might potentially produce a warning. The context only stores state for enabled
analyzers, based on the MSBuild property values.

There are a few minor differences in the warning behavior with this change:

  • Implicit calls to annotated base ctors warn because they are visible in the CFG
  • Unused local functions aren't discovered by the analysis, so don't warn
    (matching linker/AOT behavior)

This moves the logic for generating diagnostics about RequiresUnreferencedCode,
RequiresAssemblyFiles, and RequiresDynamicCode attributes from the separate
Requires* analyzers into the DynamicallyAccessedMembersAnalyzer.

This includes logic for warning on access to attributed members, not the warnings
about mismatching attributes on base/override methods. The override validation
logic is still handled by the respective analyzers.

The DynamicallyAccessedMembersAnalyzer is now turned on by any of the settings
EnableTrimAnalyzer, EnableAotAnalyzer, or EnableSingleFileAnalyzer.

A new type RequiresAnalyzerContext is used to cache per-compilation state of
the analyzers, to avoid recomputing it for each trim analysis pattern that
might potentially produce a warning. The context only stores state for enabled
analyzers, based on the MSBuild property values.
@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Sep 27, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Sep 27, 2023
@ghost ghost assigned sbomer Sep 27, 2023
@ghost
Copy link

ghost commented Sep 27, 2023

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

This moves the logic for generating diagnostics about RequiresUnreferencedCode,
RequiresAssemblyFiles, and RequiresDynamicCode attributes from the separate
Requires* analyzers into the DynamicallyAccessedMembersAnalyzer.

This includes logic for warning on access to attributed members, not the warnings
about mismatching attributes on base/override methods. The override validation
logic is still handled by the respective analyzers.

The DynamicallyAccessedMembersAnalyzer is now turned on by any of the settings
EnableTrimAnalyzer, EnableAotAnalyzer, or EnableSingleFileAnalyzer.

A new type RequiresAnalyzerContext is used to cache per-compilation state of
the analyzers, to avoid recomputing it for each trim analysis pattern that
might potentially produce a warning. The context only stores state for enabled
analyzers, based on the MSBuild property values.

Author: sbomer
Assignees: -
Labels:

linkable-framework

Milestone: -

var tree = compilation.SyntaxTrees.FirstOrDefault ();
if (tree is null) {
return null;
}
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 didn't seem necessary and required passing more context (Compilation) down into the warning logic, so I removed it.

@sbomer
Copy link
Member Author

sbomer commented Oct 2, 2023

@vitek-karas @agocke PTAL

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.

Looks good - the only problem I see is the potentially problematic suppression of warnings due to RUC/RDC/RAF in presense of intrinsic handling of AOT specific behaviors. But I don't think we have such a case yet, so it doesn't block functionality... yet.

{
DiagnosticContext diagnosticContext = new (Operation.Syntax.GetLocation ());
foreach (var diagnostic in ReflectionAccessAnalyzer.GetDiagnosticsForReflectionAccessToDAMOnMethod (diagnosticContext, ReferencedMethod))
yield return diagnostic;
if (!OwningSymbol.IsInRequiresUnreferencedCodeAttributeScope (out _)) {
Copy link
Member

Choose a reason for hiding this comment

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

We could make it more similar to how this is done in the linker/AOT - there the DiagnosticContext is created with the information if some of the warnings should be disabled. Which is initialized by the IsInRequires...Scope methods.
The way it works here is probably a little bit more efficient, but it migth become a problem if we want to handle single-file and AOT warnings here as well. For example - eventually we would like to do some more clever processing around MakeGeneric ... solely AOT related stuff. That would need to happen inside the MethodCallPattern - and the current setup would not work for it (those warnings would be suppressed by putting a RUC on the scope - which is wrong)

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 see what you mean. The analyzer approach for RequiresAttribute warnings below (foreach analyzer, requiresAnalyzer.CheckAndCreateRequiresDiagnostic) doesn't work so well with the way DiagnosticContext is defined in AOT since each analyzer only handles logic specific to that attribute.

I think a good way to unify them would be to change AOT's attribute handling to check ShouldSuppressAnalysisWarningsForRequires inside of the ReflectionMethodBodyScanner, in CheckAndReportRequires. Then we could adopt the same pattern in the analyzer.

I'll look into this as a follow-up.

Comment on lines 83 to 85
// Some RUC-annotated APIs are intrinsically handled by the trimmer
if (member is IMethodSymbol method && Intrinsics.GetIntrinsicIdForMethod (new MethodProxy (method)) != IntrinsicId.None) {
return true;
Copy link
Member

Choose a reason for hiding this comment

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

This is unfortunate - that said we have a similar hack in the trimmer. I guess improvement in the future - how to handle RUC and similar correctly with the intrinsics handling.

Copy link
Member Author

Choose a reason for hiding this comment

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

To make sure I understand the concern - the problem you're pointing out is that any AOT intrinsics in GetIntrinsicIdForMethod will now not warn for RUC which is wrong in general (the RUC diagnostics should only be relevant for trimming but not AOT intrinsics), right?

I'd agree with that. It's independent of this change, but something to keep an eye on as we add intrinsics.

Copy link
Member

Choose a reason for hiding this comment

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

That is more the point of the comment here: #92724 (comment)

This one is about the fact that we have a "External" way of suppressing RUC warnings on intrinsics. In here, we check if a method is intrinsic and then trust the other analyzer it will do its job instead. In linker we use this giant enum which describes which pattern the warning came from, and then based on that we suppress the "general" RUC processing and rely on the method body scanner instead.

NativeAOT is the only implementation where this weird approach is not used - there it's all part of the main "handle call" functionality (still not ideal where it is, but it's much better), and we don't need a way to determine if something will be handled by the intrinsics or other mechanics to decide if we should apply RUC logic or not.

This is definitely not something to fix in this PR though.

- Clean up a few unused usings
- Remove unused Instance field
And related cases involving implicit indexer references.
Extra warnings were being produced on some projects where
the single-file analyzer was enabled, causing the DAM analyzer
to light up. The dataflow warnings were missing a check of
EnableTrimAnalzer to prevent these warnings.

This also lifts the checking of MSBuild property values out into
the compilation start action, so it only needs to be done per
compilation. The context type has been renamed to reflect that
it holds state relevant to all of the analyses handled by the
dataflow logic now.
@sbomer
Copy link
Member Author

sbomer commented Oct 21, 2023

Fixed a few more asserts that were hitting after merging in #93259, and added test coverage for the asserts and related cases that came to mind:

  • Ref property passed as out param
  • Delegate creation of an invocation operation

Also added a missing check for EnableTrimAnalyzer in the DAM analyzer.

And add missing checks in DAM analyzer
@sbomer
Copy link
Member Author

sbomer commented Oct 23, 2023

Merging this to unblock follow-up work. @vitek-karas feel free to add more feedback if you have any after the last few fixes, and I'll address it in a new PR.

@sbomer sbomer merged commit c345959 into dotnet:main Oct 23, 2023
62 checks passed
@vitek-karas
Copy link
Member

Looks good - thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 25, 2023
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.

2 participants