Skip to content

Track nullability information in publicAPI files for C# compiler#41000

Merged
jcouv merged 2 commits intodotnet:mainfrom
jcouv:nullable-api
Jan 3, 2022
Merged

Track nullability information in publicAPI files for C# compiler#41000
jcouv merged 2 commits intodotnet:mainfrom
jcouv:nullable-api

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Jan 16, 2020 •
edited
Loading

Note: this PR was updated to track APIs involving oblivious reference types with a ~ prefix.

This PR ran into issue #50357
But the public API fixer can be run with the instructions below:

  1. Install dotnet-format as a global tool. It does ship as part of the SDK, but a separate version can be installed as a global tool and invoked with dotnet-format {options}.
    C:\Source\roslyn> dotnet tool install -g dotnet-format --version "6.*" --add-source https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet6/nuget/v3/index.json
  2. Restore and build Compilers.sln. This is necessary to ensure the source generator project is built and we can load the generator assembly when running dotnet-format.
    C:\Source\roslyn> .\restore.cmd
    C:\Source\roslyn> .\build.cmd
  3. Invoke the dotnet-format global tool. Running only the analyzers subcommand and fixing only the “missing Public API signature” diagnostic. We must also pass the --include-generated flag to include source generated documents in the analysis.
    C:\Source\roslyn> cd ..
    C:\Source> dotnet-format analyzers .\roslyn\Compilers.sln --diagnostics=RS0016 --no-restore --include-generated -v diag

@jcouv jcouv added Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types labels Jan 16, 2020
@jcouv jcouv self-assigned this Jan 16, 2020
@jcouv jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 17, 2020
@jcouv jcouv force-pushed the nullable-api branch 2 times, most recently from 1cb27fe to 0c33b24 Compare March 4, 2020 02:05
@jcouv jcouv mentioned this pull request Mar 4, 2020
Merged
@jcouv jcouv closed this Mar 16, 2020
@jcouv jcouv reopened this Mar 20, 2020
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Jan 5, 2021 •
edited
Loading

Here's the latest set of oblivious APIs:


~Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousMethodExpressionSyntax.Update(Microsoft.CodeAnalysis.SyntaxToken asyncKeyword, Microsoft.CodeAnalysis.SyntaxToken delegateKeyword, Microsoft.CodeAnalysis.CSharp.Syntax.ParameterListSyntax parameterList, Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode body) -> Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousMethodExpressionSyntax
~Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousMethodExpressionSyntax.Update(Microsoft.CodeAnalysis.SyntaxToken asyncKeyword, Microsoft.CodeAnalysis.SyntaxToken delegateKeyword, Microsoft.CodeAnalysis.CSharp.Syntax.ParameterListSyntax parameterList, Microsoft.CodeAnalysis.CSharp.Syntax.BlockSyntax block, Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax expressionBody) -> Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousMethodExpressionSyntax
~Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousMethodExpressionSyntax.WithAsyncKeyword(Microsoft.CodeAnalysis.SyntaxToken asyncKeyword) -> Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousMethodExpressionSyntax
~Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousMethodExpressionSyntax.WithBody(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode body) -> Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousMethodExpressionSyntax
~Microsoft.CodeAnalysis.CSharp.Syntax.LambdaExpressionSyntax.WithAsyncKeyword(Microsoft.CodeAnalysis.SyntaxToken asyncKeyword) -> Microsoft.CodeAnalysis.CSharp.Syntax.LambdaExpressionSyntax
~Microsoft.CodeAnalysis.CSharp.Syntax.LambdaExpressionSyntax.WithBody(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode body) -> Microsoft.CodeAnalysis.CSharp.Syntax.LambdaExpressionSyntax
~static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.AnonymousMethodExpression() -> Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousMethodExpressionSyntax
~static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.AnonymousMethodExpression(Microsoft.CodeAnalysis.SyntaxToken asyncKeyword, Microsoft.CodeAnalysis.SyntaxToken delegateKeyword, Microsoft.CodeAnalysis.CSharp.Syntax.ParameterListSyntax parameterList, Microsoft.CodeAnalysis.CSharp.Syntax.BlockSyntax block, Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax expressionBody) -> Microsoft.CodeAnalysis.CSharp.Syntax.AnonymousMethodExpressionSyntax

