Use ToLowerInvariant Or ToUpperInvariant Analyzer #2186

TKharaishvili · 2019-03-10T23:35:13Z

Fixes dotnet/runtime#43976

The first commit only includes the analyzer. Once the analyzer code gets approved, I'll also add the fixer.

src/Microsoft.NetCore.Analyzers/Core/Runtime/UseToLowerInvariantOrToUpperInvariant.cs

Evangelink · 2020-09-22T20:28:22Z

@TKharaishvili Do you plan to update/finish this PR?

TKharaishvili · 2020-09-22T21:11:34Z

@Evangelink I think there were some open questions that needed to be discussed. If you look at the 2nd of my 3 subdiscussions with mavasani above, in the last comment he tagged some people mentioning it.

As far as I know those questions were never addressed.

At this point I don't really remember much about this issue or even the PR itself as it all happened over a year ago.

However I'm open to getting myself back up to speed on all of it given those open questions get addressed.

sharwell

I have two problems with the current design:

As an analyzer in the globalization category, rather than focus on usage of ToLowerInvariant and ToUpperInvariant, we should focus on explicitly indicating the desired culture for the conversion.
Two code fixes should be offered. The first (primary) code fix should retain the call to ToLower but add an argument specifying current culture (semantic preservation). A second code fix can be offered to change the call to ToLowerInvariant.

If we keep the current approach of preferring to push users to change semantics by adopting invariant culture, the analyzer should probably be moved to the Performance category instead of the Globalization category, at which point the value seems to be diminished significantly.

TKharaishvili · 2021-10-21T17:56:18Z

@carlossanlop @tarekgh I managed to update the PR and address some of the original change requests. Can you gentlemen review it?
It only contains an Analyzer for now I'll start working on a Fixer soon.
Also the builds are failing with an error message that is a little hard to understand. I'd appreciate some help with that as well.

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

codecov · 2021-10-22T21:20:05Z

Codecov Report

Merging #2186 (31a5855) into main (5b0b341) will decrease coverage by 0.00%.
The diff coverage is 91.93%.

@@            Coverage Diff             @@
##             main    #2186      +/-   ##
==========================================
- Coverage   96.05%   96.04%   -0.01%     
==========================================
  Files        1315     1323       +8     
  Lines      304998   305370     +372     
  Branches     9667     9687      +20     
==========================================
+ Hits       292952   293294     +342     
- Misses       9726     9740      +14     
- Partials     2320     2336      +16

# Conflicts: # src/NetAnalyzers/RulesMissingDocumentation.md

TKharaishvili · 2022-02-09T16:09:16Z

@carlossanlop @tarekgh gentlemen, can I request another round of code review please? Both the analyzer and fixer are implemented and ready to be looked at.
I see that the CI build is failing, I looked at the logs but I wasn't able to figure out what the cause might be.

tarekgh

LGTM, thanks @TKharaishvili for getting this done.

tarekgh · 2022-02-09T17:18:49Z

@carlossanlop @buyaa-n the changes look good to me. Could you have a look and merge it if you have permission to merge?

.../UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseToLowerInvariantOrToUpperInvariantTests.cs

TKharaishvili · 2022-04-13T09:54:50Z

@tarekgh @buyaa-n I addressed the latest change requests. Had to rename Analyzer, Fixer and Test classes since the old name didn't work for new requirements.

I hope no more major changes will be necessary.

Eager to hear your reply.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SpecifyCultureForToLowerAndToUpper.cs

tarekgh

Thanks @TKharaishvili for getting this ready. LGTM. I think @buyaa-n is going to take a look too.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

...itTests/Microsoft.NetCore.Analyzers/Runtime/SpecifyCultureForToLowerAndToUpperTests.Fixer.cs

...CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpSpecifyCultureForToLowerAndToUpper.Fixer.cs

...yzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpSpecifyCultureForToLowerAndToUpper.cs

buyaa-n

Thanks @TKharaishvili I think the messaging needs some update as now it looks like an analyzer for suggesting to Specify CurrentCulture, and left a few optional NIT, other than that it looks good to me

buyaa-n · 2022-04-14T00:57:12Z

Tagging @sharwell for review as his requested change is addressed

TKharaishvili · 2022-04-15T09:00:27Z

@tarekgh @buyaa-n all the requested changes are made.

buyaa-n · 2022-04-15T17:44:57Z

