Allow configuration of safe API misuses

[dtchepak]

In some cases the NSub API can be intentionally used in a questionable manner to get a specific result.

A common example I have seen is "mocking" extensions methods. For example:

public static class MyExtensions {
  public static ICounter Counter { get; set; }
  public static int GetCount(this object blah) {
    return Counter.GetCount(object);
  }
}

We can then "mock" this extension method by cheating:

MyExtensions.Counter = Substitute.For<ICounter>();
// ...
widget.GetCount().Returns(42);

This will work because MyExtensions.GetCount() invokes ICounter.GetCount(widget), so Returns will successfully mock the ICounter call (rather than the static, unmockable MyExtensions.GetCount()). NSubstitute.Analyzers correctly reports this as "NS001 | Member GetCount can not be intercepted. Only interface members and virtual, overriding, and abstract members can be intercepted." Which would normally be fine, but in this case we're using this to intercept the underlying call instead.

@tpodolak's suggestion is to use a configuration file to allow developers to whitelist specific calls in their project. We still need to flesh out this idea, but here are some rough notes:

• Should be able to get analysers to ignore a specific error code for a specific member. e.g. ignore NS001 errors for MyProject.MyExtensions.GetCount.
• How to handle overloads? Can probably just support member name for now and ignore the complexity of overloads.
• Possibly support basic patterns/wildcards? e.g. ignore NS001 for MyProject.MyExtensions.Get*?
• Possibly consider making the errors more fine-grained. For example, NS1xx series could be used for attempts to sub non-virtuals. NS101 can be for class methods, NS102 for class properties, NS111 for static class methods, NS120 for extension methods etc. Then we could specifically ignore NS111 for MyProject.MyClass.* in the case we know all static members delegate to an underlying virtual method, but we still want to catch any instances of trying to substitute non-virtual class methods. (This is just an idea, it is probably sufficient just to do the basic whitelist instead.)

[tpodolak]

I was thinking a bit about how to specify members to ignore, and from my perspective, we shouldn't reinvent the wheel. I would go with the same format which is used for suppressing warnings in suppression files. This gives us quite good flexibility in terms of members specification e.g:

• method - M:MyNamespace.IFoo.Bar(System.Int32,System.Int32)~System.Int32
• constructor - M:MyNamespace.Foo.#ctor
• property - P:MyNamespace.Foo.Bar
• indexer - P:MyNamespace.IFoo.Item(System.Int32)
• type - T:MyNamespace.IFoo
• namespace - N:MyNamespace
• field - F:MyNamespace.Foo.MyField
  The nice thing about this format is the fact that it supports overloads, so
  M:MyNamespace.IFoo.Bar(System.Int32,System.Int32)~System.Int32
  will match different member/symbol than
  M:MyNamespace.Foo.Bar(System.Decimal,System.Decimal)~System.Int32
  Moreover, Roslyn provides a functionality for parsing this syntax, so we only would have to read the file and exclude ignored members for given rule.
  More about this format can be found here:
  https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/xmldoc/processing-the-xml-file
  Unfortunately, this syntax doesn't support wildcards but I think we can go without them (as you basically can exclude entire type or namespace from analysis). What do you think guys?
  The other matter is an analyzer file. I would go with json as this is a standard now but what would be the schema for it? Any thoughts?

[dtchepak]

@tpodolak Sounds good to me! Some random thoughts:

• Happy to do without wildcards. This syntax already lets us suppress by method, type, namespace etc, that should be plenty.
• Roslyn generates this as well as parses it, correct? So we could have a quick fix to exclude for a particular member? (Having to manually type in M:MyNamespace.IFoo.Bar(System.Int32,System.Int32)~System.Int32 would be onerous, compared with a looser "exclude `MyNamespace.IFoo.Bar" format)
• The built-in suppression mechanism is pretty close to what we want, but it only works from line that contains the error, not the target of the error? For example:

// We'd like:
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("NSubstitute.Usage", "NS001:Non-virtual setup specification.", Justification = "<Pending>", Scope = "type", Target = "~T:MyNamespace.MyExtensions")]

// But it needs 
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("NSubstitute.Usage", "NS001:Non-virtual setup specification.", Justification = "<Pending>", Scope = "member", Target = "~M:MyTests.MyFixture.MyTest")]

• Schema-wise JSON is probably expected I guess. Maybe something like this?

{
  "suppressions": [
        { "target": "T:MyNamespace.MyExtensions", "codes": ["NS001","NS002"] },
        { "target": "M:MyNamespace.IFoo.Bar(System.Int32,System.Int32)~System.Int32", "codes": ["NS001"] }
    ]
}

[tpodolak]

The built-in suppression mechanism is pretty close to what we want, but it only works from line that contains the error, not the target of the error?

@dtchepak the suppressions based on json file will have to be implemented manually, we will just use Roslyn ability to parse the target syntax and get the symbols based on that, the rest will be on us.
As far as I know, it is not possible to hook up into the existing suppression mechanism.

Roslyn generates this as well as parses it, correct? So we could have a quick fix to exclude for a particular member?

Yes, Roslyn can do both, so I think it is possible to write a quick-fix for adding suppressions to the json file. Although you probably will get this ugly pop-up window indicating that project has changed if we will add json file to the project via quick-fix (in case user didn't add it). I will try to write some PoC for quick-fix which modifies/creates the json file.

[dtchepak]

Although you probably will get this ugly pop-up window indicating that project has changed if we will add json file to the project via quick-fix (in case user didn't add it)

Does it need to be added to the project? Or just dumped in the project root? (i'm assuming will be at project-level rather than solution-level?)

[tpodolak]

@dtchepak It must be added as an additional file to the project so as I can access it from the analyzer. One way or another as for now it is impossible to properly add additional file to the project via Roslyn. We basically encountered the same bug as the one reported by StyleCops guys dotnet/roslyn#4655. So as for today the user have to create empty file on its own and set the proper build action as well. Once the file is there I will be showing the quickfixes for suppressions.

[dtchepak]

@tpodolak Can we include the file by default with the nuget package?

[tpodolak]

@dtchepak I've tried this already but it didn't work, same for custom build target/props file. All of the approaches seem not to copy files to the project but reference them from the nuget package location, which makes the nsubstitute.json file uneditable. I've looked at some existing packages which used to add config files to the projects (for instance NLog.Config) and those packages behave in the same weird way. So to sum up I wasn't able to implement this correctly - if you guys have time please take a look, I am out of ideas ;].
Regarding suppressions, I've implemented them for non-virtual call diagnostic so far. Does it make sense to make all other rules suppressible? Are there any other safe API misuses worth implementing?

[dtchepak]

@tpodolak For suppressions i'm not sure if there are other safe API "misuses". Should be fine to wait until other cases come up (unless it is trivial to implement).

Is it possible to ass a .cs or .vb file to the project (rather than a config file)? Or does that face the same problems? If so then we could try something like encoding with attributes as per AssemblyInfo.cs, or specify in a valid class structure (or even as json in a comment 😂 ).

[tpodolak]

I didn't check if there were issues with adding vs/cs files to the project but I would avoid do the hacks in order to make the suppressions fully automatic. I think as long as we document how to include the nsubstitute.json file in the project we should be fine. Either way the most complicated part (which is creating proper suppression entry) will be done by fix-provider so that is an acceptable solution at least in my opinion. When it comes to additional API misues I will add the one which is about to be mered: non-virtual calls in When...Do. As far as I can tell this is basically the same use case as in Returns/ReturnForAnyArgs etc