Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Analyzer and fixer that recommend case insensitive string comparison #6662

Merged
merged 47 commits into from
Jun 20, 2023
Merged

Analyzer and fixer that recommend case insensitive string comparison #6662

merged 47 commits into from
Jun 20, 2023

Conversation

carlossanlop
Copy link
Member

@carlossanlop carlossanlop commented May 31, 2023 •
edited
Loading

Fixes dotnet/runtime#78606

The analyzer and fixer are both functional in the happy paths and some edge cases, so it's good to start reviewing.

TODOs:

  • Handle additional IndexOf overloads that take a StringComparison.
  • Handle cases where the first invocation is parenthesized: Analyzer and fixer that recommend case insensitive string comparison #6662 (comment)
  • Add tests where there is no diagnostic expected.
  • Add tests where the string is a more complicated expression.
  • Handle cases where the ToLower{Invariant} and ToUpper{Invariant} invocation is inverted. For example, instead of a.Compare(b.ToLower()) or a.CompareTo(b.ToUpper()).
  • Handle named parameters where applicable.
  • Change title.
  • CompareTo should just be analyzed but not fixed.

@carlossanlop carlossanlop self-assigned this May 31, 2023
@carlossanlop carlossanlop requested a review from a team as a code owner May 31, 2023 05:46
@carlossanlop carlossanlop changed the title Recommend case insensitive string comparison Analyzer and fixer that recommend case insensitive string comparison May 31, 2023
mavasani
mavasani reviewed Jun 15, 2023
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Overall, looks great. I have reviewed the analyzer and the tests properly, and skimmed through the fixer code. Thank you for the exhaustive unit tests!

@carlossanlop carlossanlop mentioned this pull request Jun 15, 2023
Open
@carlossanlop carlossanlop mentioned this pull request Jun 17, 2023
Closed
@carlossanlop
Copy link
Member Author

I addressed your latest feedback, @Youssef1313 and @mavasani.

There's one CI failure (not sure if related to my change, I'm investigating), but putting that aside, does this look good enough for a sign-off?

Note that I'm going to start working on this one next: dotnet/runtime#78607 . It's closely related to this one. In the API review it was requested that, if possible, we join both this analyzer and that one under the same ID.

mavasani
mavasani approved these changes Jun 19, 2023
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Youssef1313 - would you like to do another review pass? If you are able to do so this week, we can wait for your signoff before merging. However, if you are busy, we should go ahead with the merge and you can review whenever convenient. Please let us know, thanks!

@mavasani
Copy link
Contributor

There's one CI failure

@carlossanlop You'd need to re-run msbuild /t:pack at repo root and push the changes.

Youssef1313
Youssef1313 reviewed Jun 19, 2023
View reviewed changes
...Microsoft.NetCore.Analyzers/Performance/RecommendCaseInsensitiveStringComparison.Analyzer.cs
Comment on lines +231 to +234
else if (diagnosableMethod.Equals(compareToStringMethod))
{
context.ReportDiagnostic(diagnosableInvocation.CreateDiagnostic(RecommendCaseInsensitiveStringComparerRule));
}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this will be a false-negative in some cases. Previously, I mentioned that the codefix produces code that is semantically different. Is it a false negative in the first place?

Or maybe report CompareTo under a separate ID to give user more control to disable it alone?

Copy link
Member Author

Choose a reason for hiding this comment

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

I addressed your concern about CompareTo changing the semantics. I special-cased CompareTo so that it only triggers the analyzer but not the codefix. Check out the unit tests to see how I separate them.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, this comment is about whether we should actually warn or not, and if we should warn, whether it should be a separate ID or not.
Per #6662 (comment), I'm personally okay with doing this in a follow-up (unless @mavasani thinks this should be in this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah got it. Yes, let's continue the discussion separately.

Youssef1313
Youssef1313 approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

LGTM, with a small comment on CompareTo

Youssef1313
Youssef1313 reviewed Jun 19, 2023
View reviewed changes
Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

For CompareTo, I think we should only report when the case changing method is called on both strings (or a constant known to be lower-case at compile-time). In this case, I think the codefix will produce the correct result and we can add it back.

@carlossanlop
Copy link
Member Author

For CompareTo, I think we should only report when the case changing method is called on both strings (or a constant known to be lower-case at compile-time). In this case, I think the codefix will produce the correct result and we can add it back.

Good point. If you don't have any objections, allow me to do this in V2. Right now, CompareTo is being diagnosed only, no code fix is being offered.

@Youssef1313
Copy link
Member

For CompareTo, I think we should only report when the case changing method is called on both strings (or a constant known to be lower-case at compile-time). In this case, I think the codefix will produce the correct result and we can add it back.

Good point. If you don't have any objections, allow me to do this in V2. Right now, CompareTo is being diagnosed only, no code fix is being offered.

No problem for me to do it in a follow-up. @mavasani thoughts?

@carlossanlop
Copy link
Member Author

No problem for me to do it in a follow-up. @mavasani thoughts?

I consulted with @mavasani via chat and he agrees. @Youssef1313, I added your latest CompareTo suggestion to my follow up issue: #6689

I addressed the msbuild /t:pack issue and the build should pass now. For future reference for myself, here's what I had to do:

  • git clean -fdx
  • Delete all the generated xlfs from src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers.xlf
  • Remove the entry that was previously automatically added in src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif
  • Run dotnet restore from root
  • Run msbuild /t:pack from root
  • Build everything as usual
  • Push the added changes

@carlossanlop carlossanlop merged commit 126e31e into dotnet:main Jun 20, 2023
11 checks passed
@carlossanlop carlossanlop deleted the RecommendCaseInsensitiveStringComparison branch June 20, 2023 04:26
@github-actions github-actions bot added this to the vNext milestone Jun 20, 2023
@carlossanlop carlossanlop mentioned this pull request Jun 23, 2023
Closed
@Bellarmine-Head Bellarmine-Head mentioned this pull request Aug 17, 2023
Closed
@gewarren gewarren mentioned this pull request Nov 14, 2023
Closed
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Youssef1313 Youssef1313 Youssef1313 approved these changes

@mavasani mavasani mavasani approved these changes

@jeffhandley jeffhandley Awaiting requested review from jeffhandley

@sharwell sharwell Awaiting requested review from sharwell

@stephentoub stephentoub Awaiting requested review from stephentoub

@Evangelink Evangelink Awaiting requested review from Evangelink

@buyaa-n buyaa-n Awaiting requested review from buyaa-n

Assignees

@carlossanlop carlossanlop

Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers
Projects
None yet
Milestone
vNext
4 participants
@carlossanlop @Youssef1313 @mavasani @stephentoub