Thank you @TKharaishvili, looks great to me, normally the reviewer who requested a change should approve or remove the CR before merge, tough I see @sharwell is out. As the requested changes addressed as he wanted hope he would not mind if I dismiss his change request to unblock the PR. We he have any other comment we can address that separately

The request is addressed

Youssef1313 · 2022-04-16T12:12:54Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

+    <value>Specify current culture</value>
+  </data>
+  <data name="UseInvariantVersion" xml:space="preserve">
+    <value>User an invariant version</value>


Looks like a typo? @buyaa-n

Youssef1313 · 2022-04-16T12:19:47Z

...s/VisualBasic/Microsoft.NetCore.Analyzers/Runtime/BasicSpecifyCultureForToLowerAndToUpper.vb

+            Debug.Assert(invocationNode.IsKind(SyntaxKind.InvocationExpression))
+
+            Dim invocation = CType(invocationNode, InvocationExpressionSyntax)


This can fail I think. VB doesn't require parentheses for parameterless method calls

Public Class C Public Sub M(x As String) Dim y = x.ToLower ' This is a member access expression, not an invocation. End Sub End Class

Youssef1313 · 2022-04-16T12:31:00Z

...itTests/Microsoft.NetCore.Analyzers/Runtime/SpecifyCultureForToLowerAndToUpperTests.Fixer.cs

