Make CA1852 aware of InternalsVisibleTo

[stephentoub]

Without this there are too many false positives in libraries that use InternalsVisibleTo, in particular to tests.

cc: @GrabYourPitchforks, @mavasani

[codecov]

Codecov Report

Merging #5972 (9c9f82b) into main (890b7e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5972   +/-   ##
=======================================
  Coverage   96.04%   96.04%           
=======================================
  Files        1325     1325           
  Lines      306082   306091    +9     
  Branches     9693     9694    +1     
=======================================
+ Hits       293973   293985   +12     
+ Misses       9773     9770    -3     
  Partials     2336     2336           

[sharwell]

💭 It seems like this would negate much of what this rule could be valuable for. In cases where we blanket disable a rule for an entire project, why would it not be reasonable to expect the project with IVTs to just set the severity of this rule to none?

[stephentoub]

It seems like this would negate much of what this rule could be valuable for. In cases where we blanket disable a rule for an entire project, why would it not be reasonable to expect the project with IVTs to just set the severity of this rule to none?

If you're forced to disable the rule for a project that uses IVTs because of false positives, then IMO it's better the rule is disabled to avoid those false positives in the first place. If we wanted to special-case private, we could, but nested types are also the minority here. IVT simply isn't friendly to many such analyses, e.g. the .NET trimmer effectively gives up on internals for IVT assemblies as well.

This is also directly based on the precedent in CA1812:

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidUninstantiatedInternalClasses.cs

Lines 53 to 65 in e52a3f1

	// If the assembly being built by this compilation exposes its internals to
	// any other assembly, don't report any "uninstantiated internal class" errors.
	// If we were to report an error for an internal type that is not instantiated
	// by this assembly, and then it turned out that the friend assembly did
	// instantiate the type, that would be a false positive. We've decided it's
	// better to have false negatives (which would happen if the type were *not*
	// instantiated by any friend assembly, but we didn't report the issue) than
	// to have false positives.
	var internalsVisibleToAttributeSymbol = wellKnownTypeProvider.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemRuntimeCompilerServicesInternalsVisibleToAttribute);
	if (compilation.Assembly.HasAttribute(internalsVisibleToAttributeSymbol))
	{
	return;
	}

[sharwell]

If you're forced to disable the rule for a project that uses IVTs because of false positives

This might apply to some cases. However, I can think of entire solutions (StyleCop Analyzers) where projects have IVT attributes but there are no derived types defined in the test projects. The proposal here forcibly disables a rule in all projects with these characteristics instead of using the existing mechanisms to change the rule severity.

This is also directly based on the precedent in CA1812:

I'm not confident this issue is as common as the one in CA1812. I'm also not sure an automatic exclusion in CA1812 was the right approach either; it does seem to be a documented case though.

[mavasani]

IMO, these set of analyzers belong to "solution" level or "workspace" level analyzers that we have talked about in the past. Otherwise, these will either be too aggressive (many false positives) or too restrictive (many false negatives).

Until, we can support such category of analyzers, I believe the approach chosen here and in CA1812 to reduce false positives seems reasonable. I also think we should create an editorconfig option (probably also settable via some MSBuildProperty) to bulk configure all or individual IVT based rules to switch from restrictive to aggressive: https://github.com/dotnet/roslyn-analyzers/blob/main/docs/Analyzer%20Configuration.md. @stephentoub can you please create an issue to track adding such an option and an MSBuild property?

[stephentoub]

@stephentoub can you please create an issue to track adding such an option and an MSBuild property?

Opened #5973

[geeknoid]

Unfortunately, this change makes the rule nearly useless in our environment :-(

The majority of our components have InternalsVisibleTo for tests, yet near 0% of the tests intend to derive from classes defined in the SUT (which I would expect is the case in most code bases). In effect, this change nullifies the diagnostic for most real-world code bases.

We've had a work item open for a long time to explicitly run a build of our sources with InternalsVisibleTo disabled in order to help dead code analysis, and potentially other analyzers like this one. I guess we need to get that change done...

[stephentoub]

I guess we need to get that change done...

And/or help with #5973 to make it configurable whether IVT is taken into account...