-
Notifications
You must be signed in to change notification settings - Fork 464
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.
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
Do not suggest passing IFormatProvider to certain Convert methods #7244
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7244 +/- ##
========================================
Coverage 96.49% 96.49%
========================================
Files 1443 1443
Lines 345972 346376 +404
Branches 11383 11387 +4
========================================
+ Hits 333856 334252 +396
- Misses 9235 9242 +7
- Partials 2881 2882 +1 |
var superfluousConvertToStringFormatProviderOverloads = convertType?.GetMembers(nameof(Convert.ToString)) | ||
.OfType<IMethodSymbol>() | ||
.Where(m => m.IsStatic | ||
&& m.Parameters is [{ Type.SpecialType: SpecialType.System_String or SpecialType.System_Boolean or SpecialType.System_Char }, var possibleFormatProvider] | ||
&& possibleFormatProvider.Type.Equals(iformatProviderType, SymbolEqualityComparer.Default)) ?? []; | ||
var superfluousConvertToToCharFormatProviderOverloads = convertType?.GetMembers(nameof(Convert.ToChar)) | ||
.OfType<IMethodSymbol>() | ||
.Where(m => m.IsStatic | ||
&& m.Parameters is [{ Type.SpecialType: SpecialType.System_String }, var possibleFormatProvider] | ||
&& possibleFormatProvider.Type.Equals(iformatProviderType, SymbolEqualityComparer.Default)) ?? []; | ||
var superfluousConvertToBooleanFormatProviderOverloads = convertType?.GetMembers(nameof(Convert.ToBoolean)) | ||
.OfType<IMethodSymbol>() | ||
.Where(m => m.IsStatic | ||
&& m.Parameters is [{ Type.SpecialType: SpecialType.System_String }, var possibleFormatProvider] | ||
&& possibleFormatProvider.Type.Equals(iformatProviderType, SymbolEqualityComparer.Default)) ?? []; | ||
ImmutableHashSet<IMethodSymbol> superfluousFormatProviderOverloads = superfluousConvertToStringFormatProviderOverloads | ||
.Concat(superfluousConvertToToCharFormatProviderOverloads) | ||
.Concat(superfluousConvertToBooleanFormatProviderOverloads) | ||
.ToImmutableHashSet<IMethodSymbol>(SymbolEqualityComparer.Default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got some NITs:
- Better to check null before head to avoid null check each reference and creating empty array
- For ToChar, ToBoolean we expect only one method to get, no need to create array for this
m.IsStatic
is redundant as Convert is a static type, all methods are static anyways
Something like:
var superfluousConvertToStringFormatProviderOverloads = convertType?.GetMembers(nameof(Convert.ToString)) | |
.OfType<IMethodSymbol>() | |
.Where(m => m.IsStatic | |
&& m.Parameters is [{ Type.SpecialType: SpecialType.System_String or SpecialType.System_Boolean or SpecialType.System_Char }, var possibleFormatProvider] | |
&& possibleFormatProvider.Type.Equals(iformatProviderType, SymbolEqualityComparer.Default)) ?? []; | |
var superfluousConvertToToCharFormatProviderOverloads = convertType?.GetMembers(nameof(Convert.ToChar)) | |
.OfType<IMethodSymbol>() | |
.Where(m => m.IsStatic | |
&& m.Parameters is [{ Type.SpecialType: SpecialType.System_String }, var possibleFormatProvider] | |
&& possibleFormatProvider.Type.Equals(iformatProviderType, SymbolEqualityComparer.Default)) ?? []; | |
var superfluousConvertToBooleanFormatProviderOverloads = convertType?.GetMembers(nameof(Convert.ToBoolean)) | |
.OfType<IMethodSymbol>() | |
.Where(m => m.IsStatic | |
&& m.Parameters is [{ Type.SpecialType: SpecialType.System_String }, var possibleFormatProvider] | |
&& possibleFormatProvider.Type.Equals(iformatProviderType, SymbolEqualityComparer.Default)) ?? []; | |
ImmutableHashSet<IMethodSymbol> superfluousFormatProviderOverloads = superfluousConvertToStringFormatProviderOverloads | |
.Concat(superfluousConvertToToCharFormatProviderOverloads) | |
.Concat(superfluousConvertToBooleanFormatProviderOverloads) | |
.ToImmutableHashSet<IMethodSymbol>(SymbolEqualityComparer.Default); | |
ImmutableHashSet<IMethodSymbol> superfluousFormatProviderOverloads = ImmutableHashSet<IMethodSymbol>.Empty; | |
if (convertType != null) | |
{ | |
superfluousFormatProviderOverloads = convertType.GetMembers(nameof(Convert.ToString)) | |
.OfType<IMethodSymbol>() | |
.Where(m => m.Parameters is [{ Type.SpecialType: SpecialType.System_String or SpecialType.System_Boolean or SpecialType.System_Char }, var possibleFormatProvider] | |
&& possibleFormatProvider.Type.Equals(iformatProviderType, SymbolEqualityComparer.Default)).ToImmutableHashSet(); | |
superfluousFormatProviderOverloads = superfluousFormatProviderOverloads | |
.Add(convertType.GetMembers(nameof(Convert.ToChar)) | |
.OfType<IMethodSymbol>() | |
.First(m => m.Parameters is [{ Type.SpecialType: SpecialType.System_String }, var possibleFormatProvider] | |
&& possibleFormatProvider.Type.Equals(iformatProviderType, SymbolEqualityComparer.Default))) | |
.Add(convertType.GetMembers(nameof(Convert.ToBoolean)) | |
.OfType<IMethodSymbol>() | |
.First(m => m.Parameters is [{ Type.SpecialType: SpecialType.System_String }, var possibleFormatProvider] | |
&& possibleFormatProvider.Type.Equals(iformatProviderType, SymbolEqualityComparer.Default))); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, thanks @CollinAlpert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Affected analyzer: SpecifyIFormatProviderAnalyzer
Affected diagnostic ID: CA1305
This PR prevents suggesting to pass an
IFormatProvider
toConvert.ToX
methods where passing anIFormatProvider
would change nothing.Fixes #7154