Analyzer warns when Requires... Attribute is on a static constructor #2455
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using System.Text; | ||
| using Microsoft.CodeAnalysis; | ||
|
|
||
| namespace ILLink.RoslynAnalyzer | ||
| { | ||
| static class IMethodSymbolExtensions | ||
| { | ||
| public static bool IsSetter (this IMethodSymbol method) | ||
| { | ||
| return method.MethodKind == MethodKind.PropertySet; | ||
| } | ||
|
|
||
| public static bool IsGetter (this IMethodSymbol method) | ||
| { | ||
| return method.MethodKind == MethodKind.PropertyGet; | ||
| } | ||
| public static bool IsEventMethod (this IMethodSymbol method) | ||
| { | ||
| return method.MethodKind == MethodKind.EventAdd | ||
| || method.MethodKind == MethodKind.EventRaise | ||
| || method.MethodKind == MethodKind.EventRemove; | ||
| } | ||
| private static void PrependGenericParameters(ImmutableArray<ITypeParameterSymbol> genericParameters, System.Text.StringBuilder sb) | ||
| { | ||
| sb.Insert (0, '>').Insert (0, genericParameters[genericParameters.Length - 1]); | ||
| for (int i = genericParameters.Length - 2; i >= 0; i--) | ||
| sb.Insert (0, ',').Insert (0, genericParameters[i]); | ||
|
|
||
| sb.Insert (0, '<'); | ||
| } | ||
|
|
||
| public static string GetDisplayName (this IMethodSymbol method) | ||
|
Contributor
There was a problem hiding this comment. The GetDisplayName method was made to match C# naming which is what Roslyn uses and most people are used to reading instead of IL namings. Therefore if there is a discrepancy between the way it's represented by the analyzer and linker is likely a fix in the GetDisplayName in the linker not an implementation on the analyzer
Contributor
There was a problem hiding this comment. Worst case I think you can append directly the string ".cctor" at the end when displaying the diagnostic
Member
There was a problem hiding this comment. I would expect most cases to already be covered by Roslyn. I could understand that we need to do something special for property getter/setter (and events), and possibly even .cctor, but other than that I would expect to simply call Roslyn to do this for us.
Member
Author
There was a problem hiding this comment. For now, would it be best to use the ISymbolExtensions.GetDisplayName and have separate ExpectedWarning's for the analyzer and trimmer, and then later change the trimmer's GetDisplayName to match the Analyzer/Roslyn?
Member
Author
There was a problem hiding this comment. I added a check in the GetDisplayName in the linker that exists for ctors, but was missing cctors. Now they should look the same.
Contributor
There was a problem hiding this comment. I think I was wrong is not a fix on the linker side, because if you fix in the linker then constructor and static constructor are represented equally, there is no way to differentiate them. So in the cases like the BeforeInitField test, I think things can get confusing. Same for RequiresInCompilerGeneratedCode tests, they get more confusing although you don't see the changes because we don't check for the caller name only messages in the attribute.
Member
There was a problem hiding this comment. .cctors are weird - so we might need to add a special case unfortunately.
Member
Author
There was a problem hiding this comment. I recently pushed a couple commits with different formats for the diagnostic messages. The first one includes the I think the static modifier notation would be best since it still differentiates the constructors and is more familiar to most people.
Contributor
There was a problem hiding this comment. I just created something in SharpLab to see how a warning would be printed This generates I think is key for the trimmer to have some differentiation but I don't have a preference for which one to choose
In general I think I feel more inclined for the first approach |
||
| { | ||
| var sb = new System.Text.StringBuilder (); | ||
|
|
||
| // Match C# syntaxis name if setter or getter | ||
| if (method != null && (method.IsSetter() || method.IsGetter())) { | ||
| // Append property name | ||
| string name = method.IsSetter() ? string.Concat (method.Name, ".set") : string.Concat (method.Name, ".get"); | ||
| sb.Append (name); | ||
| // Insert declaring type name and namespace | ||
| sb.Insert (0, '.').Insert (0, method.ContainingType.GetDisplayName ()); | ||
| return sb.ToString (); | ||
| } | ||
|
|
||
| if (method != null && method.IsEventMethod ()) { | ||
| // Append event name | ||
| string name = method.MethodKind switch { | ||
| MethodKind.EventAdd => string.Concat (method.Name, ".add"), | ||
| MethodKind.EventRemove => string.Concat (method.Name, ".remove"), | ||
| MethodKind.EventRaise => string.Concat (method.Name, ".raise"), | ||
| _ => throw new NotSupportedException (), | ||
| }; | ||
| sb.Append (name); | ||
| // Insert declaring type name and namespace | ||
| sb.Insert (0, '.').Insert (0, method.ContainingType.GetDisplayName ()); | ||
| return sb.ToString (); | ||
| } | ||
|
|
||
| if (method.IsConstructor ()) | ||
| sb.Append (".ctor"); | ||
| else if (method.IsStaticConstructor ()) | ||
| sb.Append (".cctor"); | ||
|
|
||
| // Append parameters | ||
| sb.Append ("("); | ||
| if (method?.Parameters.Length > 0) { | ||
| for (int i = 0; i < method.Parameters.Length - 1; i++) | ||
| sb.Append (method.Parameters[i].GetDisplayName()).Append (','); | ||
|
|
||
| sb.Append (method.Parameters[method.Parameters.Length - 1].GetDisplayName ()); | ||
| } | ||
|
|
||
| sb.Append (")"); | ||
|
|
||
| // Insert generic parameters | ||
| if (method is not null && method.IsGenericMethod) { | ||
| PrependGenericParameters (method.TypeParameters, sb); | ||
| } | ||
|
|
||
| // Insert declaring type name and namespace | ||
| if (method is not null && method.ContainingType != null) | ||
| sb.Insert (0, '.').Insert (0, method.ContainingType.GetDisplayName ()); | ||
|
|
||
| return sb.ToString (); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,12 @@ public override void VisitClassDeclaration (ClassDeclarationSyntax node) | |
| CheckMember (node); | ||
| } | ||
|
|
||
| public override void VisitConstructorDeclaration (ConstructorDeclarationSyntax node) | ||
|
Member
Author
There was a problem hiding this comment. Fix for #2446 to make sure we account for ExpectedWarning on constructors.
Contributor
There was a problem hiding this comment. Can you add a test for this? Something that Inside the static constructor calls a method annotated with RUC and just verify that we can use the ExpectedWarning attribute
Member
Author
There was a problem hiding this comment. Good thing you mentioned this, I think I found a bug that the linker doesn't check in constructors for calls to methods with RUC. The test class for this is
Contributor
There was a problem hiding this comment. I think it's because nothing references that code in the main method, since it's not referenced Trimmer will get rid of that piece of code. Notice that one of the differences between analyzer and trimmer is that analyzer will produce diagnostics even for code that will be trimmed. |
||
| { | ||
| base.VisitConstructorDeclaration (node); | ||
| CheckMember (node); | ||
| } | ||
|
|
||
| public override void VisitInterfaceDeclaration (InterfaceDeclarationSyntax node) | ||
| { | ||
| base.VisitInterfaceDeclaration (node); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.