Skip to content

Conversation

@baronfel
Copy link
Member

@baronfel baronfel commented Apr 12, 2022

The C# compiler supports a warning wave of 7 now, and we should float that on like was done for .NET 6. This PR does that, in a slightly more maintainable way IMO.

The rules are:

  • AnalysisLevel should track the current TFM if not specifed
  • AnalysisLevel should be set to latest if the current TFM is the 'latest' TFM (as defined by a property that we need to bump)
  • WarningLevel should track the current TFM if not specified
  • WarningLevel shouldn't override the user-provided value
  • WarningLevel should be set to 4 if the project is a .NET Framework project

Fixes #21599 as well

cc @Youssef1313 via dotnet/templating#4580

@ghost ghost added the Area-Infrastructure label Apr 12, 2022
Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the change on SDK side is needed, it doesn't fully resolve the suggestion for templates.

A user might not specify AnalysisLevel or might specify low value for AnalysisLevel to get a smaller set of CAxxxx analyzers to run (due to FPs, rules that user think are noisy, too much new warnings after update, etc).

For compiler warnings, they're almost always correct warnings that users are encouraged to fix.

@baronfel baronfel added this to the 7.0.1xx milestone Apr 14, 2022
@baronfel baronfel marked this pull request as draft April 14, 2022 19:28
@baronfel baronfel force-pushed the bump-analysis-level-for-net-7 branch 2 times, most recently from 72ca9d7 to 99d7a89 Compare April 14, 2022 19:32
@baronfel baronfel force-pushed the bump-analysis-level-for-net-7 branch 2 times, most recently from 71331c6 to 2191734 Compare April 14, 2022 20:59
@jmarolf jmarolf self-requested a review April 15, 2022 16:50
@baronfel baronfel marked this pull request as ready for review April 28, 2022 20:51
@baronfel baronfel force-pushed the bump-analysis-level-for-net-7 branch from 5d4f6b9 to ca9a789 Compare May 5, 2022 01:42
@baronfel
Copy link
Member Author

baronfel commented May 5, 2022

I would love to add a test that computes the uses TFM of the test project to assert that the latest and preview AnalysisLevels (and the warning level) float to the TFMs as we update them. Ideally the test would fail as soon as the TFM is changed from net7.0 to net8.0, so that we know that some update here has to be done.

@cremor
Copy link

cremor commented Jan 17, 2023

@baronfel

WarningLevel should be set to 4 if the project is a .NET Framework project

Why is this rule/condition on the framework type and not on the C# language version?

If I remember correctly, my .NET Framework project which uses an SDK-style csproj file with <LangVersion>10.0</LangVersion> and <AnalysisLevel>latest</AnalysisLevel> used warning level 6 with the .NET 6 SDK. Now with the .NET 7 SDK it only uses warning level 4 again.

Yes, I can explicitly specify the <WarningLevel>, but shouldn't setting <LangVersion> and <AnalysisLevel> be enough to say "I want the latest compiler features and warnings"?

Btw, this also affects documentation, see dotnet/docs#33559

@sliekens
Copy link

sliekens commented Jun 4, 2024

@baronfel I think this PR introduced a regression.

Before this PR, the WarningLevel could be set to match the version number of the EffectiveAnalysisLevel, but this code was removed. Now, setting AnalysisLevel=preview still results in WarnLevel=9999 but any other value for AnalysisLevel will be ignored and the effective WarnLevel is derived from the TFM.

First I thought this was a documentation issue.
dotnet/docs#41295

Now I believe it is a regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WarningLevel is no longer respected in .NET 5.0

5 participants