Skip to content

Replace linker suppressions with attributes #359

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

Merged
merged 3 commits into from
Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions CommunityToolkit.Mvvm/ComponentModel/ObservableValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -545,9 +545,9 @@ IEnumerable<ValidationResult> GetAllErrors()
"Additionally, due to the usage of validation APIs, the type of the current instance cannot be statically discovered.")]
protected void ValidateAllProperties()
{
#pragma warning disable IL2026
// Fast path that tries to create a delegate from a generated type-specific method. This
// is used to make this method more AOT-friendly and faster, as there is no dynamic code.
[RequiresUnreferencedCode("The type of the current instance cannot be statically discovered.")]
static Action<object> GetValidationAction(Type type)
{
if (type.Assembly.GetType("CommunityToolkit.Mvvm.ComponentModel.__Internals.__ObservableValidatorExtensions") is Type extensionsType &&
Expand All @@ -558,7 +558,6 @@ static Action<object> GetValidationAction(Type type)

return GetValidationActionFallback(type);
}
#pragma warning restore IL2026

// Fallback method to create the delegate with a compiled LINQ expression
static Action<object> GetValidationActionFallback(Type type)
Expand Down Expand Up @@ -615,7 +614,9 @@ from property in validatableProperties

// Get or compute the cached list of properties to validate. Here we're using a static lambda to ensure the
// delegate is cached by the C# compiler, see the related issue at https://github.com/dotnet/roslyn/issues/5835.
EntityValidatorMap.GetValue(GetType(), static t => GetValidationAction(t))(this);
EntityValidatorMap.GetValue(
GetType(),
[RequiresUnreferencedCode("The type of the current instance cannot be statically discovered.")] static (t) => GetValidationAction(t))(this);
Comment on lines 615 to +619

Choose a reason for hiding this comment

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

You can now just write GetValidationAction. The linked Roslyn issue has been closed.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is closed but the fix is slated for the C# 11 timeslot, I don't think it's there in VS stable just yet, meaning our CI will be using an older version of Roslyn too without that. I would keep that as is for now (especially given it's unrelated from this PR anyway), and we can update this later on when we bump the SDK as well 🙂

}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using System.ComponentModel;
using System.Diagnostics.CodeAnalysis;

namespace CommunityToolkit.Mvvm.ComponentModel.__Internals;

Expand All @@ -23,10 +24,12 @@ public static class __ObservableValidatorHelper
/// <param name="propertyName">The name of the property to validate.</param>
[EditorBrowsable(EditorBrowsableState.Never)]
[Obsolete("This method is not intended to be called directly by user code")]
[UnconditionalSuppressMessage(
"ReflectionAnalysis",
"IL2026:RequiresUnreferencedCode",
Justification = "This helper is called by generated code from public APIs that have the proper annotations already (and we don't want generated code to produce warnings that developers cannot fix).")]
public static void ValidateProperty(ObservableValidator instance, object? value, string propertyName)
{
#pragma warning disable IL2026
instance.ValidateProperty(value, propertyName);
#pragma warning restore IL2026
}
}
10 changes: 4 additions & 6 deletions CommunityToolkit.Mvvm/Messaging/IMessengerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,10 @@ public static void RegisterAll(this IMessenger messenger, object recipient)
ArgumentNullException.ThrowIfNull(messenger);
ArgumentNullException.ThrowIfNull(recipient);

#pragma warning disable IL2026
// We use this method as a callback for the conditional weak table, which will handle
// thread-safety for us. This first callback will try to find a generated method for the
// target recipient type, and just invoke it to get the delegate to cache and use later.
[RequiresUnreferencedCode("The type of the current instance cannot be statically discovered.")]
static Action<IMessenger, object>? LoadRegistrationMethodsForType(Type recipientType)
{
if (recipientType.Assembly.GetType("CommunityToolkit.Mvvm.Messaging.__Internals.__IMessengerExtensions") is Type extensionsType &&
Expand All @@ -109,12 +109,11 @@ public static void RegisterAll(this IMessenger messenger, object recipient)

return null;
}
#pragma warning restore IL2026

// Try to get the cached delegate, if the generator has run correctly
Action<IMessenger, object>? registrationAction = DiscoveredRecipients.RegistrationMethods.GetValue(
recipient.GetType(),
static t => LoadRegistrationMethodsForType(t));
[RequiresUnreferencedCode("The type of the current instance cannot be statically discovered.")] static (t) => LoadRegistrationMethodsForType(t));

