Change to allow style warning to show up in build and fix all unused … #5601

jgonz120 · 2024-01-18T21:24:57Z

Bug

N/A

Fixes:

Regression? Last working version:

Description

We currently have a set of code analyzer rules but they aren't checked during builds, this can lead to PRs with comments like "remove unnecessary using statement". In this PR I've enabled enforcing the code styles in build and removed the warning for several existing issues to help reduce the scope of this PR. So, while there are 466 file changes, they are only removing unused using statements.

In the future you would be getting a build error in the IDE informing you about the using statements. The most difficult part of this was identifying the using statements where were conditionally used based on the framework selected. I'm not certain how we want to treat those as I saw several variations on how they were previously handled.

Ignored warnings

CS1591: Missing XML comment for publicly visible type or member
IDE0076: Remove invalid global 'SuppressMessageAttribute'
CS1573: Parameter has no matching param tag in the XML comment
CS1574: A string passed to a cref tag referred to a member that is not available within the current build environment.
CS1587: XML comment is not placed on a valid language element
CS1572: XML comment has a param tag for a parameter that doesn't exist
CS1584: One of the parameters passed to a tag for documentation comments has invalid syntax
CS1658: Warning text for an error that was overwritten as a warning. The errors were compiler errors found within xml comments.
CS0419: Cref in comment has an ambiguous reference

PR Checklist

…using statements

…g it." This reverts commit 8839dcd.

zivkan · 2024-01-22T02:57:05Z

Directory.Build.props

+  <PropertyGroup>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
+    <NoWarn>$(NoWarn);CS1591;IDE0076;CS1573;CS1574;CS1587;CS1570;CS1572;CS1584;CS1658;CS0419;IDE0055;EnableGenerateDocumentationFile</NoWarn>


We already set common project properties, such as NoWarn, elsewhere in our build scripts: https://github.com/search?q=repo%3ANuGet%2FNuGet.Client+NoWarn+path%3A**%2F*.props&type=code

I'd rather we kept such build properties in a single place, even if it's not following conventions that most other .NET repos use (the file is also imported immediately before this new property group)

However, I think NoWarn on code analyzers is functionality equivalent to disabling them, right? We won't see the violations anywhere, not even underlined in VS. Roslyn implemented a better way to configure analyzers in the .editorconfig file, where you can set the analyzer to info level (or some other option), which shouldn't cause build failures, but will still visibly underline the code in the IDE.

However, can we also consider keeping the analyzers, and instead suppress all existing violations, to prevent new violations from being added?

Disabling an analyzer (IMO preferably though .editorconfig) for analyzers we really don't ever want is a good idea, but does not appear to be intentional given this PR's title and description. For analyzers that we think are a good idea, but too much to fix all existing violations in a single PR, I hope global or inline suppressions will allow us to use the analyzer to prevent further violations.

Having written what I did, I believe that @nkolev92 dislikes huge commits that touch many files without functional changes, because it harms his debugging workflow. I rarely use git history to investigate bugs, and I've never used git bisect. So, while I'd prefer to optimize for long term code maintenance, we should see what middle ground we can find with people who want to avoid the types of changes I porposed.

In the past when we added analyzers, @kartheekp-ms should be able to help out with tihs, we put all the suppressions in globalsupressions.
It basically allows us to keep the analyzers and it does not change code that's not affected.

Re: Additional changes, my argument tends to revolve around git blame and changing lines that don't need change. As is the PR just has removals, and potentially additions with global suppressions, so I have no concerns with that.

zivkan · 2024-01-22T03:11:06Z

src/NuGet.Core/NuGet.Commands/TrustedSignersCommand/TrustedSignersCommandRunner.cs

 using NuGet.Protocol;
+#endif
+using NuGet.Packaging.Signing;


Personally, I'd rather see the conditional using statements to be moved after the unconditional statements, so that they're all one continuous block. This change breaks the "keep using alphabetical" rule.

Also, I'm curious if someone modifies the file and needs to add a new using, will the intellicode auto fix add the new using statement alphabetically or not?

My comment applies to everywhere that conditional using statments are added, but this is the first one I've seen that makes the usings non-alphabetical.

zivkan · 2024-01-22T03:16:24Z

src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/Rfc3161TimestampUtils.cs


 #if IS_DESKTOP
 using System.Security.Cryptography.Pkcs;
+using System.Reflection;
+using System.Runtime.InteropServices;


The namespaces within the #if IS_DESKTOP block are not sorted alphabetically.

zivkan · 2024-01-22T03:31:37Z

Directory.Build.props

@@ -9,6 +9,11 @@
    <Import Project="build\config.props" />
    <Import Project="build\common.project.props" />
  </ImportGroup>
+  <PropertyGroup>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>


Does this make git format in CI redundant? If so, we should remove it from the pipeline:

NuGet.Client/eng/pipelines/templates/Static_Source_Analysis.yml

Lines 77 to 81 in bd0764b

- script: dotnet format whitespace --verify-no-changes NuGet.sln

name: dotnetFormatWhitespace

