Skip to content

Apply code fixes from recently added analyzers - #36097

Merged
pgovind merged 6 commits into
dotnet:masterfrom
pgovind:StringIndexOfAnalyzer
Jun 3, 2020
Merged

pgovind merged 6 commits into
dotnet:masterfrom
pgovind:StringIndexOfAnalyzer

Conversation

@pgovind

@pgovind pgovind commented May 8, 2020

Copy link
Copy Markdown

Code-fixes triggered by dotnet/roslyn-analyzers#3610 and #33786

I've been careful to make changes only on the projects that target netstandard2.1+ and add the analyzer to the NoWarn tags for the ones that also target 2.0.

Comment thread src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/RegistryKey.cs Outdated
@pgovind
pgovind requested a review from bartonjs May 8, 2020 03:23
Comment thread src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs Outdated
Comment thread src/libraries/System.Data.Common/src/System/Data/Common/DbConnectionOptions.cs Outdated
@stephentoub stephentoub changed the title Code-fixes Apply code fixes from recently added analyzers May 8, 2020
Comment thread src/libraries/System.Data.Odbc/src/Common/System/Data/Common/AdapterUtil.Odbc.cs Outdated
Comment thread src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.cs Outdated
@pgovind
pgovind force-pushed the StringIndexOfAnalyzer branch from da39c94 to c8e7440 Compare May 11, 2020 19:23
@pgovind

pgovind commented May 17, 2020

Copy link
Copy Markdown
Author

Friendly bump. Can I get a stamp on this please?

<AssemblyName>System.Net.WebSockets</AssemblyName>
<AllowUnsafeBlocks>True</AllowUnsafeBlocks>
<NoWarn>$(NoWarn);CS1573</NoWarn>
<NoWarn>$(NoWarn);CS1573;CA2249</NoWarn>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why was this rule disabled here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Testing with the rule enabled

<AssemblyName>System.Data.Common</AssemblyName>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<NoWarn>$(NoWarn);CS1573</NoWarn>
<NoWarn>$(NoWarn);CS1573;CA2249</NoWarn>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why was this rule disabled here? (Questioning it because this project only targets netcoreapp)

@pgovind pgovind May 20, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I honestly had the same questions. The CI build failed here without suppressing CA2249 even though the target says netcoreapp. I just assumed our build infra was also building this for another config somehow. Actually, let's test it again now

<WarningLevel>4</WarningLevel>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<NoWarn>$(NoWarn),0419,0649</NoWarn>
<NoWarn>$(NoWarn),0419,0649,CA2249,CA1830</NoWarn>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are these disabled for corelib?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

CA2249 is "Prefer string.Contains over string.IndexOf" and CA1830 is "Prefer typed StringBuilder overloads". Both these APIs are defined in corelib right? So they'd trigger the analyzer but there's nothing to do other than suppress them. I could make the suppression local if you'd prefer that

@pgovind
pgovind merged commit 6b6f520 into dotnet:master Jun 3, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants