Add UnconditionalSuppressMessageAttribute #1185

mateoatr · 2020-05-13T07:10:35Z

Related to #1147. This PR contains the first effort to support the UnconditionalSuppressMessageAttribute. It adds the ability to suppress a specific warning in members and types. Tests were added to show its usage.

…error found.

Co-authored-by: Marek Safar <[email protected]>

…SuppressMessageAttribute's copy.

src/linker/Linker/MessageContainer.cs

marek-safar · 2020-05-14T10:13:07Z

src/linker/Linker/MessageOrigin.cs

@@ -39,6 +51,15 @@ public MessageOrigin (string fileName, int sourceLine = 0, int sourceColumn = 0)
 			return null;
 		}

+		internal static MessageOrigin? TryGetOrigin (IMemberDefinition sourceMethod, int ilOffset,


I don't think this is the right place. We want the developer to be able to suppress any warning message (e.g. from 3rd party custom step). I think the whole logic should be close to MessageContainer.CreateWarning which probably will need to be expanded with additional "context" state parameter

Absolutely agree.
The suppression should work without any special code from the place where we create the warning - with the sole exception that we require the "context" parameter. I would even go as far as make some strict rule that the context must be specified unless the code is somehow special (like internal errors, command line parsing errors and so on) - otherwise we should always be able to tell what the warning applies to (assembly or more specific).

I think from the API perspective this change should add the "context" attribute to the CreateWarning method (and maybe also CreateError for consistency). Then we should have another API which is something like IsSuppressed - I would probably put it onto the MessageContainer if it's possible, or if that doesn't work then the LinkContext. It should take the necessary parameters to decide if a warning will be suppressed or not. Notable it should not require the full warning message - the indented usage would be to avoid computing potentially expensive warning message if we know it's going to be suppressed.

I've moved this logic out of MessageOrigin by adding the mentioned IsSuppressed API into the LinkContext, which is called from within CreateWarningMessage. It's now mandatory to specify a MessageOrigin for all created warnings, which must be either a filename or an objetct of a class implementing IMetadataTokenProvider (modules, types and members). The way to create warnings feels a little bit cumbersome though (Context.LogMessage (MessageContainer.CreateWarningMessage (Context, Message, Code, MessageOrigin, [optional] Subcategory))), I think this could be enhanced by adding new methods (LogMessage for info/diags, LogError for errors, LogWarning for warnings) and stop passing around the LinkContext.

marek-safar · 2020-05-14T10:14:13Z

src/linker/Linker.Steps/MarkStep.cs

@@ -1426,6 +1428,8 @@ bool MarkSpecialCustomAttributeDependencies (CustomAttribute ca, ICustomAttribut
 					l.Parameters.Count == 1 && l.Parameters[0].ParameterType.IsTypeOf ("System", "Type"),
 					provider);
 				return true;
+			} else if (dt.Name == "UnconditionalSuppressMessageAttribute" && dt.Namespace == "System.Diagnostics.CodeAnalysis") {
+				_context.Suppressions.AddLocalSuppression (ca, provider.MetadataToken);


Does it really have to be MetadataToken when the attribute cannot be attached to tokens?

All local suppressions can be attached to a a specific md token. For suppressions which cannot be attached to tokens we'll probably be storing a string instead, but I think this case only applies for suppressions that use the target/scope argument. Support for this will be incorporated in a future PR, since the signature parser will be needed for some scenarios (global suppressions made by specifying target/scope).

ok but to do the decoding here is too late as the suppression on metadata tokens would work for steps starting from MarkStep only

…ionalSuppressMessage

src/linker/Linker/MessageContainer.cs

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs

marek-safar · 2020-05-20T19:03:42Z

src/linker/Linker.Steps/MarkStep.cs

@@ -1426,6 +1428,8 @@ bool MarkSpecialCustomAttributeDependencies (CustomAttribute ca, ICustomAttribut
 					l.Parameters.Count == 1 && l.Parameters[0].ParameterType.IsTypeOf ("System", "Type"),
 					provider);
 				return true;
+			} else if (dt.Name == "UnconditionalSuppressMessageAttribute" && dt.Namespace == "System.Diagnostics.CodeAnalysis") {
+				_context.Suppressions.AddLocalSuppression (ca, provider.MetadataToken);


ok but to do the decoding here is too late as the suppression on metadata tokens would work for steps starting from MarkStep only

src/linker/ref/Linker/LinkContext.cs

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs

marek-safar · 2020-05-20T19:21:59Z

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs

+	public class UnconditionalSuppressMessageAttributeState
+	{
+		private readonly AssemblyDefinition _assembly;
+		private GlobalSuppressions _lazyGlobalSuppressions;


Why have a class with single field?

We're just making a clear distinction between the global suppressions and the local ones with this class, just as it's done in Roslyn. We could of course have it all in one class but I don't see a reason to not have this.

I think it'd be more readable to have private readonly Dictionary<MetadataToken, Dictionary<string, SuppressMessageInfo>> _globalSuppressions; here and hidden inside nested class

src/linker/Linker/LinkContext.cs

src/linker/Linker/MessageOrigin.cs

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs

Co-authored-by: Vitek Karas <[email protected]>

test/Mono.Linker.Tests.Cases/Logging/Dependencies/LogStep.cs

mateoatr · 2020-05-21T23:14:18Z

I've removed all things related to global suppressions from this PR. I'll work on them and put them on a following PR to keep this one short and don't delay much in merging the basic support for the UnconditionalSuppressMessage attribute. The code introduced here gives basic capabilities for suppressing warnings in local scopes (members and types).

Enable nullable ref types

vitek-karas · 2020-05-22T09:23:53Z

src/linker/Linker.Steps/MarkStep.cs

@@ -1012,6 +1015,8 @@ protected void MarkAssembly (AssemblyDefinition assembly)

 			foreach (ModuleDefinition module in assembly.Modules)
 				LazyMarkCustomAttributes (module, module);
+
+			_context.Suppressions = new UnconditionalSuppressMessageAttributeState (assembly);


This is probably not right. The _context is global - for all assemblies, and here we're assigning a state which is for one assembly only. Also - what if there already is another state in that variable? MarkStep doesn't work one assembly at a time - it jumps randomly between assemblies.

We either need to store the suppressions per-assembly or have some kind of global storage keyed of the metadata token providers.

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs

test/Mono.Linker.Tests.Cases/WarningSuppression/WarningMessageSuppression.cs

Co-authored-by: Vitek Karas <[email protected]>

This changes the behavior of AddLocalSuppression to add or update a (provider, id) tuple with the last seen suppression instead of keeping the first one seen.

src/linker/Linker/LinkContext.cs

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs

vitek-karas · 2020-05-22T22:23:26Z

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs

-				!Regex.IsMatch (warningId, "^IL\\d{4}")) {
+				warningId.Length < 6 ||
+				!warningId.StartsWith ("IL") ||
+				!int.TryParse (warningId.Substring (2, 4), out info.Id)) {


This means that "IL1234HiThere" will be read as 1234. Probably not what we want. Just check that Length != 6.

SuppressMessageAttribute allows the user to write anything after a colon, mainly for documentation purposes (e.g., "IL1234:Suppress this"). I think we should keep this capability.

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs

src/linker/Linker/MessageOrigin.cs

Check for null before calling `ToLower` on `Scope` parameter Use IMemberDefinition instead of IMdToken for specifying the MessageOrigin

Co-authored-by: Marek Safar <[email protected]> Co-authored-by: Vitek Karas <[email protected]> Commit migrated from dotnet/linker@d51be36

Mateo Torres Ruiz and others added 13 commits April 30, 2020 16:27

Log errors via MessageContainer

1f70684

Use LinkerErrorExceptions for all throws. Don't stop linker on first …

3ee5a98

…error found.

Move FoundErrors to LinkContext and clean exceptions.

018b132

Move errors to new md file.

15b93c1

Use same code for errors with same message.

37ba56d

Fix test and whitespace formatting

a0998e5

Update exit message

1ac3a2d

Co-authored-by: Marek Safar <[email protected]>

Add error for unexpected scenarios.

5ab700b

Print all inner exceptions.

5ec1844

Resolve merge conflicts

cf4b5af

Update FeatureSubstitutionsInvalid test.

e9bc9dc

Add UnconditionalSuppressMessage attribute.

20c4eea

Merge with master

6dca600

mateoatr added this to the .NET5.0 milestone May 13, 2020

mateoatr requested a review from vitek-karas May 13, 2020 07:10

mateoatr requested a review from marek-safar as a code owner May 13, 2020 07:10

mateoatr self-assigned this May 13, 2020

Make MessageOrigin overload internal. Allow nullable on Unconditional…

7c18a45

…SuppressMessageAttribute's copy.

marek-safar reviewed May 14, 2020

View reviewed changes

Mateo Torres Ruiz added 3 commits May 15, 2020 09:38

Merge branch 'master' of https://github.com/mono/linker into Uncondit…

802e614

…ionalSuppressMessage

Make MessageOrigin mandatory for warnings

af143b7

Add CreateWarningMessage overloads to make it cleaner

0de4956

mateoatr requested a review from marek-safar May 20, 2020 17:42

Mateo Torres Ruiz added 2 commits May 20, 2020 11:54

Merge branch 'master' of https://github.com/mono/linker into Uncondit…

297e925

…ionalSuppressMessage

Merge with master

7ba1784

marek-safar requested changes May 20, 2020

View reviewed changes

Address feedback

2bf9add

vitek-karas reviewed May 21, 2020

View reviewed changes

Apply suggestions from code review

5a9f126

Co-authored-by: Vitek Karas <[email protected]>

marek-safar reviewed May 21, 2020

View reviewed changes

test/Mono.Linker.Tests.Cases/Logging/Dependencies/LogStep.cs Outdated Show resolved Hide resolved

Remove unnecessary castings.

ee00fb5

Mateo Torres Ruiz added 2 commits May 21, 2020 16:46

Fix whitespace formatting

db89657

Enable nullable ref types

Fix mono build

17910cb

vitek-karas requested changes May 22, 2020

View reviewed changes

mateoatr and others added 4 commits May 22, 2020 09:47

Update UnconditionalSuppressMessageAttributeState.cs with suggestion

e97b422

Co-authored-by: Vitek Karas <[email protected]>

Address feedback

6e387e3

Remove AddOrUpdate

02c5196

TryAdd is only supported in .NET Core

f90d38b

This changes the behavior of AddLocalSuppression to add or update a (provider, id) tuple with the last seen suppression instead of keeping the first one seen.

mateoatr requested review from vitek-karas and marek-safar May 22, 2020 18:42

vitek-karas reviewed May 22, 2020

View reviewed changes

src/linker/Linker/LinkContext.cs Outdated Show resolved Hide resolved

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs Show resolved Hide resolved

Store only the integer value of the warning id

baa1c77

vitek-karas reviewed May 22, 2020

View reviewed changes

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs Outdated Show resolved Hide resolved

vitek-karas reviewed May 22, 2020

View reviewed changes

src/linker/Linker/UnconditionalSuppressMessageAttributeState.cs Outdated Show resolved Hide resolved

Mateo Torres Ruiz added 3 commits May 22, 2020 15:05

Don't use regex for simple parsing

7997b93

Check that warning id starts with 'IL'

0492a0f

Missed '!'

c4b0483

vitek-karas reviewed May 22, 2020

View reviewed changes

Check if first character after warning code is a colon

87e79fc

vitek-karas approved these changes May 27, 2020

View reviewed changes

marek-safar requested changes May 28, 2020

View reviewed changes

Mateo Torres Ruiz added 3 commits May 28, 2020 16:12

Use ICustomAttributeProvider as key for local suppressions

c4ad9ad

Check for null before calling `ToLower` on `Scope` parameter Use IMemberDefinition instead of IMdToken for specifying the MessageOrigin

Merge with master

519ea2c

Whitespace formatting

c5d8b2e

marek-safar approved these changes May 29, 2020

View reviewed changes

marek-safar merged commit d51be36 into dotnet:master May 29, 2020

mateoatr mentioned this pull request Jun 2, 2020

Add more warning suppression scenarios #1232

Merged

vitek-karas mentioned this pull request Jun 8, 2020

Respect the new UnconditionalSuppressMessageAttribute #1147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UnconditionalSuppressMessageAttribute #1185

Add UnconditionalSuppressMessageAttribute #1185

mateoatr commented May 13, 2020 •

edited

Loading

marek-safar May 14, 2020

vitek-karas May 18, 2020

mateoatr May 20, 2020

marek-safar May 14, 2020

mateoatr May 20, 2020

marek-safar May 20, 2020

marek-safar May 20, 2020

marek-safar May 20, 2020

mateoatr May 21, 2020

marek-safar May 21, 2020 •

edited

Loading

mateoatr commented May 21, 2020

vitek-karas May 22, 2020

vitek-karas May 22, 2020

mateoatr May 22, 2020 •

edited

Loading

Add UnconditionalSuppressMessageAttribute #1185

Add UnconditionalSuppressMessageAttribute #1185

Conversation

mateoatr commented May 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marek-safar May 21, 2020 • edited Loading

Choose a reason for hiding this comment

mateoatr commented May 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateoatr May 22, 2020 • edited Loading

Choose a reason for hiding this comment

mateoatr commented May 13, 2020 •

edited

Loading

marek-safar May 21, 2020 •

edited

Loading

mateoatr May 22, 2020 •

edited

Loading