Enable Roslyn analyzers in build process #11916

iSazonov · 2020-02-21T16:04:48Z

PR Summary

Enable Roslyn analyzers in build process
Force rule IDE0044: Add readonly modifier
Add commit to fix the rule IDE0044

PR Context

Analyzers help to make code high quality.
See .Net Runtime experience dotnet/runtime#30740

We should benefit from C# analyzers too.

PR Checklist

This change is

TravisEz13 · 2020-02-21T19:24:30Z

PowerShell.sln

Please revert this changes

Only the line or all such lines?

Reverted all.

TravisEz13

too big to review, PR review tool doesn't function

TravisEz13 · 2020-02-21T19:27:21Z

Perhaps you can enable one rule at a time to reduce the file changes. I cannot jump to files due to the large number of changes.

iSazonov · 2020-02-22T08:37:25Z

@TravisEz13 You can review commit by commit. Yes, only one rule is forced in the PR. This add "readonly" qualifier. I will split the large commit by 20-30 files.

.editorconfig

TravisEz13 · 2020-02-22T18:40:47Z

Git hub doesn't have a mechanism for tracking reviews, commit by commit. I can see if reviewable still works.

TravisEz13 · 2020-02-22T18:41:01Z

@PoshChan Please remind me in 2 days

PoshChan · 2020-02-24T18:42:01Z

@TravisEz13, this is the reminder you requested 2 days ago

Enable analyzers

TravisEz13

Reviewed 31 of 366 files at r1.
Reviewable status: 31 of 366 files reviewed (waiting on @adityapatwardhan, @anmenaga, @daxian-dbw, @iSazonov, and @SteveL-MSFT)

TravisEz13 · 2020-04-14T21:32:45Z

I'll try to review a portion each day.

TravisEz13 · 2020-04-14T21:33:02Z

@PoshChan Please remind me in 1 day.

PoshChan · 2020-04-15T21:33:07Z

@TravisEz13, this is the reminder you requested 1 day. ago

xtqqczze · 2020-04-17T01:55:54Z

This PR does not seem to enable any analyzers during the build process. If it did then the build would fail since IDE1006.severity is set to error and there are 904 IDE1006 rule violations.

xtqqczze · 2020-04-17T02:05:44Z

Currently IDE code style rules do not apply during builds, see dotnet/roslyn#33558

iSazonov · 2020-04-17T03:31:32Z

@xtqqczze Yes, you are right about that IDE rules. The PR is only initial for the enhancement of our build process. Later we can easily add other rules and force then as needed. Also I hope the .Net issue will fixed in some w A3E2 ay.

Update: the next Microsoft.CodeAnalysis.CSharp 3.6.0 will implement this in days.

vexx32

Overall these changes look good. I have a couple questions/comments on a few of them. 🙂

vexx32 · 2020-05-12T13:15:52Z

src/Microsoft.Management.Infrastructure.CimCmdlets/CimCmdletModuleInitialize.cs

+            private readonly string _name;

            /// <summary>
            /// The string defining real cmdlet name.
            /// </summary>
            internal string Value { get { return this._value; } }

-            private string _value = string.Empty;
+            private readonly string _value = string.Empty;

            /// <summary>
            /// The string defining real cmdlet name.
            /// </summary>
            internal ScopedItemOptions Options { get { return this._options; } }

-            private ScopedItemOptions _options = ScopedItemOptions.AllScope | ScopedItemOptions.ReadOnly;
+            private readonly ScopedItemOptions _options = ScopedItemOptions.AllScope | ScopedItemOptions.ReadOnly;


Am I remembering wrong or was this file removed in a recent PR since it only functions to add command aliases, which were moved to attributes?

It is still not merged.

vexx32 · 2020-05-12T16:20:44Z

src/System.Management.Automation/engine/InitialSessionState.cs

        /// This is the default function to use for tab expansion.
        /// </summary>
-        private static string s_tabExpansionFunctionText = @"
+        private static readonly string s_tabExpansionFunctionText = @"


This seems like something we'd have as const, no?

It is static. If it is wrong we should fix this in another PR.

Yeah that's why I asked. Maybe make a note of these and we can have another look in a later PR, see if it's worth making them const. 🙂

It is used only once. I believe the const would be saved in a static area too and it makes no sense to change - we get no perf benefits.

vexx32 · 2020-05-12T16:50:50Z

src/System.Management.Automation/engine/hostifaces/HostUtilities.cs

        ";

-        private static string s_createCommandExistsInCurrentDirectoryScript = @"
+        private static readonly string s_createCommandExistsInCurrentDirectoryScript = @"


Should these strings be const?

vexx32 · 2020-05-12T17:29:34Z

