Detect substituting for internal members without InternalsVisibleTo

[dtchepak]

From nsubstitute/NSubstitute#496, having a test assembly with visibility of internal members of assembly under test, but without InternalsVisibleTo("DynamicProxyGenAssembly2"), causes errors when running the tests.

It would be good to catch this case, similar to NS2003.

From Robert's comment on NSubstitute@496:

Referring to NS2003, there are no checks on members, only on the type itself. This is the analyzer method in question AnalyzeTypeAccessability

The reproduction on that issue is a good test case to illustrate the problem (just having a type with internal members local to the test assembly with trigger the issue).

I'm not sure what category this should belong to. Could be NS5xxx as a general usage problem, although it is also sort of similar to NS1xxx which covers non-virtual substitution, although in this case it is a non-substitutable member due to internal. The easiest is probably to put it in NS5xxx, but maybe we should also consider adapting the NS1xxx category to be something to do with "non-substitutable members" and include this?

Draft rule page:

NS2003

CheckId	NS????
Category	????

Cause

Substituting for an internal member of a class without proxies having visibility into internal members.

Rule description

A violation of this rule occurs when using NSubstitute features like Returns(), Received() etc. with an internal member of a class, but without giving NSubstitute's proxy assembly access to internal members.

How to fix violations

There are several ways to fix this:

• change the visibility of the member being tested from internal to public
• change the visibility of the member being tested from internal to protected internal
• expose your type being tested to DynamicProxyGenAssembly2 using:

[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")]

How to suppress violations

...

[tpodolak]

@dtchepak regarding diagnostic message for new analyzer, do you think this is enough
Internal member {member name} can not be intercepted. ?

When it comes to the diagnostic id, I think this could be another NS1xxx - we would have to change the category name/description from "Non virtual substitution" to sth better - any thoughts about that?

And the last one question, for non-virtual substitution we have three different diagnostics one for Returns, second for Received and third for When like methods - should we go with same convention for internal members detection as well?

[dtchepak]

Maybe Internal member {member name} can not be intercepted without InternalsVisibleToAttribute.? It doesn't make much sense on its own, but I think it will prompt people to read more to learn how to fix it.

I agree NS1xxx seems the right place for it. Best name I can come up with at the moment is "Non-substitutable member", but I am open to better suggestions! 😄

I'm not sure for your last question. Is a single diagnostic easier to implement? Only advantages I can think of for separate are following convention, and allowing finer grained suppression. Neither of those is that compelling, so if a single one is easier to implement, then I'd lean towards that. Also I think this will be less common that the non-virtual case (if that makes a difference).

[tpodolak]

@dtchepak single vs multiple diagnostic is more or less the same effort. I was rather wondering if it is worth to use multiple diagnostics in this particular case. But after all, I decided to go with one diagnostic only