search

dotnet / roslyn-analyzers

MIT License
1.59k stars 466 forks source link

CA1812 (Avoid uninstantiated internal classes) should be reported for abstract types, unless a derived type exists #1159

Open sharwell opened 7 years ago

sharwell commented 7 years ago

Analyzer package

Example: Microsoft.CodeQuality.Analyzers

Analyzer

CA1812: Avoid uninstantiated internal classes

Repro steps

  1. Declare an internal class with the abstract modifier
  2. Do not derive any types from the base class
internal abstract class UninstantiatedBaseType
{
}

Expected behavior

CA1812 should be reported.

Actual behavior

CA1812 is not reported.

:memo: If the abstract type has one or more derived types, CA1812 should not be reported on the base class. If/when the derived types are removed in accordance with CA1812, a new warning will indicate that the user can now remove CA1812.

333fred commented 7 years ago

Similar to #1163, this behavior is an explicit match with FxCop behavior, and unlike #1163, this would introduce a new warning where previously none existed. @dotnet/roslyn-analysis, thoughts on what we should do here? While I agree with the change in behavior, do we want to risk breaking people by reporting new warnings?

mavasani commented 5 years ago

We should add an editorconfig option to allow end users to configure this behavior.

Evangelink commented 5 years ago

What about interfaces? If an internal interface is never implemented it shall have a similar behavior as un-inherited abstract classes, don't you think?

Evangelink commented 4 years ago

I also don't understand why we don't report on struct nor enum, is there any reason not to?

JohanLarsson commented 4 years ago

Bonus points for warning on code only used by tests but that is probably a separate issue.

Evangelink commented 4 years ago

@JohanLarsson If we go this road it would be nice to be able to categorize an assembly as main code/production code VS test code. I don't want to have some diagnostic reported on an internal test class used only by test class. This class could be either an helper directly in the test assembly or in some code of test utility/test infrastructure project.

Evangelink commented 4 years ago

@mavasani We have raised a couple of interesting cases here. In your opinion, which one(s) shall be handled?