if (registrationAction is not null)
{
Expand Down Expand Up @@ -152,11 +151,11 @@ public static void RegisterAll<TToken>(this IMessenger messenger, object recipie
ArgumentNullException.ThrowIfNull(recipient);
ArgumentNullException.For<TToken>.ThrowIfNull(token);

#pragma warning disable IL2026
// We use this method as a callback for the conditional weak table, which will handle
// thread-safety for us. This first callback will try to find a generated method for the
// target recipient type, and just invoke it to get the delegate to cache and use later.
// In this case we also need to create a generic instantiation of the target method first.
[RequiresUnreferencedCode("The type of the current instance cannot be statically discovered.")]
static Action<IMessenger, object, TToken> LoadRegistrationMethodsForType(Type recipientType)
{
if (recipientType.Assembly.GetType("CommunityToolkit.Mvvm.Messaging.__Internals.__IMessengerExtensions") is Type extensionsType &&
Expand Down Expand Up @@ -224,7 +223,6 @@ from registrationMethod in registrationMethods

return Expression.Lambda<Action<IMessenger, object, TToken>>(body, arg0, arg1, arg2).Compile();
}
#pragma warning restore IL2026

// Get or compute the registration method for the current recipient type.
// As in CommunityToolkit.Diagnostics.TypeExtensions.ToTypeString, we use a lambda
Expand All @@ -233,7 +231,7 @@ from registrationMethod in registrationMethods
// For more info on this, see the related issue at https://github.com/dotnet/roslyn/issues/5835.
Action<IMessenger, object, TToken> registrationAction = DiscoveredRecipients<TToken>.RegistrationMethods.GetValue(
recipient.GetType(),
static t => LoadRegistrationMethodsForType(t));
[RequiresUnreferencedCode("The type of the current instance cannot be statically discovered.")] static (t) => LoadRegistrationMethodsForType(t));

// Invoke the cached delegate to actually execute the message registration
registrationAction(messenger, recipient, token);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#if !NET6_0_OR_GREATER

namespace System.Diagnostics.CodeAnalysis;

/// <summary>
/// Suppresses reporting of a specific rule violation, allowing multiple suppressions on a
/// single code artifact.
/// </summary>
/// <remarks>
/// <see cref="UnconditionalSuppressMessageAttribute"/> is different than
/// <see cref="SuppressMessageAttribute"/> in that it doesn't have a
/// <see cref="ConditionalAttribute"/>. So it is always preserved in the compiled assembly.
/// </remarks>
[AttributeUsage(AttributeTargets.All, Inherited = false, AllowMultiple = true)]
[Conditional("DEBUG")]
internal sealed class UnconditionalSuppressMessageAttribute : Attribute
{
/// <summary>
/// Initializes a new instance of the <see cref="UnconditionalSuppressMessageAttribute"/>
/// class, specifying the category of the tool and the identifier for an analysis rule.
/// </summary>
/// <param name="category">The category for the attribute.</param>
/// <param name="checkId">The identifier of the analysis rule the attribute applies to.</param>
public UnconditionalSuppressMessageAttribute(string category, string checkId)
{
Category = category;
CheckId = checkId;
}

/// <summary>
/// Gets the category identifying the classification of the attribute.
/// </summary>
/// <remarks>
/// The <see cref="Category"/> property describes the tool or tool analysis category
/// for which a message suppression attribute applies.
/// </remarks>
public string Category { get; }

/// <summary>
/// Gets the identifier of the analysis tool rule to be suppressed.
/// </summary>
/// <remarks>
/// Concatenated together, the <see cref="Category"/> and <see cref="CheckId"/>
/// properties form a unique check identifier.
/// </remarks>
public string CheckId { get; }

/// <summary>
/// Gets or sets the scope of the code that is relevant for the attribute.
/// </summary>
/// <remarks>
/// The Scope property is an optional argument that specifies the metadata scope for which
/// the attribute is relevant.
/// </remarks>
public string? Scope { get; set; }

/// <summary>
/// Gets or sets a fully qualified path that represents the target of the attribute.
/// </summary>
/// <remarks>
/// The <see cref="Target"/> property is an optional argument identifying the analysis target
/// of the attribute. An example value is "System.IO.Stream.ctor():System.Void".
/// Because it is fully qualified, it can be long, particularly for targets such as parameters.
/// The analysis tool user interface should be capable of automatically formatting the parameter.
/// </remarks>
public string? Target { get; set; }

/// <summary>
/// Gets or sets an optional argument expanding on exclusion criteria.
/// </summary>
/// <remarks>
/// The <see cref="MessageId "/> property is an optional argument that specifies additional
/// exclusion where the literal metadata target is not sufficiently precise. For example,
/// the <see cref="UnconditionalSuppressMessageAttribute"/> cannot be applied within a method,
/// and it may be desirable to suppress a violation against a statement in the method that will
/// give a rule violation, but not against all statements in the method.
/// </remarks>
public string? MessageId { get; set; }

/// <summary>
/// Gets or sets the justification for suppressing the code analysis message.
/// </summary>
public string? Justification { get; set; }
}

#endif