Skip to content

Commit

Permalink
Warnings cleanup (#1385)
Browse files Browse the repository at this point in the history 
The most important change is to make warning codes and messages unique - that is for each code there's exactly one message. This means adding quite a few new codes as we were sharing them rather heavily.

We also try to avoid logic when constructing messages (to allow for better localization), so instead of having smaller pieces concatenated we introduce several similar warnings.

This change also adds more tests for some of the warnings.

Infra changes:
* Add the warning code parameter to unrecognized reflection call on the interface
* Add a new ExpectedWarning test attribute which validates the code, the location and optionally parts of the message
* Improved the unrecognized reflection pattern attribute to accept only parts of the message (and to accept more than one)
* Add the ability to validat the warning code for unrecognised reflection patterns
* Refactor source context for annotated values

  Makes the source context required by making it a a .ctor argument for all annotated nodes. Make it 
  IMetadataTokenProvider. Fixes the problem with AnnotatedStringValue which was reported as "unknown" in warning 
  messages.

  This also slightly changes what is stored in the SourceContext - now it's the actual thing (so ParameterDefinition, 
  MethodReturnType and so on) - so printing it out doesn't require knowing which value node it came from. This is cleaner 
  because the SourceContext describes where the value came from, not what the value is (that's the job of the ValueNode). 
  In warnings we typically don't want to print out what the value is since in most cases we don't know the actual value (we 
  just know things about it), so we can rely on SourceContext to simplify some code.

Co-authored-by: Sven Boemer <[email protected]>
  • Loading branch information
@vitek-karas @sbomer
vitek-karas and sbomer authored Aug 13, 2020
1 parent fc5e4aa commit 2b04c06
Show file tree
Hide file tree
Showing 88 changed files with 2,905 additions and 913 deletions.
1,302 changes: 1,219 additions & 83 deletions docs/error-codes.md
View file

Large diffs are not rendered by default.

83 changes: 5 additions & 78 deletions src/linker/Linker.Dataflow/DiagnosticUtilities.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,65 +2,12 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Mono.Cecil;

namespace Mono.Linker.Dataflow
{
static class DiagnosticUtilities
{
internal static string GetDynamicallyAccessedMemberTypesDescription (DynamicallyAccessedMemberTypes memberTypes)
{
if (memberTypes == DynamicallyAccessedMemberTypes.All)
return DynamicallyAccessedMemberTypes.All.ToString ();

var results = new List<DynamicallyAccessedMemberTypes> ();
if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicConstructors))
results.Add (DynamicallyAccessedMemberTypes.NonPublicConstructors);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicConstructors))
results.Add (DynamicallyAccessedMemberTypes.PublicConstructors);
else if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor))
results.Add (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicMethods))
results.Add (DynamicallyAccessedMemberTypes.NonPublicMethods);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicMethods))
results.Add (DynamicallyAccessedMemberTypes.PublicMethods);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicProperties))
results.Add (DynamicallyAccessedMemberTypes.NonPublicProperties);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicProperties))
results.Add (DynamicallyAccessedMemberTypes.PublicProperties);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicFields))
results.Add (DynamicallyAccessedMemberTypes.NonPublicFields);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicFields))
results.Add (DynamicallyAccessedMemberTypes.PublicFields);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicEvents))
results.Add (DynamicallyAccessedMemberTypes.NonPublicEvents);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicEvents))
results.Add (DynamicallyAccessedMemberTypes.PublicEvents);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicNestedTypes))
results.Add (DynamicallyAccessedMemberTypes.NonPublicNestedTypes);

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicNestedTypes))
results.Add (DynamicallyAccessedMemberTypes.PublicNestedTypes);

if (results.Count == 0)
return DynamicallyAccessedMemberTypes.None.ToString ();

return string.Join (" | ", results.Select (r => r.ToString ()));
}