+
+namespace Microsoft.NetCore.Analyzers.Runtime.UnitTests
+{
+    public class SpecifyCultureForToLowerAndToUpperFixerTests


Tests should be consolidated into one file.

mavasani reviewed Mar 13, 2019

View reviewed changes

src/Microsoft.NetCore.Analyzers/Core/Runtime/UseToLowerInvariantOrToUpperInvariant.cs Outdated Show resolved Hide resolved

mavasani reviewed Mar 13, 2019

View reviewed changes

src/Microsoft.NetCore.Analyzers/Core/Runtime/UseToLowerInvariantOrToUpperInvariant.cs Outdated Show resolved Hide resolved

TKharaishvili commented Mar 14, 2019

View reviewed changes

src/Microsoft.NetCore.Analyzers/Core/Runtime/UseToLowerInvariantOrToUpperInvariant.cs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

carlossanlop added the Blocked label Oct 28, 2020

carlossanlop mentioned this pull request Feb 16, 2021

[Proposal] Analyzer that suggests ToUpperInvariant/ToLowerInvariant in place of ToUpper/ToLower dotnet/runtime#43976

Closed

Base automatically changed from master to main March 5, 2021 02:19

jinujoseph requested a review from a team as a code owner March 5, 2021 02:19

mavasani assigned carlossanlop, sharwell and genlu and unassigned sharwell May 20, 2021

sharwell previously requested changes May 20, 2021

View reviewed changes

carlossanlop mentioned this pull request Sep 14, 2021

.NET 7.0 developers can verify more APIs for correct usage to speed up their development dotnet/runtime#57797

Closed

18 tasks

Updated implementation

5a69115

TKharaishvili force-pushed the issue-1487-use-invariant branch from f1ec888 to 5a69115 Compare October 20, 2021 21:03

tarekgh reviewed Oct 21, 2021

View reviewed changes

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md Outdated Show resolved Hide resolved

Ran msbuild to generate relevant files

85d61b6

TKharaishvili added 3 commits January 23, 2022 19:17

Merge branch 'master' into issue-1487-use-invariant

2207bed

Fixer with tests

72cc253

Merge branch 'master' into issue-1487-use-invariant

335d2b5

# Conflicts: # src/NetAnalyzers/RulesMissingDocumentation.md

tarekgh approved these changes Feb 9, 2022

View reviewed changes

buyaa-n reviewed Mar 17, 2022

View reviewed changes

.../UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseToLowerInvariantOrToUpperInvariantTests.cs Outdated Show resolved Hide resolved

buyaa-n reviewed Mar 17, 2022

View reviewed changes

.../UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseToLowerInvariantOrToUpperInvariantTests.cs Outdated Show resolved Hide resolved

TKharaishvili requested review from sharwell, tarekgh and buyaa-n April 13, 2022 09:59

tarekgh reviewed Apr 13, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SpecifyCultureForToLowerAndToUpper.cs Outdated Show resolved Hide resolved

tarekgh approved these changes Apr 13, 2022

View reviewed changes

buyaa-n reviewed Apr 13, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

buyaa-n reviewed Apr 13, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

buyaa-n reviewed Apr 13, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

buyaa-n reviewed Apr 13, 2022

View reviewed changes

...itTests/Microsoft.NetCore.Analyzers/Runtime/SpecifyCultureForToLowerAndToUpperTests.Fixer.cs Show resolved Hide resolved

buyaa-n reviewed Apr 14, 2022

View reviewed changes

...CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpSpecifyCultureForToLowerAndToUpper.Fixer.cs Outdated Show resolved Hide resolved

buyaa-n reviewed Apr 14, 2022

View reviewed changes

...CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpSpecifyCultureForToLowerAndToUpper.Fixer.cs Outdated Show resolved Hide resolved

buyaa-n reviewed Apr 14, 2022

View reviewed changes

...yzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpSpecifyCultureForToLowerAndToUpper.cs Outdated Show resolved Hide resolved

buyaa-n approved these changes Apr 14, 2022

View reviewed changes

Code review requested changes

31a5855

buyaa-n merged commit 334bb06 into dotnet:main Apr 15, 2022

github-actions bot added this to the vNext milestone Apr 15, 2022

Youssef1313 reviewed Apr 16, 2022

View reviewed changes

Youssef1313 mentioned this pull request Apr 16, 2022

Address comments of CA1311 (Use ToLowerInvariant/ToUpperInvariant) PR #5970

Closed

Youssef1313 mentioned this pull request May 3, 2022

Missing CAxxxx documentation dotnet/docs#29264

Closed

10 tasks

TKharaishvili mentioned this pull request May 24, 2022

CA1311 various fixes #6010

Merged

TKharaishvili deleted the issue-1487-use-invariant branch June 3, 2022 12:01

jmarolf modified the milestones: vNext, .NET 7.x Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ToLowerInvariant Or ToUpperInvariant Analyzer #2186

Use ToLowerInvariant Or ToUpperInvariant Analyzer #2186

TKharaishvili commented Mar 10, 2019 •

edited by carlossanlop

Loading

Evangelink commented Sep 22, 2020

TKharaishvili commented Sep 22, 2020

This comment has been minimized.

sharwell left a comment •

edited

Loading

TKharaishvili commented Oct 21, 2021

codecov bot commented Oct 22, 2021 •

edited

Loading

TKharaishvili commented Feb 9, 2022

tarekgh left a comment

tarekgh commented Feb 9, 2022

TKharaishvili commented Apr 13, 2022

tarekgh left a comment •

edited

Loading

buyaa-n left a comment

buyaa-n commented Apr 14, 2022

TKharaishvili commented Apr 15, 2022

buyaa-n commented Apr 15, 2022

Youssef1313 Apr 16, 2022

Youssef1313 Apr 16, 2022

Youssef1313 Apr 16, 2022

		Debug.Assert(invocationNode.IsKind(SyntaxKind.InvocationExpression))

		Dim invocation = CType(invocationNode, InvocationExpressionSyntax)

Use ToLowerInvariant Or ToUpperInvariant Analyzer #2186

Use ToLowerInvariant Or ToUpperInvariant Analyzer #2186

Conversation

TKharaishvili commented Mar 10, 2019 • edited by carlossanlop Loading

Evangelink commented Sep 22, 2020

TKharaishvili commented Sep 22, 2020

This comment has been minimized.

sharwell left a comment • edited Loading

Choose a reason for hiding this comment

TKharaishvili commented Oct 21, 2021

codecov bot commented Oct 22, 2021 • edited Loading

Codecov Report

TKharaishvili commented Feb 9, 2022

tarekgh left a comment

Choose a reason for hiding this comment

tarekgh commented Feb 9, 2022

TKharaishvili commented Apr 13, 2022

tarekgh left a comment • edited Loading

Choose a reason for hiding this comment

buyaa-n left a comment

Choose a reason for hiding this comment

buyaa-n commented Apr 14, 2022

TKharaishvili commented Apr 15, 2022

buyaa-n commented Apr 15, 2022

Youssef1313 Apr 16, 2022

Choose a reason for hiding this comment

Youssef1313 Apr 16, 2022

Choose a reason for hiding this comment

Youssef1313 Apr 16, 2022

Choose a reason for hiding this comment

TKharaishvili commented Mar 10, 2019 •

edited by carlossanlop

Loading

sharwell left a comment •

edited

Loading

codecov bot commented Oct 22, 2021 •

edited

Loading

tarekgh left a comment •

edited

Loading