Visibility analyzers proposal

[j3parker]

In #326 @grant-cleary added an analyzer to restrict the visibility of a type that's registered for dependency injection.

This kind of problem has be a common cause for new analyzers (and more have been proposed but not implemented.) I think it's time to start planning a general and easier-to-use approach.

A general solution for visibility could take the form of adding annotations to a class/etc. that specified "who" could use it.

API

• [LimitedVisibilty( "Some rationale" )]: applies to various declarations; triggers our analyzer to care about the symbol. An optional argument for rationale will influence any generated diagnostics.
• [VisibileToType( "D2L.Foo" )] Allows D2L.Foo to use this symbol (minor abuse of the word "type".)
• [VisibileToAssembly( "D2L.Foo" )]: similar in intent to [InternalsVisibleTo]
• [VisibleToNamespace( "D2L.Foo" )]: applies recursively to sub-namespaces
• [VisibleTo*] annotations combine with OR logic
• [LimitedVisibility] should (as much as possible) reduce your visibility to the next lowest level (hand-waving) e.g. public -> internal, internal -> private, except for the exceptions listed in [VisibleTo*] annotations
• Consequently [LimitedVisibility] without any [VisibleTo*] should be an error, with a fixit to lower visibility + remove annotation.
• [VisibileTo*] without [LimitedVisibility] should also be an error
• [LimitedVisibility] on private things could make sense but is probably simpler to just block to start (it doesn't seem likely it'd be helpful.)
• public things may be effectively private or internal WRT other assemblies or non-parent classes/structs: perhaps consider effective/root visibility (TODO: look up spec for terminology.)

Examples

[LimitedVisibility( "This interface is an implementation detail. Please go through blerg blerg instead" )]
[VisibleToType( "D2L.Foo.Bar" )]
[VisibleToType( "D2L.Foo.Baz" )]
internal interface IFoo { /* ... */ }

This limits IFoo to the two listed types. This would be useful in larger assemblies where internal may be too permissive. The diagnostic would include the string from [LimitedVisibility] in its message to the user.

[LimitedVisibility]
[VisibleToAssembly( "D2L.SomeAssembly" )]
public interface IFoo { /* ... */ }

This limits IFoo to things within its assembly (as if it was internal) and to the other listed assembly. This is effectively a more-limited version of [InternalsVisibleTo].

The rationale for also allowing things within IFoos assembly to see/use IFoo is for making this behave consistently/on a continuum with the built in visibility attributes. If we didn't do this, you could always add [VisibleToAssembly( "MyAssembly" )] to get the visibility back. While that seems more flexible I'm not sure it's worth the cognitive overhead.

[LimitedVisibilty( "I want to make this internal, stop using it!" )]
[VisibleToAssembly( "D2L.Something" )]
[VisibleToType( "D2L.Foo" )]
[VisibleToType( "D2L.Bar" )]
[VisibleToNamespace( "D2L.SomeNamespace.Lol" )]
public interface IFoo { /* ... */ }

In this example we're trying to make IFoo internal, but we have to do some work. Different ways to whitelist (with varying degrees of specificity) could help maintain forward momentum.

If this was a very nasty thing (e.g. [JsonParamBinder]) we could register a fixit for the not-visible diagnostic and filter based on the symbol that was supposed to be not visible.

Benefits of a general solution

• Not needing to write new analyzers would be great
• Easier to use
• Consistent tooling (e.g. diagnostic messages)
• Removing hard-coded whitelists from the analyzer

Existing analyzers

These are the existing analyzers that could have overlap with a general solution:

DangerousMethodUsages

This analyzer allows us to limit the usage of dangerous methods.

• There is an audited/unaudited annotation system to suppress diagnostics.
• There is no fixits (because this is a general solution) and no custom diagnostics.
• It is possible to restrict usage of types that we don't own (it doesn't require adding an annotation to their decls)
• It inspects InvocationExpressions (a backdoor would be to use a MemberAccessExpression to store the invocable method into a var and then invoke the var.)

Conclusion: The audit/unaudited and the external types are not relevant to the visibility analysis - this should probably stay as it is and continue to be used in the way it's currently used.

JsonParamBinderAnalyzer

This restricts usage of a bad serialization attribute and encourages use of a newer good (but poorly named) replacement.

• Hard coded whitelist for legacy things
• Has a fixit to use the new attribute
• Inspects AttributeSyntax

Conclusion: This could be merged into a general solution.

OldAndBrokenServiceLocatorAnalyzer

Restricts usage of IServiceLocator and friends

• Hard coded whitelist for legacy things
• No fixits because there is no trivial fix
• Inspects IdentifierNameSyntax

Conclusion: In principal this may be movable into a general solution.

CorsHeaderAppenderUsageAnalyzer

Restricts usage of a type that we're worried about being misused but can't exist in one assembly easily.

• Hard coded whitelist
• No fixits because there is no feasible fix
• Inspects ConstructorDeclaration, ObjectCreationExpression

Conclusion: This could be merged into a general solution

[grant-cleary]

I like the idea of registering a fixit for specific cases (e.g. the [JsonParamBinder]). While we might hope that we rarely need to lock something down that aggressively, I think it's a safe assumption that those cases will continue to arise. In the general case, I definitely think it makes sense for the analyzer to ignore usages of IFoo (presuming IFoo is public) in its parent assembly.

So really, if we can build in the ability to say "Okay, we really don't want X used EVER except in Y, no exceptions," I think that's ideal. I wouldn't want it to get in the way of a decent general solution though.

[j3parker]

Some feedback from IRL conversations:

It'd be good to emphasize this would mostly be used for cleaning stuff up: the standard visibility accessors and their semantics are still preferred, and we should consciously design our assembly/DI boundaries (e.g. the public things should be usable generally.)

EDIT: in other words "what if this is used to make the code worse?" (e.g. switching internal to public, but with restrictions.) We concluded that was likely but we could detect new instances of that in PRs and add hint comments/send passive alerts to us to see how people use it.