Code analyzers feature

[zvirja]

Just thought about your comment that NSubstitute syntax has issues with non-virtual members and realized that NSubstitute could be shipped with Roslyn analyzers. You can take a look at xunit as an example. They ship a set of analyzers for their assertion library, to use the assertions correctly (e.g. Assert.True(x) instead of Assert.Equal(true, x)).

You could also create analyzes for the most known issues:

• Returns() after a non-virtual method.
• Improperly used Arg.Any().

That would be awesome as it will perform analysis during the compilation and produce warnings if any issue found. On other hand, user could easily suppress warnings, if they are not relevant.

@dtchepak @alexandrnikitin Have you already thought about this idea?

[dtchepak]

@zvirja This would be awesome! I have no idea about Roslyn stuff, I'll see what I can work out from the xunit examples.

(This issue replaces #105 now we have a proposal for fixing it)

[blairconrad]

@dtchepak, I'm sure the xunit examples are great, but you might also enjoy looking at the FakeItEasy Analyzer source. Of course the configuration syntax is different, but we (among other things) detect attempted configuration of non-virtual members!

[tpodolak]

@dtchepak I did a PoC for an analyzer detecting setup of non-virtual calls. You can find it here. It is able to detect non-virtual calls for following syntaxes:

substitute.Bar().Returns(1);
substitute.Bar().Returns<T>(1);
SubstituteExtensions.Returns(substitute.Bar(), 1);
SubstituteExtensions.Returns<T>(substitute.Bar(), 1);
substitute.Bar().ReturnsForAnyArgs(1);
substitute.Bar().ReturnsForAnyArgs<T>(1);
SubstituteExtensions.ReturnsForAnyArgs(substitute.Bar(), 1);
SubstituteExtensions.ReturnsForAnyArgs<T>(substitute.Bar(), 1);

code analysys for indirect substitute usage e.g

var substitute = NSubstitute.Substitute.For<Foo>();
var returnValue = substitute.Bar();
returnValue.Returns(1);

is rather difficult to process as it requires creating a data flow analysis, that is why I don't analyze those cases yet - I just make sure that I don't report false positives. If you are interesed we can move this code to NSubstitute repository - with proper PR, CodeReview etc. Of course some more manual testing is required before potential release. I have quite a few tests in place, but probably it would be good to have a tests against different versions of VS

[dtchepak]

Hi @tpodolak,

This looks awesome!

If I fork this to the NSubstitute org are you happy to continue working on it there? Or we can keep it under your name if you prefer (seeing you've done all the work on it 😄). Please let me know how you'd like to run/incorporate the project in terms of releases, feature requests, collaborators/repo access etc.

Let me know if there is anything I can help out with in the project (more than happy to review the code, but please keep in mind I have no Roslyn experience so I may not be very insightful 😂 ).

[tpodolak]

Hi @dtchepak
I am fine with forking this to the NSubstitute org and continuing work in there. I think the project will get more traction and possibly more contributor in here rather than in my repository. However, I probably would have to have write access to the repo in order to work faster (I assume it will be in separate repo, not the part of the actual NSubstitute). When it comes to the releases I think it would be best to release it as a separate package in the nearest future - once we have some stability we might make it a part of the NSubstitute package itself - just like it is done for xunit or fakeiteasy.

[dtchepak]

@tpodolak Forked to https://github.com/nsubstitute/NSubstitute.Analyzers. You've got full access -- I consider it as your repo but it's good to have it in the NSubstitute organisation so this stuff is kept together. 😄 Welcome to the team! 😄

[dtchepak]

Are FakeItEasy analysers shipped in the main package? There are separate packages listed?

[blairconrad]

@dtchepak, yeah, the FakeItEasy Analyzers are shipped in separate packages, but we do include the source in the main FakeItEasy repo.
Part of the reason we keep the packages separate is that someone working in VB only wouldn't want the C# analyzer and vice versa.

[dtchepak]

Thanks @blairconrad! 😄

[tpodolak]

@dtchepak thanks for the warm welcoming. One question about building the fork on CI. Do you have some organization account on AppVeyor or TravisCI where the analyzer build should be configured or I rather should build that on my AppVeyor account?

[dtchepak]

@tpodolak We build using https://ci.appveyor.com/project/NSubstitute/nsubstitute. I'm not sure how it all works to be honest. 😕 😊 @alexandrnikitin are you able to assist please?

[alexandrnikitin]

@tpodolak @dtchepak I think you should already have access to it if you login via GitHub and choose NSubstitute in account field. Let me know if it doesn't work, I'll add you by your email. I've enabled the NSubstitute.Analyzers repo in AppVeyor. CI Link

[tpodolak]

Build works as expected. I am about to push first beta package to the nuget (unfortunatelly without GH-1), should I push it as an organization or just me as owner?

[alexandrnikitin]

@tpodolak I think it's fine to push the package as organization (especially beta version for testing). What's your username on Nuget? I guess we need to add you as an owner. cc @dtchepak

[tpodolak]

@alexandrnikitin Nuget username is the same as here - tpodolak. One more thing, I was wrong about the build, I mean the build is running but there are some missing env variables there - see this pr nsubstitute/NSubstitute.Analyzers#3. I can fix it but I cannot access the settings page. Can you grant me an access to the appveyour build?

[dtchepak]

@tpodolak @alexandrnikitin I just created a NuGet organisation for NSubstitute and attempted to give everyone access. @tpodolak I've also tried to give you settings access to AppVeyor. Please let me know if there is any problem with accessing either NuGet or AppVeyor.

[dtchepak]

We have the first analysers release!

https://www.nuget.org/packages/NSubstitute.Analyzers.CSharp/
https://www.nuget.org/packages/NSubstitute.Analyzers.VisualBasic/

Will close this issue; future analyser requirements can be added to that project:
https://github.com/nsubstitute/NSubstitute.Analyzers

(I wonder if it would be better to keep all issues here instead? 🤔)