-
Notifications
You must be signed in to change notification settings - Fork 126
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
Ruc on type analyzer part 1 #2330
Ruc on type analyzer part 1 #2330
Conversation
string name = methodDefinition.SemanticsAttributes switch { | ||
MethodSemanticsAttributes.AddOn => string.Concat(methodDefinition.Name.AsSpan (4), ".add"), | ||
MethodSemanticsAttributes.RemoveOn => string.Concat (methodDefinition.Name.AsSpan (7), ".remove"), | ||
MethodSemanticsAttributes.Fire => string.Concat (methodDefinition.Name.AsSpan (6), ".raise"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure if the AsSpan should be 6 (for raise) or 5 (for fire) but seems like documentation mentions more raise than fire
Can you rebase this on |
Add flag to check if methodSymbol is contructor
Delete the diagnostic target class and just warn if the class has the attribute on the class Add support for event raise usage in the analyzer Rebase to 6.0.2xx
ce49436
to
a102cb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be cleaner if you defined a notion of "semantically has RequiresAttribute", like the following:
// TODO: Consider sharing method with linker
/// <summary>
/// True if the source of a call is considered to be annotated with the Requires... attribute
/// </summary>
protected static bool CallerHasRequiresAttribute(ISymbol member)
{
if (member.HasAttribute(RequiresAttributeName) ||
member.ContainingType.HasAttribute(RequiresAttributeName))
{
return true;
}
// Check also for RequiresAttribute in the associated symbol
if (containingSymbol is IMethodSymbol { AssociatedSymbol: {} associated } && associated.HasAttribute (RequiresAttributeName))
return true;
return false;
}
// TODO: Consider sharing method with linker
/// <summary>
/// True if the target of a call is considered to be annotated with the Requires... attribute
/// </summary>
protected static bool TargetHasRequiresAttribute(ISymbol member, [NotNullWhen (returnValue: true)] out AttributeData? requiresAttribute)
{
if (TryGetRequiresAttribute(member, out requiresAttribute))
{
return true;
}
// Also check the containing type
if ((member.IsStatic && !member.IsStaticConstructor ())
{
return TryGetRequiresAttribute(member.ContainingType, out requiresAttribute);
}
requiresAttribute = null;
return false;
}
I agree with @agocke's suggestion - and I think they should match DoesMethodRequireUnreferencedCode/IsMethodInRequiresUnreferencedCodeScope in the linker. |
Fix event test OnEventMethod.EventToTestRemove and OnEventMethod.EventToTestAdd
// If the annotated member is an event accessor, we warn on the event to match the linker behavior. | ||
if (member is IMethodSymbol eventAccessorMethod && eventAccessorMethod.AssociatedSymbol is IEventSymbol eventSymbol) { | ||
member = eventAccessorMethod.ContainingSymbol; | ||
operationContext.ReportDiagnostic (Diagnostic.Create (RequiresDiagnosticRule, | ||
eventSymbol.Locations[0], eventAccessorMethod.GetDisplayName (), GetMessageFromAttribute (requiresAttribute), GetUrlFromAttribute (requiresAttribute))); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After fixing the event test that wasn't exercising calling the annotated add and remove event I realized that the linker warns on the event always (I think wrongfully) and on the caller if it's using an event accessor so I think it's better if I delete this logic and just let the analyzer warn on the caller when tries to use the event accesor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
End up deleting this logic
… RucOnTypeAnalayzer
…ler of the event accessor
@@ -744,24 +744,26 @@ public static void Test () | |||
|
|||
class OnEventMethod | |||
{ | |||
[ExpectedWarning ("IL2026", "--EventToTestRemove.remove--")] | |||
[ExpectedWarning ("IL2026", "--EventToTestRemove.remove--", ProducedBy = ProducedBy.Trimmer)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a special code handling to print the warnings on the event instead of the caller that used the event accessor. I realized that the test OnEventMethod was wrong.
EventToTestRemove had the Requires attribute in its remove accessor, but it was adding an event
EventToTestRemove += (sender, e) => { };
Therefore there was no warning in the caller because only the adding was being called. After realizing that Linker prints in both the caller if an accessor with Requires is used and on the event unconditionally (which I believe is a bug) I decided to remove the special casing in the analyzer.
This expected warning is in the event, so it's no longer produced by the analyzer and instead there are expected warnings in the method that calls the event accessors
[ExpectedWarning ("IL2026", "--EventToTestRemove.remove--")]
[ExpectedWarning ("IL2026", "--EventToTestAdd.add--")]
public static void Test ()
{
EventToTestRemove -= (sender, e) => { };
EventToTestAdd += (sender, e) => { };
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a special code handling to print the warnings on the event instead of the caller that used the event accessor.
This doesn't really make sense to me. Is it possible there's a bug? Is there a comment on the linker side that explains why this is different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is this issue in the linker
https://github.com/mono/linker/issues/2218
@@ -1367,20 +1370,22 @@ static void TestDAMAccess () | |||
typeof (DerivedWithRequires).RequiresPublicFields (); | |||
} | |||
|
|||
[ExpectedWarning ("IL2026", "WithRequires.StaticField", ProducedBy = ProducedBy.Trimmer)] | |||
[ExpectedWarning ("IL2026", "WithRequires.StaticField")] | |||
// Analyzer does not recognize the binding flags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we note the remaining pieces that won't be finished in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean like adding:
// This attribute doesn't work because the analyzer still doesn't understand the binding flags
// [ExpectedWarning ("IL2026", "WithRequires.PrivateField", ProducedBy = ProducedBy.Analyzer)]
// This test needs support for DAM Analyzer
// [ExpectedWarning ("IL2026", "SomeDAMTest", ProducedBy = ProducedBy.Analyzer)]
// This test uses interfaces and inheritance which is still not supported by the analyzer
// [ExpectedWarning ("IL2026, "SomeInterfaceTest", ProducedBy = ProducedBy.Analyzer)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant in the PR summary, just listing the stuff that doesn't work yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks!
Add logic to check some of the Ruc on type analysis warnings Change field and event signature to match analyzer and linker to a single signature mode Add class as a diagnostic target Fix issue where attributes ProducedBy.Trimmer/ProducedBy.All are skipped if they are surrounded by ProducedBy.Analyzer attributes Bring IsConstructor and IsStaticConstructor from Roslyn source Remove special case for printing warnings in the event instead of the caller of the event accessor Scenarios of Ruc on type that are not addressed by this PR: Dealing with interfaces with RUC and having a class implementing it DAM interaction with RUC Reflection patterns, some of the tests work because they use typeof() but in general, they are not supported Commit migrated from dotnet/linker@c702d03
Add logic to check some of the Ruc on type analysis warnings
Change field and event signature to match analyzer and linker to a single signature mode
Add class as a diagnostic target
Scenarios of Ruc on type that are not addressed by this PR:
Dealing with interfaces with RUC and having a class implementing it
DAM interaction with RUC
Reflection patterns, some of the tests work because they use typeof() but in general, they are not supported
Customer Impact
Analyzer now reports more warnings due to presence of
RequiresUnreferencedCode
on types. The warnings would otherwise be only generated by the trimmer during publish.Small improvements to warning messages in the trimmer.
Testing
Added targeted tests
Risk
Low for analyzer as this only adds handling on new cases. Very low from trimmer since it only modifies warning messages.