@jcouv jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 5, 2021
@jcouv jcouv mentioned this pull request Jan 6, 2021
Closed
@jcouv jcouv mentioned this pull request Jan 9, 2021
Closed
@jcouv jcouv added PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. Blocked labels Jan 9, 2021
@jcouv jcouv mentioned this pull request Jan 26, 2021
Merged
Base automatically changed from master to main March 3, 2021 23:52
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Apr 24, 2021

I'll try to push this PR forward using dotnet forward once #51608 is merged.

@jcouv jcouv removed Blocked PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Dec 18, 2021
@jcouv jcouv marked this pull request as ready for review December 18, 2021 05:30
@jcouv jcouv requested a review from a team as a code owner December 18, 2021 05:30
@runfoapp runfoapp bot mentioned this pull request Dec 20, 2021
Closed
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Dec 28, 2021

@dotnet/roslyn-compiler for review. Thanks

333fred
333fred reviewed Dec 28, 2021
View reviewed changes
src/Compilers/CSharp/Portable/Syntax/AnonymousMethodExpressionSyntax.cs
// See the LICENSE file in the project root for more information.

#nullable disable
#pragma warning disable RS0041 // uses oblivious reference types
Copy link
Copy Markdown
Member

@333fred 333fred Dec 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There aren't that many APIs in these files. Should we just annotate them?

Copy link
Copy Markdown
Member Author

@jcouv jcouv Dec 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd tried that but it snowballed the change. Admittedly this was a while back, but my recollection is some of those have many usage sites that needed adjustment. So I'd kept the PR focused on its primary purpose.

Copy link
Copy Markdown
Member

@333fred 333fred Dec 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough reason for me.

@333fred
Copy link
Copy Markdown
Member

333fred commented Dec 28, 2021

Done review pass (commit 1)

@jcouv jcouv requested a review from 333fred December 29, 2021 18:48
@RikkiGibson
Copy link
Copy Markdown
Member

RikkiGibson commented Jan 3, 2022

Could you please include the instructions in the PR description in some in-repo documentation?

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Jan 3, 2022

Could you please include the instructions in the PR description in some in-repo documentation?

Done. However the instructions will need to be updated/simplified when certain bits are published. @JoeRobich can advise.

@jcouv jcouv requested a review from RikkiGibson January 3, 2022 20:52
RikkiGibson
RikkiGibson approved these changes Jan 3, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but it still wasn't clear to me how/when the fixer was changed to include non-annotated and oblivious reference types.

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jan 3, 2022

In my opinion, the PR title "Update Public API files" includes no useful information that one generally expects to find there.

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Jan 3, 2022

@RikkiGibson The functionality for the publicAPI analyzer/fixer to track nullability annotations was added a while back. We've turned on #nullable enable in the Core project, but couldn't do it for the C# project, because the fixer doesn't work well with source-generators. The instructions are a mitigation for that.

@jcouv jcouv changed the title Update Public API files Track nullability information in publicAPI files for C# compiler Jan 3, 2022
@jcouv jcouv merged commit 7b06584 into dotnet:main Jan 3, 2022
@jcouv jcouv deleted the nullable-api branch January 3, 2022 23:53
@ghost ghost added this to the Next milestone Jan 3, 2022
@Cosifne Cosifne modified the milestones: Next, 17.1.P3 Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@333fred 333fred 333fred approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types

Projects

None yet

Milestone

17.1.P3

Development

Successfully merging this pull request may close these issues.

5 participants

@jcouv @333fred @RikkiGibson @AlekseyTs @Cosifne