internal static IMetadataTokenProvider GetMethodParameterFromIndex (MethodDefinition method, int parameterIndex)
{
int declaredParameterIndex;
Expand All @@ -78,35 +25,15 @@ internal static IMetadataTokenProvider GetMethodParameterFromIndex (MethodDefini
return null;
}

internal static string GetMetadataTokenDescriptionForErrorMessage (IMetadataTokenProvider targetContext) =>
targetContext switch
{
ParameterDefinition parameterDefinition => GetParameterDescriptionForErrorMessage (parameterDefinition),
MethodReturnType methodReturnType => $"return value of method '{GetMethodSignatureDisplayName (methodReturnType.Method)}'",
FieldDefinition fieldDefinition => $"field '{fieldDefinition}'",
// MethodDefinition is used to represent the "this" parameter as we don't support annotations on the method itself.
MethodDefinition methodDefinition => $"implicit 'this' parameter of method '{methodDefinition.GetDisplayName ()}'",
GenericParameter genericParameter => GetGenericParameterDescriptionForErrorMessage (genericParameter),
_ => $"'{targetContext}'",
};
internal static string GetParameterNameForErrorMessage (ParameterDefinition parameterDefinition) =>
string.IsNullOrEmpty (parameterDefinition.Name) ? $"#{parameterDefinition.Index}" : parameterDefinition.Name;

static string GetParameterDescriptionForErrorMessage (ParameterDefinition parameterDefinition)
{
if (string.IsNullOrEmpty (parameterDefinition.Name))
return $"parameter #{parameterDefinition.Index} of method '{GetMethodSignatureDisplayName (parameterDefinition.Method)}'";
else
return $"parameter '{parameterDefinition.Name}' of method '{GetMethodSignatureDisplayName (parameterDefinition.Method)}'";
}

static string GetGenericParameterDescriptionForErrorMessage (GenericParameter genericParameter)
{
var declaringMemberName = genericParameter.DeclaringMethod != null ?
internal static string GetGenericParameterDeclaringMemberDisplayName (GenericParameter genericParameter) =>
genericParameter.DeclaringMethod != null ?
genericParameter.DeclaringMethod.GetDisplayName () :
genericParameter.DeclaringType.GetDisplayName ();
return $"generic parameter '{genericParameter.Name}' from '{declaringMemberName}'";
}

static string GetMethodSignatureDisplayName (IMethodSignature methodSignature) =>
internal static string GetMethodSignatureDisplayName (IMethodSignature methodSignature) =>
(methodSignature is MethodDefinition method) ? method.GetDisplayName () : methodSignature.ToString ();
}
}
91 changes: 67 additions & 24 deletions src/linker/Linker.Dataflow/FlowAnnotations.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using Mono.Cecil;
using Mono.Cecil.Cil;
Expand Down Expand Up @@ -101,7 +103,7 @@ static bool IsDynamicallyAccessedMembersAttribute (CustomAttribute attribute)
return attributeType.Name == "DynamicallyAccessedMembersAttribute" && attributeType.Namespace == "System.Diagnostics.CodeAnalysis";
}

DynamicallyAccessedMemberTypes GetMemberTypesForDynamicallyAccessedMemberAttribute (ICustomAttributeProvider provider, IMemberDefinition locationMember = null)
DynamicallyAccessedMemberTypes GetMemberTypesForDynamicallyAccessedMembersAttribute (ICustomAttributeProvider provider, IMemberDefinition locationMember = null)
{
if (!_context.CustomAttributes.HasCustomAttributes (provider))
return DynamicallyAccessedMemberTypes.None;
Expand All @@ -110,10 +112,10 @@ DynamicallyAccessedMemberTypes GetMemberTypesForDynamicallyAccessedMemberAttribu
continue;
if (attribute.ConstructorArguments.Count == 1)
return (DynamicallyAccessedMemberTypes) (int) attribute.ConstructorArguments[0].Value;
else if (attribute.ConstructorArguments.Count == 0)
_context.LogWarning ($"DynamicallyAccessedMembersAttribute was specified but no argument was proportioned", 2020, locationMember ?? (provider as IMemberDefinition));
else
_context.LogWarning ($"DynamicallyAccessedMembersAttribute was specified but there is more than one argument", 2022, locationMember ?? (provider as IMemberDefinition));
_context.LogWarning (
$"Attribute 'System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute' doesn't have the required number of parameters specified",
2028, locationMember ?? (provider as IMemberDefinition));
}
return DynamicallyAccessedMemberTypes.None;
}
Expand All @@ -128,7 +130,7 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
if (!IsTypeInterestingForDataflow (field.FieldType))
continue;

