Skip to content
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

Respect the new UnconditionalSuppressMessageAttribute #1147

Closed
eerhardt opened this issue Apr 27, 2020 · 6 comments
Closed

Respect the new UnconditionalSuppressMessageAttribute #1147

eerhardt opened this issue Apr 27, 2020 · 6 comments
Assignees
Labels
Milestone

Comments

@eerhardt
Copy link
Member

When code wants to suppress a linker warning, for example when the author is certain it is safe, they can add an UnconditionalSuppressMessageAttribute. We should support this new attribute in the linker.

See dotnet/runtime#35339 for the discussion.

@marek-safar marek-safar added this to the .NET5.0 milestone Apr 27, 2020
@mateoatr mateoatr self-assigned this May 5, 2020
@vitek-karas
Copy link
Member

There are several aspects of this which can be done in stages:

  • Basic support of the attribute on a method - suppresses specified warning inside the method's body.
  • Implement support for MessageId - adds the ability to direct the suppression to a specific symbol in the method's body. For example if the linker raises a warning due to a call to Type.GetField then it should be possible to direct the suppression to just that by specifying MessageId with a value of GetField (or maybe System.Type.GetField, or so on - needs a detailed design)
  • Support the attribute on more scopes - type, module, assembly and so on. This will be performance sensitive work
  • Support targeting the attribute - that is support for the Scope and Target properties. Note that this requires a parser of the string in the Target property. We need this to be aligned with the same functionality in roslyn, both the parser and the semantics. The parser for the Target property can be found here.

Performance considerations: When a warning is suppressed it should be fast - linker should not spend my time finding and suppressing the warning. This is expected to be a relatively common case (likely used inside our frameworks so all apps are affected). This means that finding and matching the suppression attributes must be fast and when a warning is suppressed linker should not spend any time computing the details of the warning (specifically constructing the warning message).

@vitek-karas
Copy link
Member

/cc @sbomer - the Target property processing is similar problem to the target of the DynamicDependnecyAttribute in #1148. If we decide to use the same format we should share the implementation of the parser and the semantics of looking it up in the cecil OM.

@MichalStrehovsky
Copy link
Member

the Target property processing is similar problem to the target of the DynamicDependnecyAttribute in #1148

Note that the format of the Target for this attribute is already invented. UnconditionalSuppressMessageAttribute is supposed to work the same way as SuppressMessageAttribute and that one already defines how Target is supposed to look like.

@MichalStrehovsky
Copy link
Member

We might be able to steal the parser for Target from Roslyn. Roslyn team typically spends a lot of time thinking about corner cases, so it might be beneficial to at least look at it, if not fork it into our repo.

@sbomer
Copy link
Member

sbomer commented May 8, 2020

@eerhardt pointed me to some folks who have more context on the Target - it apparently supports the format documented at https://github.com/dotnet/csharplang/blob/master/spec/documentation-comments.md#id-string-format, as well as a legacy FxCop format for back-compat.

@vitek-karas
Copy link
Member

This should be done with #1185 and #1232. It's a not full support for all special cases, but it should be enough for the current needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants