Skip to content

Enable CA1852/CA1812 Analyzers#7881

Merged
RussKie merged 1 commit intodotnet:mainfrom
elachlan:analyzer-changes
Oct 5, 2022
Merged

Enable CA1852/CA1812 Analyzers#7881
RussKie merged 1 commit intodotnet:mainfrom
elachlan:analyzer-changes

Conversation

@elachlan
Copy link
Copy Markdown
Contributor

@elachlan elachlan commented Oct 4, 2022

Fixes #7879

Singular warning for CA1812 supressed due to usage in interop.

Synced globalconfig with runtime and added #TODO to items that were warning.

Line  182: dotnet_diagnostic.CA1416.severity = none # TODO: warning
Line  260: dotnet_diagnostic.CA1727.severity = suggestion # TODO: warning
Line  273: dotnet_diagnostic.CA1810.severity = none # TODO: warning
Line  297: dotnet_diagnostic.CA1821.severity = none # TODO: warning
Line  394: dotnet_diagnostic.CA1853.severity =  none # TODO: warning
Line  397: dotnet_diagnostic.CA1854.severity =  none # TODO: warning
Line  406: dotnet_diagnostic.CA2007.severity = none # TODO: warning
Line  439: dotnet_diagnostic.CA2019.severity = none # TODO: warning
Line  466: dotnet_diagnostic.CA2208.severity = none # TODO: warning
Line  533: dotnet_diagnostic.CA2245.severity = none # TODO: warning
Line  557: dotnet_diagnostic.CA2253.severity = none # TODO: warning
Line  851: dotnet_diagnostic.IL3000.severity = none # TODO: warning
Line  873: dotnet_diagnostic.SA1001.severity = none # TODO: warning
Line  912: dotnet_diagnostic.SA1014.severity = none # TODO: warning
Line  924: dotnet_diagnostic.SA1018.severity = none # TODO: warning
Line  930: dotnet_diagnostic.SA1020.severity = none # TODO: warning
Line  948: dotnet_diagnostic.SA1026.severity = none # TODO: warning
Line  951: dotnet_diagnostic.SA1027.severity = none # TODO: warning
Line  954: dotnet_diagnostic.SA1028.severity = none # TODO: warning
Line  963: dotnet_diagnostic.SA1102.severity = none # TODO: warning
Line  966: dotnet_diagnostic.SA1105.severity = none # TODO: warning
Line  996: dotnet_diagnostic.SA1113.severity = none # TODO: warning
Line 1002: dotnet_diagnostic.SA1115.severity = none # TODO: warning
Line 1020: dotnet_diagnostic.SA1121.severity = none # TODO: warning
Line 1044: dotnet_diagnostic.SA1129.severity = none # TODO: warning
Line 1074: dotnet_diagnostic.SA1141.severity = none # TODO: warning
Line 1077: dotnet_diagnostic.SA1142.severity = none # TODO: warning
Line 1095: dotnet_diagnostic.SA1205.severity = none # TODO: warning
Line 1116: dotnet_diagnostic.SA1212.severity = none # TODO: warning
Line 1137: dotnet_diagnostic.SA1302.severity = none # TODO: warning
Line 1179: dotnet_diagnostic.SA1400.severity = none # TODO: warning
Line 1191: dotnet_diagnostic.SA1404.severity = none # TODO: warning
Line 1203: dotnet_diagnostic.SA1408.severity = none # TODO: warning
Line 1212: dotnet_diagnostic.SA1411.severity = none # TODO: warning
Line 1278: dotnet_diagnostic.SA1518.severity = none # TODO: warning
Line 1494: dotnet_diagnostic.IDE0020.severity = none # TODO: warning
Line 1521: dotnet_diagnostic.IDE0029.severity = none # TODO: warning
Line 1527: dotnet_diagnostic.IDE0031.severity = none # TODO: warning
Line 1542: dotnet_diagnostic.IDE0036.severity = none # TODO: warning
Line 1593: dotnet_diagnostic.IDE0054.severity = none # TODO: warning
Line 1608: dotnet_diagnostic.IDE0059.severity = none # TODO: warning
Line 1618: dotnet_diagnostic.IDE0062.severity = none # TODO: warning
Line 1627: dotnet_diagnostic.IDE0065.severity = none # TODO: warning
Line 1645: dotnet_diagnostic.IDE0074.severity = none # TODO: warning
Line 1669: dotnet_diagnostic.IDE0082.severity = none # TODO: warning
Line 1681: dotnet_diagnostic.IDE0100.severity = none # TODO: warning
Line 1684: dotnet_diagnostic.IDE0110.severity = none # TODO: warning
Line 1705: dotnet_diagnostic.IDE0170.severity = none # TODO: warning
Line 1711: dotnet_diagnostic.IDE0200.severity = none # TODO: warning
Microsoft Reviewers: Open in CodeFlow

@elachlan elachlan requested a review from a team as a code owner October 4, 2022 04:42
@ghost ghost assigned elachlan Oct 4, 2022
RussKie
RussKie previously approved these changes Oct 4, 2022
@RussKie
Copy link
Copy Markdown
Contributor

RussKie commented Oct 4, 2022

Were you able to confirm the analyzers flag the types you identified in #7831 (comment)?

@RussKie RussKie added the waiting-author-feedback The team requires more information from the author label Oct 4, 2022
@elachlan
Copy link
Copy Markdown
Contributor Author

elachlan commented Oct 4, 2022

Were you able to confirm the analyzers flag the types you identified in #7831 (comment)?

It did not! I am looking into it.

@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Oct 4, 2022
@RussKie RussKie added the waiting-author-feedback The team requires more information from the author label Oct 4, 2022
@elachlan
Copy link
Copy Markdown
Contributor Author

elachlan commented Oct 4, 2022

I think its the version of Microsoft.CodeAnalysis.NetAnalyzers, but I can't see it in eng\Versions.props. I guess this is tied to the SDK version?

@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Oct 4, 2022
@gpetrou
Copy link
Copy Markdown
Contributor

gpetrou commented Oct 4, 2022

I think its the version of Microsoft.CodeAnalysis.NetAnalyzers, but I can't see it in eng\Versions.props. I guess this is tied to the SDK version?

The version that we use is in https://github.com/dotnet/winforms/blob/be7b0368d986b6220d259f2dd0785ccf3c728f0e/eng/CodeStyle.targets.
Usually when we update the version, we also update the rules config file to match what runtime has in https://github.com/dotnet/runtime/blob/release/7.0/eng/CodeAnalysis.src.globalconfig.

@elachlan
Copy link
Copy Markdown
Contributor Author

elachlan commented Oct 4, 2022

I can sync with runtime if you want? Or did the team want to do that?

@gpetrou
Copy link
Copy Markdown
Contributor

gpetrou commented Oct 4, 2022

I can sync with runtime if you want? Or did the team want to do that?

I am an external contributor, so I am not representing the team. What I did in the past was to sync the rules with runtime and add severity = none with TODOs for items that give warnings and can be done in separate PRs. You can see in the current main that some TODOs are still there.

@elachlan
Copy link
Copy Markdown
Contributor Author

elachlan commented Oct 4, 2022

@NewellClark winforms is looking at implementing CA1852. Some of the classes are not being picked up by the analyzer. Am I misunderstanding the scope of the analyzer?

Example:

/// <summary>
/// Control we use to provide the content alignment UI.
/// </summary>
private class ContentUI : Control

@RussKie
Copy link
Copy Markdown
Contributor

RussKie commented Oct 4, 2022

The version that we use is in be7b036/eng/CodeStyle.targets.

Oh, thank you! (I should know these things, shouldn't I? :))
We should move the versions to eng\Versions.props so we have all versions in the same place.

@elachlan
Copy link
Copy Markdown
Contributor Author

elachlan commented Oct 4, 2022

The version that we use is in be7b036/eng/CodeStyle.targets.

Oh, thank you! (I should know these things, shouldn't I? :)) We should move the versions to eng\Versions.props so we have all versions in the same place.

Already done :)

@RussKie
Copy link
Copy Markdown
Contributor

RussKie commented Oct 4, 2022

I can sync with runtime if you want? Or did the team want to do that?

We take all the help we can from the community :)

@RussKie
Copy link
Copy Markdown
Contributor

RussKie commented Oct 4, 2022

Do you mind updating <PackageReference Include="StyleCop.Analyzers" Version="1.2.0-beta.354" PrivateAssets="all" /> as well?

Comment thread eng/CodeStyle.targets Outdated
Comment thread eng/CodeStyle.targets Outdated
Comment thread eng/Versions.props Outdated
Comment thread eng/CodeAnalysis.src.globalconfig
Comment thread eng/CodeAnalysis.test.globalconfig Outdated
@RussKie RussKie added the waiting-author-feedback The team requires more information from the author label Oct 4, 2022
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Oct 5, 2022
@elachlan elachlan requested a review from RussKie October 5, 2022 01:06
@elachlan
Copy link
Copy Markdown
Contributor Author

elachlan commented Oct 5, 2022

@RussKie only issue to resolve now is why the CA1852 doesn't detect the private classes we found in the other PR.

Does winforms use InternalsVisibleToAttribute?

@RussKie
Copy link
Copy Markdown
Contributor

RussKie commented Oct 5, 2022

Does winforms use InternalsVisibleToAttribute?

Yes, quite extensively: https://github.com/dotnet/winforms/search?q=InternalsVisibleTo

@elachlan
Copy link
Copy Markdown
Contributor Author

elachlan commented Oct 5, 2022

Comment from the Analyzer code:
// To avoid false positives, as with CA1812 (avoid uninstantiated internal classes), skip any assemblies with InternalsVisibleTo.

The projects has the following and so is excluded:

// expose internal types to System.Design for type forwarding
[assembly: InternalsVisibleTo("System.Design, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
[assembly: InternalsVisibleTo("System.Windows.Forms.Design.Tests, PublicKey=00000000000000000400000000000000")]
[assembly: InternalsVisibleTo("System.Windows.Forms.UI.IntegrationTests, PublicKey=00000000000000000400000000000000")]

We can continue to use it, but it won't find anything on projects using InternalsVisibleTo.

@elachlan
Copy link
Copy Markdown
Contributor Author

elachlan commented Oct 5, 2022

An escape hatch for this behaviour is being tracked at dotnet/roslyn-analyzers#5973

@RussKie
Copy link
Copy Markdown
Contributor

RussKie commented Oct 5, 2022

Awesome! Thank you heaps for taking the lead on this.

@RussKie RussKie merged commit 536e37c into dotnet:main Oct 5, 2022
@RussKie RussKie added the enhancement Product code improvement that does NOT require public API changes/additions label Oct 5, 2022
@elachlan elachlan deleted the analyzer-changes branch October 5, 2022 06:27
@ghost ghost added this to the 8.0 Preview1 milestone Oct 5, 2022
v-elnovikova pushed a commit to v-elnovikova/winforms that referenced this pull request Oct 18, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement Product code improvement that does NOT require public API changes/additions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable CA1852/CA1812 Analyzers

3 participants