Skip to content

Conversation

@ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Aug 11, 2025

Brings along commit history from roslyn-analyzers


Validate source-build and the VMR build with these changes.
When this flows into the VMR, the "roslyn-analyzers" RepositoryReference needs to be removed.

mavasani and others added 30 commits October 3, 2023 20:56
Write CA1824 as non-compilation-end
* Don't warn if object creation in local assignment is used as argument

* Do not warn if options are used in a loop

* Remove whitespace

* Improve comment in IsLocalReferenceInsideChildLoop
Suggest CA2007 for await foreach without specified cancellation
Don't emit CA1849 for nameof expressions
Add analyzer for SemanticModel.GetDeclaredSymbol
Do not raise CA1065 when in delegate
Generate unique name for CA1861
docs(grammatical): correct typos
…oreAsync` (#6976)

* WIP

* feedback

* Remove VB test

* Update src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

Co-authored-by: Buyaa Namnan <[email protected]>
---------

Co-authored-by: Buyaa Namnan <[email protected]>
buyaa-n and others added 8 commits June 20, 2025 09:48
…te (#7569)

* Handle guarded maccatalyst attribute issue that suppressed by call site

* Skip any guarded platform that was suppressed by call site
* Handle extension members by 'make member static'

* Suppress warning introduced by Roslyn bump

* Suppress warning in test

* Update tests

* Update Roslyn

To bring in dotnet/roslyn#79571.

* Update Roslyn

To bring in dotnet/roslyn#79576.
…atform counterpart (#7721)

* Add a basic fixer to suggest replacing intrinsics with their cross-platform counterpart

* Remove an unnecessary using

* Ensure we use a new analyzer code, not an existing one

* Make csharp_indent_case_contents_when_block silent until the rest of the repo can be updated

* Don't set csharp_indent_case_contents_when_block in this PR

* Respond to PR feedback

* Fix nullability of the func parameter

* Fix nullability warning on arg0 parameter

* Prefer properties over custom tags

* Fix nullability

* Fix formatting

* Run msbuild /t:pack again

* Check for the existence of Vector64/128/256/512

* Split the tests apart into multiple files

* Mark test class as partial

* Address various PR feedback

* Fix formatting

* Additional cleanup and handle more PR feedback

* Ensure the additional tests compile

* Ensure the generator is passed through

* Ensure the SyntaxGeneratorExtensions are part of the shared project

* Missing semicolon

* Formatting

* Fix tests
@ViktorHofer ViktorHofer marked this pull request as ready for review August 14, 2025 19:30
Copilot AI review requested due to automatic review settings August 14, 2025 19:30
@ViktorHofer ViktorHofer requested a review from a team as a code owner August 14, 2025 19:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request moves Microsoft.CodeAnalysis.NetAnalyzers from the roslyn-analyzers repository into the SDK, bringing along commit history. It introduces a comprehensive set of code quality analyzers focused on API design guidelines.

  • Adds 20+ analyzer implementations covering various API design patterns and best practices
  • Includes corresponding code fix providers for most analyzers
  • Implements analyzers for disposable patterns, exception constructors, operator overloads, and naming conventions

Reviewed Changes

Copilot reviewed 251 out of 1515 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
PropertiesShouldNotBeWriteOnly.Fixer.cs Empty code fix provider implementation
PassSystemUriObjectsInsteadOfStrings.cs Analyzer detecting string parameters that should use Uri type
ParameterNamesShouldMatchBaseDeclaration.cs Analyzer ensuring parameter names match base/interface declarations
ParameterNamesShouldMatchBaseDeclaration.Fixer.cs Code fix to rename parameters to match base declarations
OverrideMethodsOnComparableTypes.cs Analyzer for IComparable types missing required overrides
OverrideMethodsOnComparableTypes.Fixer.cs Code fix to implement missing Equals/operator overloads
OverrideGetHashCodeOnOverridingEquals.Fixer.cs Code fix to add GetHashCode override
OverrideEqualsOnOverloadingOperatorEquals.Fixer.cs Code fix to add Equals override when operator== exists
OverrideEqualsAndOperatorEqualsOnValueTypes.cs Analyzer for value types missing equality implementations
OverrideEqualsAndOperatorEqualsOnValueTypes.Fixer.cs Code fix to implement equality members
OverloadOperatorEqualsOnOverridingValueTypeEquals.cs Analyzer for value types overriding Equals without operators
OverloadOperatorEqualsOnOverridingValueTypeEquals.Fixer.cs Code fix to implement equality operators
OperatorsShouldHaveSymmetricalOverloads.cs Analyzer ensuring symmetric operator pairs
OperatorsShouldHaveSymmetricalOverloads.Fixer.cs Code fix to generate missing symmetric operators
OperatorOverloadsHaveNamedAlternates.cs Analyzer requiring named alternatives for operators
OperatorOverloadsHaveNamedAlternates.Fixer.cs Code fix to add named method alternatives
NonConstantFieldsShouldNotBeVisible.cs Analyzer for public non-readonly static fields
NonConstantFieldsShouldNotBeVisible.Fixer.cs Empty code fix provider implementation
NestedTypesShouldNotBeVisible.cs Analyzer discouraging public nested types
MovePInvokesToNativeMethodsClass.cs Analyzer for P/Invoke organization
MovePInvokesToNativeMethodsClass.Fixer.cs Empty code fix provider implementation
MarkAttributesWithAttributeUsage.cs Analyzer ensuring AttributeUsage on custom attributes
MarkAttributesWithAttributeUsage.Fixer.cs Code fix to add AttributeUsage attribute
MarkAssembliesWithComVisible.cs Analyzer for COM visibility assembly attributes
MarkAssembliesWithComVisible.Fixer.cs Empty code fix provider implementation
MarkAssembliesWithClsCompliant.Fixer.cs Empty code fix provider implementation
MarkAssembliesWithAttributesDiagnosticAnalyzer.cs Analyzer for required assembly-level attributes
MarkAssembliesWithAssemblyVersion.Fixer.cs Empty code fix provider implementation
InterfaceMethodsShouldBeCallableByChildTypes.cs Analyzer for explicit interface implementations
InterfaceMethodsShouldBeCallableByChildTypes.Fixer.cs Code fix to improve interface implementation accessibility
ImplementStandardExceptionConstructors.cs Analyzer ensuring standard exception constructors
ImplementStandardExceptionConstructors.Fixer.cs Code fix to add missing exception constructors
ImplementIDisposableCorrectly.cs Comprehensive IDisposable pattern analyzer
ImplementIDisposableCorrectly.Fixer.cs Empty code fix provider implementation
IdentifiersShouldNotMatchKeywords.cs Analyzer preventing keyword conflicts in identifiers
IdentifiersShouldNotMatchKeywords.Fixer.cs Empty code fix provider implementation
IdentifiersShouldNotHaveIncorrectSuffix.cs Analyzer for proper type/member naming suffixes
IdentifiersShouldNotHaveIncorrectSuffix.Fixer.cs Empty code fix provider implementation

{
// Fixer not yet implemented.
return Task.CompletedTask;

Copy link

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

[nitpick] This fixer class returns an empty FixableDiagnosticIds array and doesn't implement any actual fixes. Consider removing the trailing newline after the return statement for consistency with other empty implementations.

Suggested change

Copilot uses AI. Check for mistakes.

private static async Task<Document> GetUpdatedDocumentForParameterRenameAsync(Document document, ISymbol parameter, string newName, CancellationToken cancellationToken)
{
Solution newSolution = await Renamer.RenameSymbolAsync(document.Project.Solution, parameter, newName, document.Project.Solution.Options, cancellationToken).ConfigureAwait(false);
Copy link

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

The method passes document.Project.Solution.Options but this property may be obsolete in newer versions of Roslyn. Consider using the newer options API if available.

Suggested change
Solution newSolution = await Renamer.RenameSymbolAsync(document.Project.Solution, parameter, newName, document.Project.Solution.Options, cancellationToken).ConfigureAwait(false);
var options = await document.GetOptionsAsync(cancellationToken).ConfigureAwait(false);
Solution newSolution = await Renamer.RenameSymbolAsync(document.Project.Solution, parameter, newName, options, cancellationToken).ConfigureAwait(false);

Copilot uses AI. Check for mistakes.
OperationKind.Block => ValidateOperations(((IBlockOperation)operation).Operations),
OperationKind.ExpressionStatement => ValidateExpression((IExpressionStatementOperation)operation),
OperationKind.Try => ValidateTryOperation((ITryOperation)operation),
_ => operation.IsOperationNoneRoot(),// Ignore operation roots with no IOperation API support (OperationKind.None)
Copy link

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The switch expression uses pattern matching but inconsistently mixes 'or' patterns with separate cases. Consider using consistent pattern matching throughout for better readability.

Suggested change
_ => operation.IsOperationNoneRoot(),// Ignore operation roots with no IOperation API support (OperationKind.None)
_ => operation.IsOperationNoneRoot(), // Ignore operation roots with no IOperation API support (OperationKind.None)

Copilot uses AI. Check for mistakes.
@KalleOlaviNiemitalo
Copy link
Contributor

Would it be possible to add the repository name to the pull request numbers in the commit messages? Or add a "commit migrated from" trailer like in dotnet/runtime@e8859b4.

Comment on lines +40 to +43
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing" Version="1.1.2" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.CodeRefactoring.Testing" Version="1.1.2" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic.CodeFix.Testing" Version="1.1.2" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic.CodeRefactoring.Testing" Version="1.1.2" />
Copy link
Member

Choose a reason for hiding this comment

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

Should these versions be hard-coded or should they flow from some repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

These should be hardcoded. We don't flow from roslyn-sdk.

<Error Condition="'@(CodeStyleVisualBasicConfig)' == ''" Text="Something moved around in Code style packeges, could not find globalconfig files. Adjust code here accordingly. TFM change?" />
<Copy SourceFiles="@(CodeStyleVisualBasicConfig)" DestinationFiles="@(CodeStyleVisualBasicConfig->'$(CodeStyleVisualBasicConfigDirectory)/%(RecursiveDir)%(Filename)%(Extension)')" SkipUnchangedFiles="true"/>
<Copy SourceFiles="@(ILLinkAnalyzersTargets)" DestinationFiles="@(ILLinkAnalyzersTargets->'$(AnalyzerTargetsDirectory)/%(RecursiveDir)%(Filename)%(Extension)')" SkipUnchangedFiles="true"/>
<Copy SourceFiles="@(ILLinkAnalyzersAssemblies)" DestinationFiles="@(ILLinkAnalyzersAssemblies->'$(AnalyzerAssembliesDirectory)/%(RecursiveDir)%(Filename)%(Extension)')" SkipUnchangedFiles="true"/>
Copy link
Member

Choose a reason for hiding this comment

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

Where were these coming from previously?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is dead code which dates back to when illink was integrated into the SDK.

@ViktorHofer
Copy link
Member Author

Would it be possible to add the repository name to the pull request numbers in the commit messages? Or add a "commit migrated from" trailer like in dotnet/runtime@e8859b4.

Certainly possible but from a pragmatic standpoint, not feasible due to time. Today's a public holiday for me and so I can't refresh this PR again with updated commits. This needs to go into RC1 for which the snap date is today. But I will add the information to the parent merge commit. Thanks for the suggestion though.

@ViktorHofer ViktorHofer merged commit e14d0e3 into main Aug 15, 2025
28 checks passed
@ViktorHofer ViktorHofer deleted the MoveRoslynAnalyzersToSdk branch August 15, 2025 10:57
@KalleOlaviNiemitalo
Copy link
Contributor

Was the reason of this move announced somewhere?

@ViktorHofer
Copy link
Member Author

Hello @KalleOlaviNiemitalo. No, the motivation wasn't shared publicly. We don't always share that, or even a timeline (example: the dotnet/format move) but let me elaborate for this case.

Essentially, already 90% of the analyzers got moved earlier this year into the roslyn repository. Keeping this repo's infrastructure alive just for the remaining NetAnalyzers component and handling branding and branching wasn't worth the cost. Therefore, we decided to move Netanalyzers into the sdk repository which follows other repositories that already got moved there.

We still need to decide how to best transfer the existing issues over. The roslyn-analyzers repository is also still used for servicing.

@xtqqczze
Copy link
Contributor

xtqqczze commented Oct 7, 2025

Would it be possible to add the repository name to the pull request numbers in the commit messages? Or add a "commit migrated from" trailer like in dotnet/runtime@e8859b4.

@ViktorHofer I think it was an unfortunate oversight not to include the suggested changes.

As a result, this has led to a significant loss of metadata, all GitHub Issue and Pull Request links from roslyn-analyzers are now permanently broken within the IDE. Unfortunately, there’s no way to recover the original references, since issue and PR numbers collide between repositories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.