Skip to content

Enable build time warnings for IDE code analysis rules #34215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 9, 2021
Merged

Enable build time warnings for IDE code analysis rules #34215

merged 2 commits into from
Jul 9, 2021

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Jul 8, 2021

  • Enables build time warnings for readonly, modifier ordering and const
  • Fixes all of the warnings

@pranavkm pranavkm marked this pull request as ready for review July 8, 2021 23:02
pranavkm
pranavkm commented Jul 8, 2021
View reviewed changes
.editorconfig
@@ -195,6 +195,7 @@ dotnet_diagnostic.CA2208.severity = warning
dotnet_diagnostic.IDE0035.severity = warning

# IDE0036: Order modifiers
csharp_preferred_modifier_order = public,private,protected,internal,static,extern,new,virtual,abstract,sealed,override,readonly,unsafe,volatile,async:suggestion
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on https://github.com/dotnet/runtime/blob/main/.editorconfig#L42 (also what the analyzer defaults to)

.editorconfig
@@ -204,7 +205,7 @@ dotnet_diagnostic.IDE0043.severity = warning
dotnet_diagnostic.IDE0044.severity = warning

# IDE0073: File header
dotnet_diagnostic.IDE0073.severity = warning
dotnet_diagnostic.IDE0073.severity = suggestion
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This trips on files from the runtime repo (that have an MIT license) as well as some other code that's copied from that repo. Skipping for now since it seems low value for now.

@BrennanConroy
Copy link
Member

How did you fix all these? Is there a command line tool to do this now?

pranavkm
pranavkm commented Jul 8, 2021
View reviewed changes
eng/targets/CSharp.Common.targets
@@ -27,6 +27,9 @@
'$(ISBenchmarkProject)' == 'true' OR
'$(IsSampleProject)' == 'true' OR
'$(IsMicrobenchmarksProject)' == 'true'">$(NoWarn);CA1416</NoWarn>

<!-- Enable .NET code style analysis during build for src projects. -->
<EnforceCodeStyleInBuild Condition="'$(EnforceCodeStyleInBuild)' == ''">true</EnforceCodeStyleInBuild>
Copy link
Contributor Author

@pranavkm pranavkm Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@BrennanConroy this enables these rules to run on the CLI.

Copy link
Member

Choose a reason for hiding this comment

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

This fixes them when it finds them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, just makes it appear as errors. I used VS to apply the code fix. But presumably once we go thru a few passes of enabling and fixing warnings for individual rules, it's not going to be a big issue. Building in the IDE and on the CLI should produce these errors and they would be localized to new code.

Copy link
Member

Choose a reason for hiding this comment

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

I used VS to apply the code fix.

How long did this take you? Did you have to go project by project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried applying it on the sln file but the fixes wouldn't stick. I was however able to get pretty much all of it covered by opening the MVC and Middleware slnfs, and having it load all transitive dependencies. There were a few strays that the build would catch, but not many.

I'm sure there's a better way to go about it, but it's not super painful.

@pranavkm
Enable build time warnings for IDE code analysis rules
da39793 
* Enables build time warnings for readonly, modifier ordering and const
* Fixes all of the warnings
@pranavkm pranavkm force-pushed the prkrishn/allow-build-time-errors branch from 4bb4bbb to da39793 Compare July 8, 2021 23:56
@pranavkm pranavkm enabled auto-merge (squash) July 9, 2021 16:58
@pranavkm pranavkm disabled auto-merge July 9, 2021 16:58
@pranavkm pranavkm enabled auto-merge (squash) July 9, 2021 16:59
Tratcher
Tratcher approved these changes Jul 9, 2021
View reviewed changes
src/Identity/ApiAuthorization.IdentityServer/src/Options/ClientBuilder.cs
@@ -14,8 +14,7 @@ namespace Microsoft.AspNetCore.ApiAuthorization.IdentityServer
public class ClientBuilder
{
private const string NativeAppClientRedirectUri = "urn:ietf:wg:oauth:2.0:oob";

Client _client;
readonly Client _client;
Copy link
Member

Choose a reason for hiding this comment

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

Visibility modifier?

src/Identity/UI/src/Areas/Identity/Pages/V4/Account/Manage/ResetAuthenticator.cshtml.cs
private readonly SignInManager<TUser> _signInManager;
ILogger<ResetAuthenticatorModel> _logger;
readonly ILogger<ResetAuthenticatorModel> _logger;
Copy link
Member

Choose a reason for hiding this comment

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

Visibility

src/Shared/HttpSys/NativeInterop/HttpApiTypes.cs
@@ -573,7 +573,7 @@ internal struct HTTP_SERVER_AUTHENTICATION_INFO
internal bool DisableNTLMCredentialCaching;
internal ulong ExFlags;
HTTP_SERVER_AUTHENTICATION_DIGEST_PARAMS DigestParams;
HTTP_SERVER_AUTHENTICATION_BASIC_PARAMS BasicParams;
readonly HTTP_SERVER_AUTHENTICATION_BASIC_PARAMS BasicParams;
Copy link
Member

Choose a reason for hiding this comment

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

revert, we should leave the structs alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll clean this up in a follow up.

src/Shared/HttpSys/NativeInterop/HttpApiTypes.cs
Comment on lines +591 to +592
readonly ushort RealmLength;
readonly char* Realm;
Copy link
Member

Choose a reason for hiding this comment

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

revert

@pranavkm pranavkm merged commit 10beca5 into main Jul 9, 2021
@pranavkm pranavkm deleted the prkrishn/allow-build-time-errors branch July 9, 2021 18:21
@ghost ghost added this to the 6.0-preview7 milestone Jul 9, 2021
@martincostello martincostello mentioned this pull request Jul 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tratcher Tratcher Tratcher approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@javiercn javiercn Awaiting requested review from javiercn

@JunTaoLuo JunTaoLuo Awaiting requested review from JunTaoLuo

@Pilchie Pilchie Awaiting requested review from Pilchie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.0-preview7
Development

Successfully merging this pull request may close these issues.
4 participants
@pranavkm @BrennanConroy @halter73 @Tratcher