Unify RequiresUnreferencedCode and RequiresAssemblyFiles attributes

[vitek-karas]

Background and Motivation

Currently the shape of these attributes is like:

class RequiresUnreferencedCodeAttribute : Attribute
{
    public RequiresUnreferencedCodeAttribute(string message);
    public string Message { get; }
    public string Url { get; set; }
}

class RequiresAssemblyFilesAttribute : Attribute
{
    public RequiresAssemblyFilesAttribute();
    public string Message { get; set; }
    public string Url { get; set; }
}

The two attributes are very similar in their usage, they annotate a certain method as problematic when used with trimming or single-file respectively. But their usage is different:

[RequiresUnreferencedCode("This method doesn't play nice with trimming")]
[RequiresAssemblyFiles(Message = "This method doesn't play nice with single-file either")]
void ReallyProblematicMethod() {}

The first attribute makes the Message mandatory since it's a constructor parameter which can't be omitted. The second attribute makes Message optional and doesn't provide a way to specify it in the constructor.

From UX perspective we want people to specify the message if they at all can, since without it the produced warnings are not very actionable. It would also be good for these two attributes to have consistent usage patterns.

Proposed API

We can't really change RequiresUnreferencedCode since that already shipped in .NET 5. But we can adapt RequiresAssemblyFiles to match its behavior:

namespace System.Diagnostics.CodeAnalysis;

class RequiresAssemblyFilesAttribute : Attribute
{
-    public RequiresAssemblyFilesAttribute();
+    public RequiresAssemblyFilesAttribute(string message);
-    public string Message { get; set; }
+    public string Message { get; }
    public string Url { get; set; }
}

If we think that message should be optional, then keep the parameter less constructor as well.

Usage Examples

[RequiresUnreferencedCode("This method doesn't play nice with trimming")]
[RequiresAssemblyFiles("This method doesn't play nice with single-file either")]
void ReallyProblematicMethod() {}

Risks

As proposed it's a breaking change compared to previously released .NET 6 Previews. We don't expect this attribute to be used by many people yet.

There's associated cost in the "breaking" change that we would need to fix all of the occurrences of this attribute in the framework (less than 100) and that the Roslyn analyzer for this attribute will need to react to the change.

[ghost]

Tagging subscribers to this area: @ajcvickers
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and Motivation

Currently the shape of these attributes is like:

class RequiresUnreferencedCodeAttribute : Attribute
{
    public RequiresUnreferencedCodeAttribute(string message);
    public string Message { get; }
    public string Url { get; set; }
}

class RequiresAssemblyFilesAttribute : Attribute
{
    public RequiresAssemblyFilesAttribute();
    public string Message { get; set; }
    public string Url { get; set; }
}

The two attributes are very similar in their usage, they annotate a certain method as problematic when used with trimming or single-file respectively. But their usage is different:

[RequiresUnreferencedCode("This method doesn't play nice with trimming")]
[RequiresAssemblyFiles(Message = "This method doesn't play nice with single-file either")]
void ReallyProblematicMethod() {}

The first attribute makes the Message mandatory since it's a constructor parameter which can't be omitted. The second attribute makes Message optional and doesn't provide a way to specify it in the constructor.

From UX perspective we want people to specify the message if they at all can, since without it the produced warnings are not very actionable. It would also be good for these two attributes to have consistent usage patterns.

Proposed API

We can't really change RequiresUnreferencedCode since that already shipped in .NET 5. But we can adapt RequiresAssemblyFiles to match its behavior:

namespace System.Diagnostics.CodeAnalysis;

class RequiresAssemblyFilesAttribute : Attribute
{
-    public RequiresAssemblyFilesAttribute();
+    public RequiresAssemblyFilesAttribute(string message);
-    public string Message { get; set; }
+    public string Message { get; }
    public string Url { get; set; }
}

Usage Examples

