Suspicious Cast from Char to Int32 Analyzer #5506
Conversation
- Supports string.Split(char, int, StringSplitOptions) - Supports string.Split(string, int, StringSplitOptions) - Supports StringBuilder(int)
- Fix possible null-deref in new IOperationExtensions.GetArgumentsInParameterOrder() method
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SuspiciousCastFromCharToInt.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SuspiciousCastFromCharToInt.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SuspiciousCastFromCharToInt.cs
Outdated
Show resolved
Hide resolved
...tAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/SuspiciousCastFromCharToIntTests.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SuspiciousCastFromCharToInt.Fixer.cs
Outdated
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SuspiciousCastFromCharToInt.Fixer.cs
Outdated
Show resolved
Hide resolved
...lyzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpSuspiciousCastFromCharToInt.Fixer.cs
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #5506 +/- ##
=======================================
Coverage 95.49% 95.50%
=======================================
Files 1217 1217
Lines 277298 277298
Branches 16741 16741
=======================================
+ Hits 264817 264829 +12
+ Misses 10280 10279 -1
+ Partials 2201 2190 -11 |
| namespace Microsoft.NetCore.CSharp.Analyzers.Runtime | ||
| { | ||
| [ExportCodeFixProvider(LanguageNames.CSharp), Shared] | ||
| public sealed class CSharpSuspiciousCastFromCharToIntFixer : SuspiciousCastFromCharToIntFixer |
There was a problem hiding this comment.
A more declarative approach would be to have the base type be a generic type with TInvocationExpressionSyntax as the type parameter and the below method GetMemberAccessExpressionSyntax take a TInvocationExpressionSyntax parameter.
There was a problem hiding this comment.
Looking at the second method, probably the type should be SuspiciousCastFromCharToIntFixer<TInvocationExpressionSyntax, TParameterSyntax>
| <value>Implicit cast from char to int in method call is suspicious. Consider using a different overload.</value> | ||
| </data> | ||
| <data name="SuspiciousCastFromCharToIntMessage" xml:space="preserve"> | ||
| <value>Suspicious implicit cast from char to int</value> |
There was a problem hiding this comment.
I am not sure if this message is descriptive enough for the users. Maybe give more context in the message that this argument is being used for count parameter and the char is converted into int and used as a count, rather than a string split character. We may also want to recommend users that they probably intended to use a different overload that takes multiple string split characters.
|
|
||
| if (stringSplitMethods.Contains(invocation.TargetMethod)) | ||
| { | ||
| foreach (var argument in invocation.Arguments) |
There was a problem hiding this comment.
Can we instead be more specific and check only the argument specified for the second parameter? You probably want to invoke the below method with parameterIndex = 1
roslyn-analyzers/src/Utilities/Compiler/Extensions/IOperationExtensions.cs
Lines 804 to 806 in 44b5783
| void AddIfNotNull(IMethodSymbol? method) | ||
| { | ||
| if (method is not null) | ||
| builder!.Add(method); |
There was a problem hiding this comment.
Interesting, is builder a nullable reference type? Why is ! required here?
There was a problem hiding this comment.
It was complaining because AddIfNotNull is a local function and builder is a closed-over local. We know that AddIfNotNull is never called before builder is initialized, but for some reason the compiler does not know that and was reporting a warning.
| if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemStringSplitOptions, out var stringSplitOptionsType)) | ||
| return ImmutableHashSet<IMethodSymbol>.Empty; | ||
|
|
||
| var builder = ImmutableHashSet.CreateBuilder<IMethodSymbol>(SymbolEqualityComparer.Default); |
There was a problem hiding this comment.
We also have PooledHashSet in the repository that can likely be used.
| { | ||
| return argument.Value is IConversionOperation conversion && | ||
| conversion.IsImplicit && | ||
| conversion.Operand.Type.SpecialType is SpecialType.System_Char && |
There was a problem hiding this comment.
Can the operand or its type be null for error cases or say if null literal is used?
| return argument.Value is IConversionOperation conversion && | ||
| conversion.IsImplicit && | ||
| conversion.Operand.Type.SpecialType is SpecialType.System_Char && | ||
| argument.Parameter.Type.SpecialType is SpecialType.System_Int32; |
There was a problem hiding this comment.
This check can be avoided if we invoked IsImplicitCharToIntConversion only for the argument corresponding to the parameter that we knew is of int type.
|
|
||
| foreach (var argument in creation.Arguments) | ||
| { | ||
| if (IsImplicitCharToIntConversion(argument)) |
There was a problem hiding this comment.
Same comment here. I think we should not do this check for all arguments, but instead narrow it down to specific constructor parameter/argument where we know the parameter is an integral count.
| @@ -1,4 +1,4 @@ | |||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | |||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | |||
| [InlineData( | ||
| "'c', ({|#0:'a'|}), System.StringSplitOptions.None", | ||
| "new char[] { 'c', 'a' }, System.StringSplitOptions.None")] | ||
| public Task StringSplit_CharInt32StringSplitOptions_Fixed_CS(string testArgumentList, string fixedArgumentList) |
There was a problem hiding this comment.
Can we add tests with named arguments, both is parameter order and in non-parameter order?
There was a problem hiding this comment.
Please also add tests where the some parameters have named arguments and some don't, for example, if the split options parameter had a named argument, we should retain the argument name in fixed code.
There was a problem hiding this comment.
Oh, I see you do have some tests with named arguments just above. I am not sure if we want to change the argument order in the fixed code though. For example if options: System.StringSplitOptions.None is the first argument, we should probably retain it so in the fixed code and have the code fix add a named argument for new array argument. If it's too much work to get it working in the fixer, then its fine to retain what you have.
| { | ||
| private StringBuilderCtor_Int32(IMethodSymbol ctor) : base(ctor) { } | ||
|
|
||
| public static StringBuilderCtor_Int32? Create(Compilation compilation, RequiredSymbols symbols) |
|
Ping @NewellClark |
|
@NewellClark, are you still working on this? |
|
@stephentoub My apologies. I completely forgot I was assigned to that. I will resume work on it immediately. |
|
No need to apologize. Thanks for working on these. |
|
Ping @NewellClark are you still planning to work on this? |
Fix #47927
I noticed there wasn't a category for Correctness in DiagnosticCategoriesAndIdRanges.txt, so I went ahead and added one. However, when I did, I found that there are a couple analyzers that use this category, even though it wasn't present in the file, so I had to add an RS range of IDs as well.
The analyzer reports a diagnostic on implicit char-to-int conversions on arguments to any
StringBuilderconstructor, as well asstring.Split(char, int, StringSplitOptions)andstring.Split(string, int, StringSplitOptions).The fixer converts
string.Splitcalls to eitherstring.Split(char[], StringSplitOptions)orstring.Split(string[], StringSplitOptions), and convertsStringBuilder(int)calls toStringBuilder(string)calls. Violations in otherStringBuilderconstructors are reported but not fixed.I ran the analyzer against dotnet/runtime and found nothing. I tried to run the analyzer against roslyn-analyzers, but a bunch of other analyzers (not my analyzer) kept throwing exceptions. Maybe I'm running against roslyn-analyzers incorrectly?