displayName: Check whitespace formatting

continueOnError: ${{ parameters.isOfficialBuild }}

condition: succeededOrFailed()

We already set it to true:

NuGet.Client/build/common.project.props

Line 113 in bd0764b

<EnforceCodeStyleInBuild Condition=" '$(EnforceCodeStyleInBuild)' != '' " >true</EnforceCodeStyleInBuild>

This was already attempted before: d888411

It doesn't fully work anyways. There are many bugs: https://github.com/dotnet/roslyn/issues?q=is%3Aissue+EnforceCodeStyleInBuild+is%3Aopen

In particular: dotnet/roslyn#49439 and dotnet/roslyn#41640

Tldr; style analyzers don't quite work if the documentation file is not generated, but there are many places in our builds where we explicitly disable the documentation files so that they don't get packed.

nkolev92

EnforceCodestyleInBuild is already enabled, but does not fully work because of roslyn limitations.

Unfortunately it's not the most reliable thing at this point.

We should also try not to suppress analyzers, but rather suppress violations and leave the analyzers on for future changes.

nkolev92 · 2024-01-22T17:58:38Z

Directory.Build.props

@@ -9,6 +9,11 @@
    <Import Project="build\config.props" />
    <Import Project="build\common.project.props" />
  </ImportGroup>
+  <PropertyGroup>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>


We already set it to true:

NuGet.Client/build/common.project.props

Line 113 in bd0764b

<EnforceCodeStyleInBuild Condition=" '$(EnforceCodeStyleInBuild)' != '' " >true</EnforceCodeStyleInBuild>

This was already attempted before: d888411

It doesn't fully work anyways. There are many bugs: https://github.com/dotnet/roslyn/issues?q=is%3Aissue+EnforceCodeStyleInBuild+is%3Aopen

In particular: dotnet/roslyn#49439 and dotnet/roslyn#41640

Tldr; style analyzers don't quite work if the documentation file is not generated, but there are many places in our builds where we explicitly disable the documentation files so that they don't get packed.

nkolev92 · 2024-01-22T18:00:03Z

Directory.Build.props

@@ -9,6 +9,11 @@
    <Import Project="build\config.props" />
    <Import Project="build\common.project.props" />
  </ImportGroup>
+  <PropertyGroup>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>


Don't set unconditional properties. Might not apply to this property, but overwrites can lead to issues.

The changes would also normally go to common.project.props.

nkolev92 · 2024-01-22T18:03:56Z

Directory.Build.props

+  <PropertyGroup>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
+    <NoWarn>$(NoWarn);CS1591;IDE0076;CS1573;CS1574;CS1587;CS1570;CS1572;CS1584;CS1658;CS0419;IDE0055;EnableGenerateDocumentationFile</NoWarn>


In the past when we added analyzers, @kartheekp-ms should be able to help out with tihs, we put all the suppressions in globalsupressions.
It basically allows us to keep the analyzers and it does not change code that's not affected.

Re: Additional changes, my argument tends to revolve around git blame and changing lines that don't need change. As is the PR just has removals, and potentially additions with global suppressions, so I have no concerns with that.

ghost · 2024-01-29T19:00:26Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

jgonz120 added 8 commits January 17, 2024 18:02

Change to allow style warning to show up in build and fix all unused …

7763782

…using statements

Fix using statements for debug

a58fdab

moved GenerateDocumentationFile to specific projects requiring it.

8839dcd

Revert "moved GenerateDocumentationFile to specific projects requirin…

6bd3f75

…g it." This reverts commit 8839dcd.

remove generate doc from commandline.xplat to see how it affects build

b3ff3b5

Ignore IDE0055

d002fff

remove comment

8fa24a1

move EnableGenerateDocumentationFile ignore

fd6258e

zivkan reviewed Jan 22, 2024

View reviewed changes

nkolev92 reviewed Jan 22, 2024

View reviewed changes

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jan 29, 2024

ghost closed this Feb 5, 2024

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change to allow style warning to show up in build and fix all unused … #5601

Change to allow style warning to show up in build and fix all unused … #5601

jgonz120 commented Jan 18, 2024

zivkan Jan 22, 2024

zivkan Jan 22, 2024

nkolev92 Jan 22, 2024

zivkan Jan 22, 2024

zivkan Jan 22, 2024

zivkan Jan 22, 2024

nkolev92 Jan 22, 2024

nkolev92 left a comment

nkolev92 Jan 22, 2024

nkolev92 Jan 22, 2024

nkolev92 Jan 22, 2024

ghost commented Jan 29, 2024

	- script: dotnet format whitespace --verify-no-changes NuGet.sln
	name: dotnetFormatWhitespace
	displayName: Check whitespace formatting
	continueOnError: ${{ parameters.isOfficialBuild }}
	condition: succeededOrFailed()

Change to allow style warning to show up in build and fix all unused … #5601

Change to allow style warning to show up in build and fix all unused … #5601

Conversation

jgonz120 commented Jan 18, 2024

Bug

Description

Ignored warnings

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jan 29, 2024