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.

Sign up for GitHub

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

Jump to bottom

Move repo to newer version of Public Api analyzers #65737

Merged
merged 4 commits into from
Dec 4, 2022
Merged

Move repo to newer version of Public Api analyzers #65737

merged 4 commits into from
Dec 4, 2022

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented Dec 3, 2022

No description provided.

@mavasani mavasani requested a review from a team as a code owner December 3, 2022 06:17
@mavasani
Copy link
Contributor Author

mavasani commented Dec 3, 2022 •
edited
Loading

@jasonmalinowski It seems the correctness analyzers leg is failing as you recently added a public API that has no entry in unshipped file:

src/Workspaces/Core/Portable/Workspace/ProjectSystem/IFileChangeWatcher.cs(26,25): error RS0016: (NETCORE_ENGINEERING_TELEMETRY=Build) Symbol 'WatchedDirectory' is not part of the declared public API

Looking at your PR which added this public API: #65701, it seems the public API addition was likely unintentional. I am going to to switch it to an internal type, and feel free to make it public again if that was an intentional change.

EDIT: It seems #65701 added 2 more public interfaces: IFileChangeContext and IWatchedFile. I am not sure if any of them were intentionally made public. I have switched all of them back to internal. I have filed #65738 to verify if my changes are correct or not.

@mavasani mavasani requested a review from a team as a code owner December 3, 2022 06:43
@mavasani mavasani mentioned this pull request Dec 3, 2022
Closed
AlekseyTs
AlekseyTs approved these changes Dec 3, 2022
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 3)

@mavasani mavasani requested review from a team as code owners December 3, 2022 16:15
@mavasani mavasani mentioned this pull request Dec 3, 2022
Closed
@mavasani
Copy link
Contributor Author

mavasani commented Dec 3, 2022

Filed #65746 to review all the non-infra changes in this PR. I am going to merge this PR as soon as it is green to unblock re-enabling the public API analyzer and avoid more public API changes to unintentionally sneak in.

mavasani
mavasani commented Dec 3, 2022
View reviewed changes
src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs
@@ -3331,7 +3331,7 @@ public override SyntaxNode BaseExpression()
public override SyntaxNode TypedConstantExpression(TypedConstant value)
=> ExpressionGenerator.GenerateExpression(value);

protected override SyntaxNode GenerateExpression(ITypeSymbol? type, object? value, bool canUseFieldReference)
private protected override SyntaxNode GenerateExpression(ITypeSymbol? type, object? value, bool canUseFieldReference)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe this was intended to be a public API, but should be confirmed with Cyrus as part of #65746

Copy link
Member

Choose a reason for hiding this comment

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

SyntaxGenerator is not publicly inheritable (it has tons of internal abstract members). As such, all 'protected' members are save to change in any fashion.

Note: i would love it public-api-analyzer understood this. I think it understands if there is an explicit internal constructor, but not if there are internal abstracts.

mavasani
mavasani commented Dec 3, 2022
View reviewed changes
src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt
@@ -15,6 +15,8 @@ Microsoft.CodeAnalysis.Editing.DeclarationModifiers.IsFile.get -> bool
Microsoft.CodeAnalysis.Editing.DeclarationModifiers.WithIsFile(bool isFile) -> Microsoft.CodeAnalysis.Editing.DeclarationModifiers
*REMOVED*static Microsoft.CodeAnalysis.Editing.SyntaxGenerator.DefaultRemoveOptions -> Microsoft.CodeAnalysis.SyntaxRemoveOptions
Microsoft.CodeAnalysis.Editing.SyntaxEditor.SyntaxEditor(Microsoft.CodeAnalysis.SyntaxNode root, Microsoft.CodeAnalysis.Host.SolutionServices services) -> void
*REMOVED*abstract Microsoft.CodeAnalysis.Editing.SyntaxGenerator.LiteralExpression(object value) -> Microsoft.CodeAnalysis.SyntaxNode
Microsoft.CodeAnalysis.Editing.SyntaxGenerator.LiteralExpression(object value) -> Microsoft.CodeAnalysis.SyntaxNode
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These 2 changes should be confirmed with Cyrus as part of #65746. It seems he converted an existing public abstract method to a public non-abstract method.

Copy link
Member

Choose a reason for hiding this comment

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

yes. that's correct (and safe from an ABI perspective). Thsi type cannot be publicly overridden either, so this is fine.

mavasani
mavasani commented Dec 3, 2022
View reviewed changes
src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt
@@ -52,8 +54,7 @@ override Microsoft.CodeAnalysis.LoadTextOptions.GetHashCode() -> int
override Microsoft.CodeAnalysis.LoadTextOptions.ToString() -> string
static Microsoft.CodeAnalysis.LoadTextOptions.operator !=(Microsoft.CodeAnalysis.LoadTextOptions left, Microsoft.CodeAnalysis.LoadTextOptions right) -> bool
static Microsoft.CodeAnalysis.LoadTextOptions.operator ==(Microsoft.CodeAnalysis.LoadTextOptions left, Microsoft.CodeAnalysis.LoadTextOptions right) -> bool
virtual Microsoft.CodeAnalysis.FileTextLoader.CreateText(System.IO.Stream stream, Microsoft.CodeAnalysis.LoadTextOptions options, System.Threading.CancellationToken cancellationToken) -> Microsoft.CodeAnalysis.Text.SourceText
*REMOVED*override Microsoft.CodeAnalysis.FileTextLoader.LoadTextAndVersionAsync(Microsoft.CodeAnalysis.Workspace workspace, Microsoft.CodeAnalysis.DocumentId documentId, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.Task<Microsoft.CodeAnalysis.TextAndVersion>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TextLoader and FileTextLoader related changes in this file should be reviewed with Tomas as part of #65746

mavasani
mavasani commented Dec 3, 2022
View reviewed changes
src/Workspaces/Core/Portable/Workspace/ProjectSystem/IFileChangeWatcher.cs
@@ -23,7 +23,7 @@ internal interface IFileChangeWatcher
/// just creating a single directory watch on the root of a project for source file changes: rather than creating a file watcher
/// for each individual file, we can just watch the entire directory and that's it.
/// </remarks>
public sealed class WatchedDirectory
internal sealed class WatchedDirectory
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes to this file should be confirmed with JasonMal as part of #65746

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, that wasn't intended to be made public, thanks for catching that!

@mavasani mavasani merged commit 594437b into dotnet:main Dec 4, 2022
@ghost ghost added this to the Next milestone Dec 4, 2022
@mavasani mavasani deleted the BumpAnalyzersVersion branch December 4, 2022 18:44
@mavasani mavasani mentioned this pull request Dec 5, 2022
Closed
@allisonchou allisonchou mentioned this pull request Dec 15, 2022
Closed
@Cosifne Cosifne modified the milestones: Next, 17.5 P3 Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@333fred 333fred 333fred approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees
No one assigned
Labels
Area-Analyzers
Projects
None yet
Milestone
17.5 P3
Development

Successfully merging this pull request may close these issues.
6 participants
@mavasani @jasonmalinowski @333fred @CyrusNajmabadi @AlekseyTs @Cosifne