DynamicallyAccessedMemberTypes annotation = GetMemberTypesForDynamicallyAccessedMemberAttribute (field);
DynamicallyAccessedMemberTypes annotation = GetMemberTypesForDynamicallyAccessedMembersAttribute (field);
if (annotation == DynamicallyAccessedMemberTypes.None) {
continue;
}
Expand All @@ -148,7 +150,7 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
// IL space assigns index 0 to the `this` parameter on instance methods.


DynamicallyAccessedMemberTypes methodMemberTypes = GetMemberTypesForDynamicallyAccessedMemberAttribute (method);
DynamicallyAccessedMemberTypes methodMemberTypes = GetMemberTypesForDynamicallyAccessedMembersAttribute (method);

int offset;
if (method.HasImplicitThis ()) {
Expand All @@ -161,12 +163,16 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
paramAnnotations[0] = methodMemberTypes;
}
} else if (methodMemberTypes != DynamicallyAccessedMemberTypes.None) {
_context.LogWarning ($"The DynamicallyAccessedMembersAttribute is only allowed on method parameters, return value or generic parameters.", 2041, method);
_context.LogWarning (
$"The 'DynamicallyAccessedMembersAttribute' is not allowed on methods. It is allowed on method return value or method parameters though.",
2041, method, subcategory: MessageSubCategory.TrimAnalysis);
}
} else {
offset = 0;
if (methodMemberTypes != DynamicallyAccessedMemberTypes.None) {
_context.LogWarning ($"The DynamicallyAccessedMembersAttribute is only allowed on method parameters, return value or generic parameters.", 2041, method);
_context.LogWarning (
$"The 'DynamicallyAccessedMembersAttribute' is not allowed on methods. It is allowed on method return value or method parameters though.",
2041, method, subcategory: MessageSubCategory.TrimAnalysis);
}
}

Expand All @@ -175,7 +181,7 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
continue;
}

DynamicallyAccessedMemberTypes pa = GetMemberTypesForDynamicallyAccessedMemberAttribute (method.Parameters[i], method);
DynamicallyAccessedMemberTypes pa = GetMemberTypesForDynamicallyAccessedMembersAttribute (method.Parameters[i], method);
if (pa == DynamicallyAccessedMemberTypes.None) {
continue;
}
Expand All @@ -187,13 +193,13 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
}

DynamicallyAccessedMemberTypes returnAnnotation = IsTypeInterestingForDataflow (method.ReturnType) ?
GetMemberTypesForDynamicallyAccessedMemberAttribute (method.MethodReturnType, method) : DynamicallyAccessedMemberTypes.None;
GetMemberTypesForDynamicallyAccessedMembersAttribute (method.MethodReturnType, method) : DynamicallyAccessedMemberTypes.None;