[RequiresUnreferencedCode("This method doesn't play nice with trimming")]
[RequiresAssemblyFiles("This method doesn't play nice with single-file either")]
void ReallyProblematicMethod() {}

Alternative solutions

A non-breaking solution (but a bit more messy would be to just add overloads to RequiresAssemblyFiles to make it possible to be used the same way as RequiresUnreferencedCode, but don't enforce it - so:

namespace System.Diagnostics.CodeAnalysis;

class RequiresAssemblyFilesAttribute : Attribute
{
    public RequiresAssemblyFilesAttribute();
+    public RequiresAssemblyFilesAttribute(string message);
    public string Message { get; set; }
    public string Url { get; set; }
}

Risks

As proposed it's a breaking change compared to previously released .NET 6 Previews. The alternative solution has basically no risk as it's purely additive.

There's associated cost in the "breaking" change that we would need to fix all of the occurences of this attribute in the framework (less than 100) and that the Roslyn analyzer for this attribute will need to react to the change.

Author:	vitek-karas
Assignees:	-
Labels:	api-suggestion, area-System.ComponentModel.DataAnnotations, untriaged
Milestone:	-

[vitek-karas]

@tlakollo @eerhardt @agocke @sbomer

[eerhardt]

Looking across our code base, I only see 2 places that don't set a message, and they are both in a class that derives from Assembly.

runtime/src/libraries/System.Reflection.Context/src/System/Reflection/Context/Delegation/DelegatingAssembly.cs

Lines 76 to 79 in fcd12b8

	[Obsolete]
	[RequiresAssemblyFiles]
	#endif
	public override string? CodeBase

runtime/src/libraries/System.Reflection.Context/src/System/Reflection/Context/Delegation/DelegatingAssembly.cs

Lines 95 to 98 in fcd12b8

	[Obsolete]
	[RequiresAssemblyFiles]
	#endif
	public override string EscapedCodeBase

I agree that we should force a Message here. If a dev needs to apply this attribute to an API, they should give an indication about why this API requires "assembly files" and what callers can do to mitigate it if they need to work in single-file apps.

From the original API review I see:

We shouldn't take a message because they are hard to change

Which I don't fully understand. cc @terrajobst

[ghost]

Tagging subscribers to this area: @agocke, @vitek-karas, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and Motivation

Currently the shape of these attributes is like:

class RequiresUnreferencedCodeAttribute : Attribute
{
    public RequiresUnreferencedCodeAttribute(string message);
    public string Message { get; }
    public string Url { get; set; }
}

class RequiresAssemblyFilesAttribute : Attribute
{
    public RequiresAssemblyFilesAttribute();
    public string Message { get; set; }
    public string Url { get; set; }
}

The two attributes are very similar in their usage, they annotate a certain method as problematic when used with trimming or single-file respectively. But their usage is different:

[RequiresUnreferencedCode("This method doesn't play nice with trimming")]
[RequiresAssemblyFiles(Message = "This method doesn't play nice with single-file either")]
void ReallyProblematicMethod() {}

The first attribute makes the Message mandatory since it's a constructor parameter which can't be omitted. The second attribute makes Message optional and doesn't provide a way to specify it in the constructor.

From UX perspective we want people to specify the message if they at all can, since without it the produced warnings are not very actionable. It would also be good for these two attributes to have consistent usage patterns.

Proposed API

We can't really change RequiresUnreferencedCode since that already shipped in .NET 5. But we can adapt RequiresAssemblyFiles to match its behavior:

namespace System.Diagnostics.CodeAnalysis;

class RequiresAssemblyFilesAttribute : Attribute
{
-    public RequiresAssemblyFilesAttribute();
+    public RequiresAssemblyFilesAttribute(string message);
-    public string Message { get; set; }
+    public string Message { get; }
    public string Url { get; set; }
}

Usage Examples

[RequiresUnreferencedCode("This method doesn't play nice with trimming")]
[RequiresAssemblyFiles("This method doesn't play nice with single-file either")]
void ReallyProblematicMethod() {}

Alternative solutions

A non-breaking solution (but a bit more messy would be to just add overloads to RequiresAssemblyFiles to make it possible to be used the same way as RequiresUnreferencedCode, but don't enforce it - so:

namespace System.Diagnostics.CodeAnalysis;

class RequiresAssemblyFilesAttribute : Attribute
{
    public RequiresAssemblyFilesAttribute();
+    public RequiresAssemblyFilesAttribute(string message);
    public string Message { get; set; }
    public string Url { get; set; }
}

Risks

As proposed it's a breaking change compared to previously released .NET 6 Previews. The alternative solution has basically no risk as it's purely additive.

There's associated cost in the "breaking" change that we would need to fix all of the occurences of this attribute in the framework (less than 100) and that the Roslyn analyzer for this attribute will need to react to the change.

Author:	vitek-karas
Assignees:	-
Labels:	api-ready-for-review, area-Single-File, blocking
Milestone:	6.0.0

[tlakollo]

Explaining the original api review, the premise was that we prefer having a url over having a message. If we have a good url that explains in detail then we don't need the message therefore it was optional. But since url is not always available that was also optional.

[tlakollo]

Although something that wasn't discussed in the original RequiresAssemblyFiles API review was the actual implementation, so far we don't embed any URL information in the RequiresAssemblyFiles attribute and instead, we put a help URL in the analyzer diagnostic descriptor
The way I see it, I would only use the URL field in the attribute to go in-depth on why that specific member RequiresAssemblyFiles vs the description provided in the helpurl

[vitek-karas]

I could be persuaded that message should be optional - in which case we should make it optional on RequiresUnreferencedCode as well (can be done without breaking anything). My main concern is that the attribute are very similar in function, but the usage of them is at least on the surface very different.

[eerhardt]

I definitely don't think RequiresUnreferencedCode should be applied without a message. Applying that basically means "you can never use this API in a trimmed or NativeAOT app. And you are on your own for figuring out why and what to use instead".

the premise was that we prefer having a url over having a message

so far we don't embed any URL information in the RequiresAssemblyFiles attribute and instead, we put a help URL in the analyzer diagnostic descriptor

If we really believe that RequiresAssemblyFiles doesn't need a message because of this, I think that is enough reasoning to explain why the 2 attributes have different shaped APIs:

• RequiresAssemblyFiles has a decent help URL by default, thus a custom message isn't necessary
• RequiresUnreferencedCode does NOT have a decent help URL, because there are so many different reasons it could be applied. Thus it requires a custom message.

In that case, I would take the Alternative solution, but with a slight modification and remove the Message setter:

namespace System.Diagnostics.CodeAnalysis;

class RequiresAssemblyFilesAttribute : Attribute
{
    public RequiresAssemblyFilesAttribute();
+    public RequiresAssemblyFilesAttribute(string message);
-    public string Message { get; set; }
+    public string Message { get; }
    public string Url { get; set; }
}

[vitek-karas]

I wanted to keep the message setter in the alternative solution basically just so that it doesn't break any existing usage... but we could go with your proposal as well.

[eerhardt]

It's new in net6.0, right? We might as well make it "right" from the start.

Having a ctor parameter and a setter is super confusing:

[RequiresAssemblyFiles("A message", Message = "A Different Message")]
void Foo() {}

[vitek-karas]

OK- makes sense. I updated the proposal.

[terrajobst]

• We're happy with the default message most of the time, so we shouldn't remove the parameterless constructor.

namespace System.Diagnostics.CodeAnalysis
{
    public sealed class RequiresAssemblyFilesAttribute : Attribute
    {
        public RequiresAssemblyFilesAttribute();
        public RequiresAssemblyFilesAttribute(string message);
        public string Message { get; }
        public string Url { get; set; }
    }
}