Update net analyzer version

[jonathanou]

Justification

The current latest stable .NET analyzer is 7.0.4, we should upgrade our dependency so consumers of our nuget package can get the latest analysis rules.

The list of rules that the .NET analyzer checks depends on <AnalysisLevel>. By default, <AnalysisLevel> is set to "latest", but that means latest that your current SDK supports. Thus for projects building in .NET 6, the default <AnalysisLevel> is 6.0, even if there is a newer level available from the nuget package.

At the same time, it is possible for .NET 6 project to specify <AnalysisLevel>7.0</AnalysisLevel> to get the .NET 7 code analysis rules without upgrading to .NET 7 SDK—this allows consumers to take advantage of newer code analysis rules sooner.

Implementation

• Update Microsoft.CodeAnalysis.NetAnalyzers to 7.0.4.

Testing

Built a nuget package locally and updated ASW to use the local nuget. After setting <AnalysisLevel>7.0</AnalysisLevel>, I started to get new warnings.

[jryckman]

I'm a little worried about updating the Roslynator.Anazlyers because the last time I did it, some of the rules stopped working the way we wanted them to. I think we were relying on existing behavior which was changed in subsequent versions. Some of our anaylzers stopped reporting warnings or others started throwing warnings where they didn't used to. My experience was years ago though, so maybe things have changed. I tested this on the entire NXG stack and saw bad behaviors.

If we were going to update this, I'd like to make sure it gets tested on our largest codebases to ensure it retains proper behavior.

[nick-beer]

This touches on something I've been wanting for a while now. I think we should remove our dependency on the microsoft analyzers and instead take whatever we're given from the SDK we're using. Taking the dependency in this analyzer forces us to make these updates as new analyzers are released, rather than just uptaking the new SDK.

As for the roslynator analyzers - are we really even using those? I'm pretty sure we exclude them entirely in ASW. I'd be in favor of also removing our dependency to those analyzers.

[jryckman]

We used to use the Roslyn analyzers quite heavily. I don't know the state of the current C# standards though.

[nick-beer]

@jryckman - Looking back through the ASW history - I don't see any evidence of us using the analyzer assemblies delivered through the Roslynator NuGet package. They've never been enabled for the .NET 6 build, and when I tried enabling them for the .NET 6 build, the number of warnings and errors was huge - indicating to me that they had never been on in a meaningful way. Additionally, we don't have any suppressions or similar for the diagnostics from those analyzers - another indication to me that we've never actually used them.

You mentioned the "Roslyn analyzers" - which I'm not sure how to interpret. We've definitely used the analyzers provided by Microsoft that are built on the Roslyn SDK. I'm just not sure that we've ever used the analyzers supplied by Roslynator - which this package depends upon.

[jryckman]

@nick-beer Sorry about the typo. I meant Rosylnator. It might be that I used the Roslynator analyzers to clean up the NXG monorepo at some point because there were certain rules that were useful in getting our code to where we wanted it to be, but perhaps I never turned them on permanently. If we aren't using them now, we should get rid of it for simplicity.

[jonathanou]

I think we should remove our dependency on the microsoft analyzers and instead take whatever we're given from the SDK we're using.

@nick-beer , I had thought of that but because we only upgrade our SDK every two years, we don't get to use new warnings that are released in the non-LTS SDK.

With the nuget package approach, we are still able to get the latest CA rules. By setting the analysis level to a higher level, we'll be able to guard against new warning violations much earlier. This will make our next LTS SDK upgrade easier as there are less warnings to fix.

[nick-beer]

I had thought of that but because we only upgrade our SDK every two years, we don't get to use new warnings that are released in the non-LTS SDK.

@jonathanou I think the real problem here is that we do not (cannot) update our SDK with every release. The SDK that we use should not be tied to the runtime version our applications require (ie - we should be building with SDK version 7.0 and targeting net 6.0/LTS for runtime).

Once we're able to update our SDK without impacting the runtime version we target, we again won't need (or want) the analyzer versions we use to be defined by this analyzer package.

Additionally, we should probably update the props/targets files in this package to ensure the analysis levels we want/expect for NI are set consistently across the repos that use this analyzer package.

[jonathanou]

@jonathanou I think the real problem here is that we do not (cannot) update our SDK with every release. The SDK that we use should not be tied to the runtime version our applications require (ie - we should be building with SDK version 7.0 and targeting net 6.0/LTS for runtime).

Once we're able to update our SDK without impacting the runtime version we target, we again won't need (or want) the analyzer versions we use to be defined by this analyzer package.

@nick-beer What is the reason for not being able to update the SDK every year?

Additionally, we should probably update the props/targets files in this package to ensure the analysis levels we want/expect for NI are set consistently across the repos that use this analyzer package.

