-
Notifications
You must be signed in to change notification settings - Fork 795
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
Add support for C# Experimental
attribute
#18253
base: main
Are you sure you want to change the base?
Conversation
❗ Release notes required
|
Experimental
attributeExperimental
attribute
@@ -1795,7 +1794,7 @@ type Exception with | |||
if s <> "" then | |||
os.AppendString(Obsolete2E().Format s) | |||
|
|||
| Experimental(s, _) -> os.AppendString(ExperimentalE().Format s) | |||
| Experimental(message = s) -> os.AppendString(ExperimentalE().Format s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose the message includes the added properties if they were present even in plain (text output) build results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially the ID might be something useful for searching with.
The url would be better a clicklable, but at least plaintext for command line builds is IMO also beneficial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Experimental
attribute does not have a message property for now. It seems that will be added in the next mayor version to match the obsolete attribute See.
dotnet/runtime#108216
But I think we can handle this so it will work when it is available for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the DiagnosticId
We have 2 options:
- Use the provided id from the attribute
[Experimental(diagnosticId: "MY001")]
as a error number which potentially might collide with existing error numbers. - Use 57 like we do in F# but that will defeat the purpose of the
diagnosticId
I guess the problem is to report the experimental as error number 57 and then show a diagnostic id was part the error text which will be confusing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@T-Gro I have updated this PR to follow the same Obsolete
structures of messages. See tests. Im still not sure about adding the diagnosticId
as part of the error text as explain in my previous message.
Is there some reason this is insufficient? https://github.com/dotnet/fsharp/blob/main/src/FSharp.Core/prim-types.fs#L265 |
@edgarfgp added support for detecting the same attribute which was added in .NET / supported by C#12+ - https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-12.0/experimental-attribute |
Description
Fixes #18198
Checklist