[Proposal] Analyzer that suggests ToUpperInvariant/ToLowerInvariant in place of ToUpper/ToLower

[jamesqo]

When using String.ToUpper() or String.ToLower(), suggest using String.ToUpperInvariant() and String.ToLowerInvariant().

Something similar is already in place here, but for string comparisons: CA1309: Use ordinal StringComparison

A PR already exists to fix this. We want to get this proposal approved before reviewing it: dotnet/roslyn-analyzers#2186

• Analyzer: small
• Fixer: small
• Suggested category: Globalization
• Suggested severity: Info (to match CA1309)
• Milestone: 6.0

Usage examples:

string s = "my string";
string lower, upper;


// Suggest to use ToLowerInvariant() or to explicitly indicate the culture.
// Offer fix to use ToLowerInvariant()
lower = s.ToLower();
lower = "str".ToLower();

// Suggest to use ToUpperInvariant() or to explicitly indicate the culture.
// Offer fix to use ToUpperInvariant()
upper = s.ToUpper();
upper = "str".ToUpper();

CultureInfo culture = new CultureInfo("es-ES", false);

// Should not suggest
lower = s.ToLower(culture);
upper = s.ToUpper(culture);



// Should we suggest to use To*Invariant() in these cases? tarekgh thinks it's not necessary, but we can discuss it:
// Is this the same as ToLowerInvariant? https://source.dot.net/#System.Private.CoreLib/String.Manipulation.cs,1710
lower = s.ToLower(CultureInfo.InvariantCulture);
// Is this the same as ToUpperInvariant? https://source.dot.net/#System.Private.CoreLib/String.Manipulation.cs,1725
upper = s.ToUpper(CultureInfo.InvariantCulture);

[TKharaishvili]

@mavasani created a PR for this - dotnet/roslyn-analyzers#2186

[carlossanlop]

@mavasani should this go through API review in the runtime repo, or is this just an improvement to an existing analyzer?

[carlossanlop]

@mavasani This issue was opened back in 2017 before we decided that Roslyn analyzer proposals should be approved in the API review meeting in the runtime repo. It already has a PR opened back in 2019, but it wasn't discussed here or approved. Should we move it to the runtime repo to approve it first?

[mavasani]

Yes, I agree we need a proposal/triage in runtime repo before taking this forward.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @tarekgh, @safern, @krwq
See info in area-owners.md if you want to be subscribed.

[tarekgh]

@jamesqo as suggested in this thread you need to add the proposal up in the issue description.

are you suggesting to have the analyzer suggest ToUpperInvariant/ToLowerInvariant all the time when ToUpper/ToLower show up? or would be depending on the context of the callsite?

[krwq]

I think if we do not recommend to use ToUpper/ToLower at all we should deprecate it or change implementation to call into To*Invariant (perhaps with a switch to bring back the old behavior)

[tarekgh]

or change implementation to call into To*Invariant (perhaps with a switch to bring back the old behavior)

I don't think it is a good idea to change the behavior even with a switch. From other discussions with the community in other issues, it looks using the switch is not a solution for all situations. I agree to deprecate String.ToUpper/ToLower() though and perhaps we think exposing something like ToUpperOrdinal/ToLowerOrdinal. but anyway, I think having the analyzer warn about String.ToUpper/ToLower() is a good thing anyway.

[carlossanlop]

I updated the description to get this proposal ready for review. @jamesqo @TKharaishvili @tarekgh let me know if you have any comments about my edit. Otherwise, let's wait for the API review meeting to discuss this.

[tarekgh]

I am fine with the proposal. Only the last part lower = s.ToLower/ToUpper(CultureInfo.InvariantCulture); is not really necessary as I am seeing both usages still correct.

[terrajobst]

Video

• This is one of the more noisy analyzers from FxCop
  • It makes sense for class libraries / server projects, but for WinForms, WPF it doesn't really make a difference
  • Can we scope the analyzer for the project type / output type? Not all libraries are the same (e.g. some are user control, some are test projects)
• However, it seems useful to have a severity of hidden so that people can invoke the fixer to replace all calls to the appropriate overload

[tarekgh]

@TKharaishvili just checking, are you still working on that?

@carlossanlop looks you were doing more follow up here. do you have more info about the plan for this one?

[tarekgh]

I moved this to 7.0 as I am not seeing urgency to have it for 6.0. That is fine if we get a PR for 6.0 too.

[TKharaishvili]

@tarekgh I won't have any time to work on this in the next 2 months. I'll come back to it afterwards if that works for you guys.

[carlossanlop]

@TKharaishvili are you still going to work on this?

[TKharaishvili]

@carlossanlop yes, I'll have an updated PR over the next several days.

[TKharaishvili]

Hi @carlossanlop I tested positive for covid and won't be able to get any work done for some time. If you guys need this taken care of urgently, please unassign me from the issue, otherwise I'll get back to it once I'm better.
I apologize for the inconvenience.

[TKharaishvili]

Hi @carlossanlop I'm back at it. I see a lot has changed in the repo since I first sent that PR(which is to be expected for a 3 year gap). Originally I added the analyzer source file to the Microsoft.NetCore.Analyzers project. That project is no longer there but I found something similar, I'm thinking the Microsoft.CodeAnalysis.NetAnalyzers project -> Microsoft.NetCore.Analyzers.Runtime folder is the right place for it. Would you agree?

Also I'm thinking of dropping this current PR and creating a new one instead. Doing a merge after this long is proving to be too much of a headache.

[tarekgh]

@carlossanlop @buyaa-n could you please help with @TKharaishvili question in the previous comment?

@TKharaishvili are you still willing to submit the PR? Thanks!

[buyaa-n]

Sorry for late reply @TKharaishvili @tarekgh

I'm thinking the Microsoft.CodeAnalysis.NetAnalyzers project -> Microsoft.NetCore.Analyzers.Runtime folder is the right place for it. Would you agree?

Yes that is the right place

Also I'm thinking of dropping this current PR and creating a new one instead. Doing a merge after this long is proving to be too much of a headache.

Right, sounds good, feel free to close your old PR then

[TKharaishvili]

@tarekgh @buyaa-n I already proceeded since there was no response for a while. Decided to update the existing PR, it currently has the analyzer in it and I believe it was looked at by some of the team members. All that's missing right now is a fixer. I'll resume working on that next week.

[danmoseley]

@TKharaishvili hello, are you still working on this ?

[TKharaishvili]

@danmoseley I pushed the latest code review requested changes yesterday. Waiting for another round of review now.