Yes, there will be other props/targets file changes needed (like setting <AnalysisMode>), but I'm saving that til later when I'm done with ASW and we know every property we want to set.

If we don't update the Net analyzer right now though, then the <AnalysisLevel> needs to be to 6.0 since that's the latest that our Net analyzer supports. If we can/want to update the nuget package, then I prefer we set it to 7.0 to get the .NET 7 CA rules too.

FYI for my team's project, we've been using the latest 8 preview net analyzer and it's worked well for us at level 8.0.

[jonathanou]

@nick-beer Sorry about the typo. I meant Rosylnator. It might be that I used the Roslynator analyzers to clean up the NXG monorepo at some point because there were certain rules that were useful in getting our code to where we wanted it to be, but perhaps I never turned them on permanently. If we aren't using them now, we should get rid of it for simplicity.

@jryckman for my project I've been using Roslynator rules and they're working well. However you mentioning newer versions had broken behaviors did remind me that we were seeing roslynator rules not breaking the build, despite seeing errors in Visual Studio.

I had assumed it might have been something wrong with our build, but when I have some time I will drop our build down to the version NI analyzer is currently using to see if that "fixes" it.

[nick-beer]

What is the reason for not being able to update the SDK every year?

I think the biggest reason is that we use global.json as a proxy for what runtime version we should download/install on our pipelines. Once we break that link so that we can test against the runtime version we care about, we should be able to update our SDK at will (including no longer pinning the SDK to a specific revision).

[nick-beer]

@jonathanou - thinking about this some more, I'm fine with this change going ahead. I would like (at some point in the future) for us to break this dependency and allow the SDK to determine the analyzers we use, but that doesn't have to be now. Additionally, if you'd like to keep the Roslynator dependencies in place, that's fine as well - though I know we don't actually use them in ASW.

[jonathanou]

@jryckman for my project I've been using Roslynator rules and they're working well. However you mentioning newer versions had broken behaviors did remind me that we were seeing roslynator rules not breaking the build, despite seeing errors in Visual Studio.

I had assumed it might have been something wrong with our build, but when I have some time I will drop our build down to the version NI analyzer is currently using to see if that "fixes" it.

@jryckman , once I dropped down to 4.1.1, all the configured rule violations correctly failed the build. I've reverted the Roslynator version upgrade change.

I tried looking for an issue on the Roslynator github repo. The closest matching one that I could find was dotnet/roslynator#1158, but that issue is for the VS extension, not the analyzer nuget package.

Once we move to .NET 8, I will try upgrading again to the latest version to see if the problem is fixed. Until then, let's stay on 4.1.1.

[jryckman]

@jonathanou Glad you were able to check and verify the Roslynator change. I want to make sure that the updates that we put in can be consumed by ASW at the very least in addition to the other C# development repos. @nick-beer confirmed that ASW isn't using Roslynator, but are all of the other dependencies good? Is there outstanding work based on @nick-beer 's comments?

Are we at a point that we can submit this?

[nick-beer]

It's still my opinion that we should remove all the dependencies that this package has. We don't use the Roslynator analyzers in any of the major repos consuming this package (right? are there others I don't know about?), we don't configure it in any way within this package, and it's something we have to then maintain. We should not have a dependency on the SDK analyzers either - we should just use what we get from the SDK.

However, this is a simple change that does improve the current situation, and Jonathan is already doing a lot of work to fix code violations based on the .NET 7 analyzers - so IMO it's fine for this to go in as is.

[jonathanou]

Are we at a point that we can submit this?

@jryckman , yes I think so.

It's still my opinion that we should remove all the dependencies that this package has. We don't use the Roslynator analyzers in any of the major repos consuming this package (right? are there others I don't know about?)

My team's project does use it to control the coding convention, because I got really tired with leaving feedback asking developers to fix it. Our team is constantly moving to different projects and taking on new team members/contractors and I'm finding myself having to repeat the same feedback frequently. (this is actually my main motivation to start doing work on all the analyzer stuff.)

I don't have a strong opinion on whether NI's analyzer should include Roslynator though. It would be nice to enforce a consistent style and formatting company-wide, but given Roslynator's bugs and the NI analyzer's current lack of configured Roslynator rules, it does seem removing it won't really affect anything. If needed, we can always add it back in with a properly configured list of rules (though applying to ASW is going to suck since I'm sure there are many violations).

However, this is a simple change that does improve the current situation, and Jonathan is already doing a lot of work to fix code violations based on the .NET 7 analyzers - so IMO it's fine for this to go in as is.

Yeah that's my preference too; being able to enforce .NET 7 warnings in ASW is more urgent right now. I can come back and clean up the dependencies later when I have time to test it more thoroughly.

[jryckman]

I went ahead and merged this in. Let's keep it going and make sure we can tidy this up to the point where we're happy with the latest versions (removed dependencies we don't need, etc).