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

[API Proposal]: Add Message property to ExperimentalAttribute #107963

Closed
jeffhandley opened this issue Sep 18, 2024 · 8 comments · Fixed by #108216
Closed

[API Proposal]: Add Message property to ExperimentalAttribute #107963

jeffhandley opened this issue Sep 18, 2024 · 8 comments · Fixed by #108216
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@jeffhandley
Copy link
Member

Background and motivation

With our recent adoptions of the [Experimental] attribute, we found it would have been valuable to add a custom message for the experimental APIs to add context for why the API is marked as experimental. The desired experience would be similar to that of [Obsolete] where a custom message can be provided in addition to the custom diagnostic ID.

API Proposal

  [System.AttributeUsageAttribute(System.AttributeTargets.Assembly | System.AttributeTargets.Class |
   System.AttributeTargets.Constructor | System.AttributeTargets.Delegate | System.AttributeTargets.Enum |
   System.AttributeTargets.Event | System.AttributeTargets.Field | System.AttributeTargets.Interface |
   System.AttributeTargets.Method | System.AttributeTargets.Module | System.AttributeTargets.Property |
   System.AttributeTargets.Struct, Inherited=false)]
  public sealed partial class ExperimentalAttribute : System.Attribute
  {
      public ExperimentalAttribute(string diagnosticId) { }
+     public ExperimentalAttribute(string diagnosticId, string? message) { }
      public string DiagnosticId { get { throw null; } }
+     public string? Message { get { throw null; } }
      public string? UrlFormat { get { throw null; } set { } }
  }

API Usage

namespace System.Runtime.Intrinsics;

public abstract partial class X86Base
{
    [Experimental("SYSLIB5004",  "X86Base.DivRem is experimental since performance is not as optimized as T.DivRem",
                  UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
    public static (uint Quotient, uint Remainder) DivRem(uint lower, uint upper, uint divisor) { throw null; }
    {
    }
}

Alternative Designs

Create a new mechanism for registering the messages. Or keep the status quo and rely on the linked documentation to provide fuller context.

Risks

We will need to follow the model we applied with ObsoleteAttribute where the compiler respects whichever ExperimentalAttribute is defined for the assembly using it, where that type might have Message or might not. When applying this attribute down-level to targets before Message was introduced, libraries can opt to include an internal copy of the class that includes the property so the message can be applied.

We will need to discuss the format of the compiler message, localization, and determine if the default message should be emitted in addition to the custom message, or if only the custom message would be emitted. We should be able to follow the [Obsolete] attribute behavior.

@jeffhandley jeffhandley added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime labels Sep 18, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Sep 18, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

@jeffhandley
Copy link
Member Author

/cc @333fred @jaredpar

Spawned from #107905 (comment) by @stephentoub

@jeffhandley jeffhandley added this to the Future milestone Sep 18, 2024
@jeffhandley jeffhandley added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Sep 18, 2024
@terrajobst
Copy link
Contributor

terrajobst commented Sep 19, 2024

When I designed the attribute I considered that but ultimately decided against it:

  • Obsolete.Message was originally the only way to add context. We added DiagnosticId and UrlFormat later
  • You can't easily change the message, especially for APIs shipped in the ref set
  • You can't localize the message
  • The message length is practically very limited

I'd say if you want to provide additional context, use DiagnosticId and UrlFormat. Whatever this points to can be updated, include sample code, etc.

@jeffhandley
Copy link
Member Author

Thanks for sharing that background, @terrajobst. I can acknowledge all those points and I agree it's an option to keep the status quo and rely on the linked documentation to provide fuller context.

With the [Experimental] attributes we've applied so far, I instinctively expected to add a Message to the attribute as it would be visible in IntelliSense rather than requiring docs to be opened and read. For the DivRem example, it would be nice for message visible inside the IDE to reference the alternate API that isn't experimental. I kind of want to argue that the message from Roslyn is "too scary," but it's probably sufficiently scary.

'{0}' is for evaluation purposes only and is subject to change or removal in future updates.

I think it's a decision of whether we want to provide API-specific, non-localizable, non-serviceable, short messages in IntelliSense, or if we want users to always have to click the link and read the localizable, serviceable, detailed docs.

@terrajobst
Copy link
Contributor

terrajobst commented Sep 24, 2024

Video

  • We decided against the constructor as optional arguments are usually set via properties
  • The shape of [Obsolete] is already subtly different, the constructor might be confusing
  • We probably wouldn't use it most of the time, but we shouldn't block it for easy cases / third parties
  • This requires work in the C#/VB compilers to support message and output it if present
namespace System;

public partial class ExperimentalAttribute
{
    public string? Message { get; set; }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 24, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Sep 24, 2024
@jodydonetti
Copy link
Contributor

jodydonetti commented Oct 19, 2024

As the author of the original proposal from 5 years ago (which already contained the Message property) I'd like to chip in with my 2 cents.

I understand and agree with having an external url that allows us to support full blown localization plus the ability, in the online resource, to have longer explanations, rich examples, extra info and things like images/videos.

At the same time though, from a pragmatic perspective, we should also consider other things like:

  • IMMEDIACY: if a user needs to click to a link every single time just to quickly understand what is going on and have a minimum of context, it takes away from the developer experience, slowing down the discovery process
  • OFFLINE: in case of offline access, a user would not know anything at all about why something is experimental
  • LINGUA FRANCA: although it is nice to be able to have a localized experience for users, we should also acknowledge that english has a special place in the global developer experience. Every keyword and standard construct of basically any language is in english, like if, function, public, protected and so on, and that is because it is the lingua franca on the development world. Inclusiveness is important, but using english as a minimum/baseline language is not problematic, imho, and gives actual concrete help to actual people
  • BURDEN: 3rd party/free/OSS packages may not have the same resources as Microsoft, and being somehow "forced" to create and maintain an online page is probably not the best thing. Yes, the url can point to a GitHub page in the repo itself, so no extra costs etc, but it still needs to be maintained

If developers want, they can always use the external url and localize everything, add images and so on.

Having said that, having an optional english-only message that can be directly used to very quickly give a minimum of context on what is going on is, again imho, a very sensible thing to do that achieves the very concrete goal of providing some context without taking anything away.

Just my 2 cents, hope this helps.

ps: just as a sidenote for future similar developments, starting again today I would have opted for the other way around, meaning mandatory message (minimum effort and concrete immediate result) + optional external url. But still, this is a very welcome and concretely useful addition.

@KrzysztofCwalina
Copy link
Member

Should the property be called UriFormat? We tend to use URI instead of URL in .NET APIs.

@tannergooding
Copy link
Member

UrlFormat is an existing API, it already shipped back in .NET 8

@github-actions github-actions bot locked and limited conversation to collaborators Dec 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants