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

False positive for rule CA1002 on sealed classes #88310

Open
EvinWatson opened this issue May 12, 2023 · 6 comments · May be fixed by dotnet/roslyn-analyzers#6729
Open

False positive for rule CA1002 on sealed classes #88310

EvinWatson opened this issue May 12, 2023 · 6 comments · May be fixed by dotnet/roslyn-analyzers#6729
Labels
area-System.Collections bug code-analyzer Marks an issue that suggests a Roslyn analyzer in-pr There is an active PR which will close this issue when it is merged Priority:3 Work that is nice to have
Milestone
Future

Comments

@EvinWatson
Copy link

EvinWatson commented May 12, 2023 •
edited

Analyzer

Diagnostic ID: CA1002: Do not expose generic lists

Analyzer source

NuGet Package: Microsoft.CodeAnalysis.NetAnalyzers

Version: 7.0.0

Describe the bug

This rule prevents the use of Generic Lists for inheritance issues, however sealed classes cannot be inherited, so the rule should not apply. I suggest creating an exception for sealed classes.

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/sealed

Steps To Reproduce

  1. Create a 'sealed' class.
  2. Create a property of type Generic List.
  3. Observe the analyzer give error CA1002.

Expected behavior

Analyzer should not cite CA1002 on sealed classes.

Actual behavior

Analyzer cites CA1002 on sealed classes.

Additional context

None
@mavasani mavasani added bug help wanted [up-for-grabs] Good issue for external contributors Priority:3 Work that is nice to have labels Jun 22, 2023
@CollinAlpert CollinAlpert linked a pull request Jul 2, 2023 that will close this issue
Open
@mavasani
Copy link
Member

mavasani commented Jul 3, 2023

I believe the rule description is talking about inheritance for the collection itself, and not the element type of the collection: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1002#rule-description

I'll move this issue to dotnet/runtime to get it triaged.

@mavasani mavasani transferred this issue from dotnet/roslyn-analyzers Jul 3, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 3, 2023
@ghost
Copy link

ghost commented Jul 3, 2023

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

Analyzer

Diagnostic ID: CA1002: Do not expose generic lists

Analyzer source

NuGet Package: Microsoft.CodeAnalysis.NetAnalyzers

Version: 7.0.0

Describe the bug

This rule prevents the use of Generic Lists for inheritance issues, however sealed classes cannot be inherited, so the rule should not apply. I suggest creating an exception for sealed classes.

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/sealed

Steps To Reproduce

  1. Create a 'sealed' class.
  2. Create a property of type Generic List.
  3. Observe the analyzer give error CA1002.

Expected behavior

Analyzer should not cite CA1002 on sealed classes.

Actual behavior

Analyzer cites CA1002 on sealed classes.

Additional context

None

Author: EvinWatson
Assignees: -
Labels:

bug, area-Meta, help wanted, untriaged, Priority:3
Milestone: -

@ericstj ericstj added code-analyzer Marks an issue that suggests a Roslyn analyzer area-System.Collections and removed area-Meta labels Jul 20, 2023
@ghost
Copy link

ghost commented Jul 20, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

Analyzer

Diagnostic ID: CA1002: Do not expose generic lists

Analyzer source

NuGet Package: Microsoft.CodeAnalysis.NetAnalyzers

Version: 7.0.0

Describe the bug

This rule prevents the use of Generic Lists for inheritance issues, however sealed classes cannot be inherited, so the rule should not apply. I suggest creating an exception for sealed classes.

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/sealed

Steps To Reproduce

  1. Create a 'sealed' class.
  2. Create a property of type Generic List.
  3. Observe the analyzer give error CA1002.

Expected behavior

Analyzer should not cite CA1002 on sealed classes.

Actual behavior

Analyzer cites CA1002 on sealed classes.

Additional context

None

Author: EvinWatson
Assignees: -
Labels:

bug, area-System.Collections, help wanted, untriaged, code-analyzer, Priority:3
Milestone: -

@ericstj
Copy link
Member

ericstj commented Jul 20, 2023

Moving to System.Collections area since this is feedback on an analyzer that applies to use of collections. cc @buyaa-n

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Jul 24, 2023
@eiriktsarpalis eiriktsarpalis modified the milestone: 8.0.0 Jul 24, 2023
@eiriktsarpalis eiriktsarpalis added the untriaged New issue has not been triaged by the area owner label Jul 24, 2023
@eiriktsarpalis
Copy link
Member

@buyaa-n is this something we should consider addressing for 8.0?

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Jul 24, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Jul 24, 2023
@eiriktsarpalis eiriktsarpalis removed the help wanted [up-for-grabs] Good issue for external contributors label Jul 24, 2023
@buyaa-n
Copy link
Member

buyaa-n commented Jul 26, 2023

@buyaa-n is this something we should consider addressing for 8.0?

There is an open PR already in the roslyn-analyzers repo, so we can address it in 8.0. It seems the issue moved here for deciding if we support the suggested fix. For me the suggestion CA1002 Analyzer should not flag on sealed classes makes sense.

Let me know what you think @eiriktsarpalis @stephentoub

@eiriktsarpalis eiriktsarpalis added the in-pr There is an active PR which will close this issue when it is merged label Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-System.Collections bug code-analyzer Marks an issue that suggests a Roslyn analyzer in-pr There is an active PR which will close this issue when it is merged Priority:3 Work that is nice to have
Projects
None yet
Milestone
Future
Development

Successfully merging a pull request may close this issue.

Don't emit CA1002 when type is sealed CollinAlpert/roslyn-analyzers
5 participants
@eiriktsarpalis @ericstj @mavasani @buyaa-n @EvinWatson