DynamicallyAccessedMemberTypes[] genericParameterAnnotations = null;
if (method.HasGenericParameters) {
for (int genericParameterIndex = 0; genericParameterIndex < method.GenericParameters.Count; genericParameterIndex++) {
var genericParameter = method.GenericParameters[genericParameterIndex];
var annotation = GetMemberTypesForDynamicallyAccessedMemberAttribute (genericParameter, method);
var annotation = GetMemberTypesForDynamicallyAccessedMembersAttribute (genericParameter, method);
if (annotation != DynamicallyAccessedMemberTypes.None) {
if (genericParameterAnnotations == null)
genericParameterAnnotations = new DynamicallyAccessedMemberTypes[method.GenericParameters.Count];
Expand Down Expand Up @@ -229,7 +235,7 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
continue;
}

DynamicallyAccessedMemberTypes annotation = GetMemberTypesForDynamicallyAccessedMemberAttribute (property);
DynamicallyAccessedMemberTypes annotation = GetMemberTypesForDynamicallyAccessedMembersAttribute (property);
if (annotation == DynamicallyAccessedMemberTypes.None) {
continue;
}
Expand All @@ -250,7 +256,9 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
}

if (annotatedMethods.Any (a => a.Method == setMethod)) {
_context.LogWarning ($"Trying to propagate DynamicallyAccessedMemberAttribute from property '{property.FullName}' to its setter '{setMethod.GetDisplayName ()}', but it already has such attribute on the 'value' parameter.", 2043, setMethod);
_context.LogWarning (
$"'DynamicallyAccessedMembersAttribute' on property '{property.GetDisplayName ()}' conflicts with the same attribute on its accessor '{setMethod.GetDisplayName ()}'.",
2043, setMethod, subcategory: MessageSubCategory.TrimAnalysis);
} else {
int offset = setMethod.HasImplicitThis () ? 1 : 0;
if (setMethod.Parameters.Count > 0) {
Expand All @@ -277,7 +285,8 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
}

if (annotatedMethods.Any (a => a.Method == getMethod)) {
_context.LogWarning ($"Trying to propagate DynamicallyAccessedMemberAttribute from property '{property.FullName}' to its getter '{getMethod.GetDisplayName ()}', but it already has such attribute on the return value.",
_context.LogWarning (
$"'DynamicallyAccessedMembersAttribute' on property '{property.GetDisplayName ()}' conflicts with the same attribute on its accessor '{getMethod.GetDisplayName ()}'.",
2043, getMethod, subcategory: MessageSubCategory.TrimAnalysis);
} else {
annotatedMethods.Add (new MethodAnnotations (getMethod, null, annotation, null));
Expand All @@ -287,16 +296,19 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
FieldDefinition backingField;
if (backingFieldFromGetter != null && backingFieldFromSetter != null &&
backingFieldFromGetter != backingFieldFromSetter) {
_context.LogWarning ($"Could not find a unique backing field for property '{property.FullName}' to propagate DynamicallyAccessedMembersAttribute. The backing fields from getter '{backingFieldFromGetter.FullName}' and setter '{backingFieldFromSetter.FullName}' are not the same.", 2042, property);
_context.LogWarning (
$"Could not find a unique backing field for property '{property.GetDisplayName ()}' to propagate 'DynamicallyAccessedMembersAttribute'.",
2042, property, subcategory: MessageSubCategory.TrimAnalysis);
backingField = null;
} else {
backingField = backingFieldFromGetter ?? backingFieldFromSetter;
}

if (backingField != null) {
if (annotatedFields.Any (a => a.Field == backingField)) {
_context.LogWarning ($"Trying to propagate DynamicallyAccessedMemberAttribute from property '{property.FullName}' to its field '{backingField}', but it already has such attribute.",
2043, backingField, subcategory: MessageSubCategory.TrimAnalysis);
_context.LogWarning (
$"'DynamicallyAccessedMemberAttribute' on property '{property.GetDisplayName ()}' conflicts with the same attribute on its backing field '{backingField.GetDisplayName ()}'.",
2056, backingField, subcategory: MessageSubCategory.TrimAnalysis);
} else {
annotatedFields.Add (new FieldAnnotation (backingField, annotation));
}
Expand All @@ -308,7 +320,7 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
if (type.HasGenericParameters) {
for (int genericParameterIndex = 0; genericParameterIndex < type.GenericParameters.Count; genericParameterIndex++) {
var genericParameter = type.GenericParameters[genericParameterIndex];
var annotation = GetMemberTypesForDynamicallyAccessedMemberAttribute (genericParameter, type);
var annotation = GetMemberTypesForDynamicallyAccessedMembersAttribute (genericParameter, type);
if (annotation != DynamicallyAccessedMemberTypes.None) {
if (typeGenericParameterAnnotations == null)
typeGenericParameterAnnotations = new DynamicallyAccessedMemberTypes[type.GenericParameters.Count];
Expand Down Expand Up @@ -455,12 +467,43 @@ void ValidateMethodGenericParametersHaveNoAnnotations (ref MethodAnnotations met

void LogValidationWarning (IMetadataTokenProvider provider, IMetadataTokenProvider baseProvider, IMemberDefinition origin)
{
_context.LogWarning (
$"DynamicallyAccessedMemberTypes in DynamicallyAccessedMembersAttribute on {DiagnosticUtilities.GetMetadataTokenDescriptionForErrorMessage (provider)} " +
$"don't match overridden {DiagnosticUtilities.GetMetadataTokenDescriptionForErrorMessage (baseProvider)}. " +
$"All overridden members must have the same DynamicallyAccessedMembersAttribute usage.",
2047,
origin);
Debug.Assert (provider.GetType () == baseProvider.GetType ());
Debug.Assert (!(provider is GenericParameter genericParameter) || genericParameter.DeclaringMethod != null);
switch (provider) {
case ParameterDefinition parameterDefinition:
var baseParameterDefinition = (ParameterDefinition) baseProvider;
_context.LogWarning (
$"'DynamicallyAccessedMemberTypes' in 'DynamicallyAccessedMembersAttribute' on the parameter '{DiagnosticUtilities.GetParameterNameForErrorMessage (parameterDefinition)}' of method '{DiagnosticUtilities.GetMethodSignatureDisplayName (parameterDefinition.Method)}' " +
$"don't match overridden parameter '{DiagnosticUtilities.GetParameterNameForErrorMessage (baseParameterDefinition)}' of method '{DiagnosticUtilities.GetMethodSignatureDisplayName (baseParameterDefinition.Method)}'. " +
$"All overridden members must have the same 'DynamicallyAccessedMembersAttribute' usage.",
2092, origin, subcategory: MessageSubCategory.TrimAnalysis);
break;
case MethodReturnType methodReturnType:
_context.LogWarning (
$"'DynamicallyAccessedMemberTypes' in 'DynamicallyAccessedMembersAttribute' on the return value of method '{DiagnosticUtilities.GetMethodSignatureDisplayName (methodReturnType.Method)}' " +
$"don't match overridden return value of method '{DiagnosticUtilities.GetMethodSignatureDisplayName (((MethodReturnType) baseProvider).Method)}'. " +
$"All overridden members must have the same 'DynamicallyAccessedMembersAttribute' usage.",
2093, origin, subcategory: MessageSubCategory.TrimAnalysis);
break;
// No fields - it's not possible to have a virtual field and override it
case MethodDefinition methodDefinition:
_context.LogWarning (
$"'DynamicallyAccessedMemberTypes' in 'DynamicallyAccessedMembersAttribute' on the implicit 'this' parameter of method '{DiagnosticUtilities.GetMethodSignatureDisplayName (methodDefinition)}' " +
$"don't match overridden implicit 'this' parameter of method '{DiagnosticUtilities.GetMethodSignatureDisplayName ((MethodDefinition) baseProvider)}'. " +
$"All overridden members must have the same 'DynamicallyAccessedMembersAttribute' usage.",
2094, origin, subcategory: MessageSubCategory.TrimAnalysis);
break;
case GenericParameter genericParameterOverride:
var genericParameterBase = (GenericParameter) baseProvider;
_context.LogWarning (
$"'DynamicallyAccessedMemberTypes' in 'DynamicallyAccessedMembersAttribute' on the generic parameter '{genericParameterOverride.Name}' of '{DiagnosticUtilities.GetGenericParameterDeclaringMemberDisplayName (genericParameterOverride)}' " +
$"don't match overridden generic parameter '{genericParameterBase.Name}' of '{DiagnosticUtilities.GetGenericParameterDeclaringMemberDisplayName (genericParameterBase)}'. " +
$"All overridden members must have the same 'DynamicallyAccessedMembersAttribute' usage.",
2095, origin, subcategory: MessageSubCategory.TrimAnalysis);
break;
default:
throw new NotImplementedException ($"Unsupported provider type{provider.GetType ()}");
}
}

readonly struct TypeAnnotations
Expand Down
Loading

0 comments on commit 2b04c06

Please sign in to comment.