-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Support marking generated public APIs as experimental #82131
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
base: main
Are you sure you want to change the base?
Conversation
jasonmalinowski
left a comment
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.
Let a few comments at least. My biggest high level thought is whether we should just be suppressing the experimental warning for our entire codebase; since if we build together and ship together, I'm not sure we're getting any benefit for it. (The attribute absolutely makes sense for other projects!)
| internal const string GeneratorHostOutputs = "RSEXPERIMENTAL004"; | ||
| internal const string GeneratorHostOutputs_Url = "https://github.com/dotnet/roslyn/issues/74753"; | ||
|
|
||
| // The URL is customized per-api to point at the test plan for the feature |
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.
Seeing this comment made me think 'which URL?' Given the other examples in this file I expected to see a URL here; maybe it's expanding on this comment a bit.
src/Features/CSharp/Portable/Completion/CompletionProviders/NamedParameterCompletionProvider.cs
Outdated
Show resolved
Hide resolved
src/Tools/Source/CompilerGeneratorTools/Source/IOperationGenerator/IOperationClassWriter.cs
Outdated
Show resolved
Hide resolved
No. This will help ensure that manually-written public APIs that include the API are also marked |
| (node is ExpressionSyntax && (isSpeculative || allowNamedArgumentName || !SyntaxFacts.IsNamedArgumentName(node))) || | ||
| (node is ConstructorInitializerSyntax | ||
| or PrimaryConstructorBaseTypeSyntax | ||
| #pragma warning disable RSEXPERIMENTAL006 // With Element: https://github.com/dotnet/roslyn/issues/80613 |
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.
Could we instead suppress this warning globally within the compiler codebase?
That would avoid sprinkling of suppressions and would make it much easier for the PR that adds a new LangVer #Closed
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.
No. The non-suppressed global warning helped me catch manually-written public APIs that also need to be marked as Experimental. We could consider suppressing it in test projects/IDE, but I very much do not think we should do so in the main compiler codebase.
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.
The non-suppressed global warning helped me catch manually-written public APIs that also need to be marked as Experimental.
How exactly did that help you?
| // - [ ] update _MaxAvailableLangVersion cap (a relevant test should break when new version is introduced) | ||
| // - [ ] update the "UpgradeProject" codefixer | ||
| // - [ ] Remove the `Experimental` section from any entries being shipped in Syntax.xml and OperationInterfaces.xml, and rerun the generator | ||
| // - [ ] Search the codebase for the URLs that were linked in the Syntax.xml and OperationInterfaces.xml files and remove suppressions or attributes added for those issues. |
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'm hoping we can remove this work item by using a global suppression for the compiler codebase #Closed
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.
No. See previous comment.
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.
Can we remove this work item now that we've landed decision to suppress globally?
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.
We can adjust it, not remove it. Some APIs may still need to be adjusted because they were manually marked.
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.
Search the codebase for the URLs [...] and remove suppressions
URLs wouldn't be linked in the suppressions, right?
I can think of two cases:
I'm still in favor of a global suppression, but maybe we could pick a few files (those where we put public APIs) to re-enable the warning? In reply to: 3793141614 |
|
A possible middle-ground: we add file-level suppression for files with internal types (Binder, SemanticModel, etc) and all our test projects to reduce need for local suppressions In reply to: 3795392872 |
src/Tools/Source/CompilerGeneratorTools/Source/IOperationGenerator/IOperationClassWriter.cs
Outdated
Show resolved
Hide resolved
jcouv
left a comment
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.
Done with review pass (commit 5)
| <xs:complexType> | ||
| <xs:attribute name="Name" type="xs:string" use="required" /> | ||
| <xs:attribute name="Base" type="xs:string" use="required" /> | ||
| <xs:attribute name="ExperimentalUrl" type="xs:string" use="optional" /> |
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.
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.
It is maintained manually. It predates my time here, and presumably provides completion for the XML structure in Syntax.xml. If we decide we don't want it, that's ok with me, but I think that should be a separate PR. For now, I'm going to keep this as is and update appropriately.
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.
Yes, it's used for that. I think the checker also uses this to validate the structure of the XML too?
|
@333fred I think it would be good to provide a general description:
|
|
I very much dislike the suppressions sprinkled throughout the code base and would like to understand what benefits they provide by comparison to a global suppression. #Closed |
|
Done with review pass (commit 6) #Closed |
|
@dotnet/roslyn-compiler for review |
|
It looks like there are some related CI errors |
|
Done with review pass (commit 10) |
This reverts commit 1bbf7d9.
| // See the LICENSE file in the project root for more information. | ||
| // < auto-generated /> | ||
| #nullable enable | ||
| #pragma warning disable RSEXPERIMENTAL006 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. |
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.
Do we still need this suppression given the suppression editorconfig?
Also applies to other generated files #Closed
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 can remove it from the iop trees, but not the syntax generator ones, since those actually use source generation and the .editorconfig value does not apply to them.
| static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.WithElement(Microsoft.CodeAnalysis.SyntaxToken withKeyword, Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentListSyntax! argumentList) -> Microsoft.CodeAnalysis.CSharp.Syntax.WithElementSyntax! | ||
| virtual Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor.VisitWithElement(Microsoft.CodeAnalysis.CSharp.Syntax.WithElementSyntax! node) -> void | ||
| virtual Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor<TResult>.VisitWithElement(Microsoft.CodeAnalysis.CSharp.Syntax.WithElementSyntax! node) -> TResult? | ||
| [RSEXPERIMENTAL006]Microsoft.CodeAnalysis.CSharp.Syntax.WithElementSyntax |
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.
Should the SyntaxKind above bet considered experimental too?
| Microsoft.CodeAnalysis.IPropertySymbol.ReduceExtensionMember(Microsoft.CodeAnalysis.ITypeSymbol! receiverType) -> Microsoft.CodeAnalysis.IPropertySymbol? | ||
| Microsoft.CodeAnalysis.OperationKind.CollectionExpressionElementsPlaceholder = 129 -> Microsoft.CodeAnalysis.OperationKind | ||
| Microsoft.CodeAnalysis.Operations.ICollectionExpressionElementsPlaceholderOperation | ||
| [RSEXPERIMENTAL006]Microsoft.CodeAnalysis.OperationKind.CollectionExpressionElementsPlaceholder = 129 -> Microsoft.CodeAnalysis.OperationKind |
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.
Should ConstructArguments below be marked as experimental?
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.
This appears to be a bug in the analyzer. It is marked as experimental: https://github.com/dotnet/roslyn/pull/82131/changes#diff-4f30dad78155dc7e2f28813d10dab3bb825c845d1f14233117a362615aa83343R3972-R3973
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've fixed the bug in the PR, but since we reference the package via nuget, not via a project reference, consuming the bugfix will need to wait for the next time we bump this.
jcouv
left a comment
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.
Done with review pass (commit 11)
|
|
||
| if (!string.IsNullOrEmpty(experimentalUrl)) | ||
| { | ||
| WriteLine($"[Experimental(global::Microsoft.CodeAnalysis.RoslynExperiments.PreviewLanguageFeatureApi, UrlFormat = {QuoteString(experimentalUrl)})]"); |
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 think this is the same as WriteExperimentalAttribute? And then delete QuoteString since this might be the only user?
| - Any time the compiler reads in metadata containing crypto hashes, even if it's an attribute value, ensure the crypto hash algorithm name is included in the metadata (e.g. prefixing it to the hash value), so that it can be changed over time and the compiler can continue to read both metadata using both the old and new algorithms. | ||
| - All newly added APIs are experimental | ||
| - Tracking issue for marking APIs as non-experimental | ||
| - APIs are marked with `[Experimental(RoslynExperiments.PreviewLanguageFeatureApi, UrlFormat = @"link to tracking issue")]` |
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 wouldn't expect backslashes in the URL
| - APIs are marked with `[Experimental(RoslynExperiments.PreviewLanguageFeatureApi, UrlFormat = @"link to tracking issue")]` | |
| - APIs are marked with `[Experimental(RoslynExperiments.PreviewLanguageFeatureApi, UrlFormat = "link to tracking issue")]` |
| dotnet_diagnostic.xUnit2029.severity = none | ||
|
|
||
| # RS0006: Roslyn experimental language API | ||
| dotnet_diagnostic.RSEXPERIMENTAL006.severity = none |
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.
Did you consider putting this into a .globalconfig file instead? Then we wouldn't need separate suppressions in source-generated files.
| // - [ ] update _MaxAvailableLangVersion cap (a relevant test should break when new version is introduced) | ||
| // - [ ] update the "UpgradeProject" codefixer | ||
| // - [ ] Remove the `Experimental` section from any entries being shipped in Syntax.xml and OperationInterfaces.xml, and rerun the generator | ||
| // - [ ] Search the codebase for the URLs that were linked in the Syntax.xml and OperationInterfaces.xml files and remove suppressions or attributes added for those issues. |
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.
Search the codebase for the URLs [...] and remove suppressions
URLs wouldn't be linked in the suppressions, right?
We now mark preview language feature public APIs as experimental before the language version ships fully. As demonstration, I've marked the collection expression arguments public APIs as experimental, so we can see what this actually looks like.