src/System.Management.Automation/security/nativeMethods.cs

        internal struct CRYPT_PROVIDER_CERT
        {
-            private DWORD _cbStruct;
+            private readonly DWORD _cbStruct;
            internal IntPtr pCert; // PCCERT_CONTEXT
-            private BOOL _fCommercial;
-            private BOOL _fTrustedRoot;
-            private BOOL _fSelfSigned;
-            private BOOL _fTestCert;
-            private DWORD _dwRevokedReason;
-            private DWORD _dwConfidence;
-            private DWORD _dwError;
-            private IntPtr _pTrustListContext; // CTL_CONTEXT*
-            private BOOL _fTrustListSignerCert;
-            private IntPtr _pCtlContext; // PCCTL_CONTEXT
-            private DWORD _dwCtlError;
-            private BOOL _fIsCyclic;
-            private IntPtr _pChainElement; // PCERT_CHAIN_ELEMENT
+            private readonly BOOL _fCommercial;
+            private readonly BOOL _fTrustedRoot;
+            private readonly BOOL _fSelfSigned;
+            private readonly BOOL _fTestCert;
+            private readonly DWORD _dwRevokedReason;
+            private readonly DWORD _dwConfidence;
+            private readonly DWORD _dwError;
+            private readonly IntPtr _pTrustListContext; // CTL_CONTEXT*
+            private readonly BOOL _fTrustListSignerCert;
+            private readonly IntPtr _pCtlContext; // PCCTL_CONTEXT
+            private readonly DWORD _dwCtlError;
+            private readonly BOOL _fIsCyclic;
+            private readonly IntPtr _pChainElement; // PCERT_CHAIN_ELEMENT


We don't provide a constructor to set these and they're readonly. How would these be set?

Same question for the rest of the structs in this file actually.

I'd ask why they private :-)

Looks like false positives.

Yeah both are good questions here 😁

Not sure how this struct is actually used to be honest, but I don't see how it would be useful at the moment.

In fact, all these structures are created in P/Invoke, they are really readonly for C# and "private" here is "named unused".
I believe we can accept the change.

iSazonov · 2020-05-13T03:23:36Z

@TravisEz13 Could you please continue?

xtqqczze · 2020-05-19T12:21:23Z

@iSazonov something to be aware of: dotnet/roslyn#43051

iSazonov · 2020-05-19T12:35:40Z

@xtqqczze Thanks! We use warnaserror in the project.
And we force errors

# IDE0044: Add readonly modifier
dotnet_style_readonly_field = true:error

iSazonov · 2020-05-19T17:19:03Z

@TravisEz13 Could you please continue the code review?

TravisEz13

Reviewed 35 of 366 files at r1.
Reviewable status: 57 of 366 files reviewed (waiting on @adityapatwardhan, @anmenaga, @daxian-dbw, @iSazonov, @SteveL-MSFT, @TravisEz13, and @vexx32)

ghost · 2020-06-09T14:00:19Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

xtqqczze · 2020-10-26T16:01:00Z

@iSazonov It seems this PR is stale, we could continue with IDE0044 in #13880? We could split the changes into multiple PRs for ease of review.

iSazonov · 2020-10-26T16:23:33Z

@iSazonov It seems this PR is stale, we could continue with IDE0044 in #13880? We could split the changes into multiple PRs for ease of review.

Yes.

iSazonov requested review from SteveL-MSFT, TravisEz13, adityapatwardhan, anmenaga and daxian-dbw as code owners February 21, 2020 16:04

ghost assigned daxian-dbw Feb 21, 2020

iSazonov force-pushed the cleanup-analyzers-fixes-2 branch from 833cc04 to a174eb6 Compare February 21, 2020 16:22

TravisEz13 reviewed Feb 21, 2020 8000

View reviewed changes

TravisEz13 requested changes Feb 21, 2020

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 21, 2020

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 22, 2020

iSazonov force-pushed the cleanup-analyzers-fixes-2 branch from a174eb6 to 47c88bc Compare February 22, 2020 17:59

PoshChan reviewed Feb 22, 2020

View reviewed changes

.editorconfig Outdated Show resolved Hide resolved

iSazonov force-pushed the cleanup-analyzers-fixes-2 branch from 47c88bc to a6b8bc0 Compare March 9, 2020 17:28

iSazonov added 11 commits March 29, 2020 11:34

Fix build

60616db

Update Solution. Add Refernce to EditorConfig

f9fc539

Enable analyzers

Disable IDE0003: "Remove qualification" like 'this.'

490305b

Commit 1

6385c10

Commit 2

68afea6

Commit 3

f563af3

Commit 4

44f037c

Commit 5

1282cfd

Commit 6

71b3e12

Commit 7

9afbf06

Commit 8

7058d35

TravisEz13 reviewed Apr 14, 2020

View reviewed changes

Merge branch 'master' into cleanup-analyzers-fixes-2

194501b

TravisEz13 modified the milestones: 7.1.0-preview.2, 7.1.0-preview.3 Apr 24, 2020

iSazonov mentioned this pull request May 12, 2020

Nullable annotations #12631

Closed

vexx32 approved these changes May 12, 2020

View reviewed changes

Merge branch 'master' into cleanup-analyzers-fixes-2

36ac477

TravisEz13 modified the milestones: 7.1.0-preview.3, 7.1.0-preview.4 May 14, 2020

TravisEz13 reviewed May 20, 2020

View reviewed changes

xtqqczze mentioned this pull request May 27, 2020

Add readonly modifier to internal static members #11777

Merged

14 tasks

xtqqczze mentioned this pull request Jun 4, 2020

Enable Microsoft.CodeAnalysis.NetAnalyzers code analyzers #12892

Closed

8 tasks

ghost added the Review - Needed The PR is being reviewed label Jun 9, 2020

adityapatwardhan removed this from the 7.1.0-preview.4 milestone Jun 29, 2020

ghost removed the Review - Needed The PR is being reviewed label Oct 26, 2020

iSazonov closed this Oct 26, 2020

iSazonov deleted the cleanup-analyzers-fixes-2 branch October 26, 2020 16:23

Enable Roslyn analyzers in build process #11916

Enable Roslyn analyzers in build process #11916

Uh oh!

Conversation

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!