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.

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

Suspicious Cast from Char to Int32 Analyzer #5506

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System.Composition;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.NetCore.Analyzers.Runtime;

namespace Microsoft.NetCore.CSharp.Analyzers.Runtime
{
[ExportCodeFixProvider(LanguageNames.CSharp), Shared]
public sealed class CSharpSuspiciousCastFromCharToIntFixer : SuspiciousCastFromCharToIntFixer
Copy link
Contributor

@mavasani mavasani Oct 25, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the second method, probably the type should be SuspiciousCastFromCharToIntFixer<TInvocationExpressionSyntax, TParameterSyntax>

{
internal override SyntaxNode GetMemberAccessExpressionSyntax(SyntaxNode invocationExpressionSyntax)
{
return ((InvocationExpressionSyntax)invocationExpressionSyntax).Expression;
}

internal override SyntaxNode GetDefaultValueExpression(SyntaxNode parameterSyntax) => ((ParameterSyntax)parameterSyntax).Default.Value;
}
}
1 change: 1 addition & 0 deletions src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@
Rule ID | Category | Severity | Notes
--------|----------|----------|-------
CA1849 | Performance | Disabled | UseAsyncMethodInAsyncContext, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1849)
CA2019 | Reliability | Warning | SuspiciousCastFromCharToIntAnalyzer, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2019)
CA5404 | Security | Disabled | DoNotDisableTokenValidationChecks, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca5404)
CA5405 | Security | Disabled | DoNotAlwaysSkipTokenValidationInDelegates, [Documentation](https://docs.microsoft.com/visualstudio/code-quality/ca5405)
Original file line number Diff line number Diff line change
Expand Up @@ -1795,4 +1795,16 @@
<data name="DoNotAlwaysSkipTokenValidationInDelegatesMessage" xml:space="preserve">
<value>The {0} is set to a function that is always returning true. By setting the validation delegate, you are overriding default validation and by always returning true, this validation is completely disabled.</value>
</data>
<data name="SuspiciousCastFromCharToIntDescription" xml:space="preserve">
<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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

</data>
<data name="SuspiciousCastFromCharToIntTitle" xml:space="preserve">
<value>Suspicious cast from char to int</value>
</data>
<data name="SuspiciousCastFromCharToIntCodeFixTitle" xml:space="preserve">
<value>Use correct overload</value>
</data>
</root>
Original file line number Diff line number Diff line change
@@ -0,0 +1,264 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;
using Analyzer.Utilities;
using Analyzer.Utilities.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Operations;
using RequiredSymbols = Microsoft.NetCore.Analyzers.Runtime.SuspiciousCastFromCharToIntAnalyzer.RequiredSymbols;

namespace Microsoft.NetCore.Analyzers.Runtime
{
public abstract class SuspiciousCastFromCharToIntFixer : CodeFixProvider
{
public sealed override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(SuspiciousCastFromCharToIntAnalyzer.RuleId);

public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var doc = context.Document;
var token = context.CancellationToken;
var root = await doc.GetSyntaxRootAsync(token).ConfigureAwait(false);
var model = await doc.GetSemanticModelAsync(token).ConfigureAwait(false);

// If there were unnecessary parentheses on the argument expression, then we won't get the reported IArgumentOperation
// from the call to SemanticModel.GetOperation().
// This is because the Syntax property of an IArgumentOperation does NOT include the unnecessary parentheses.
// In other words, if we have an IArgumentOperation 'argument', then the following two expressions are equivalent:
// argument.SemanticModel.GetOperation(argument.Syntax)
// is equivalent to
// argument.Value.WalkDownConversion()
var reportedOperation = model.GetOperation(root.FindNode(context.Span), token);
var argumentOperation = reportedOperation as IArgumentOperation ?? reportedOperation?.WalkUpConversion()?.Parent as IArgumentOperation;
if (argumentOperation is null)
return;

var targetMethod = argumentOperation.Parent switch
{
IInvocationOperation invocation => invocation.TargetMethod,
IObjectCreationOperation objectCreation => objectCreation.Constructor,
_ => default
};

if (targetMethod is null)
return;

var cache = CreateFixerCache(model.Compilation);

if (!cache.TryGetValue(targetMethod, out var fixer))
return;

var codeAction = CodeAction.Create(
MicrosoftNetCoreAnalyzersResources.SuspiciousCastFromCharToIntCodeFixTitle,
token => fixer.GetChangedDocumentAsync(new FixCharCastContext(context, argumentOperation.Parent, this, token)),
nameof(MicrosoftNetCoreAnalyzersResources.SuspiciousCastFromCharToIntCodeFixTitle));
context.RegisterCodeFix(codeAction, context.Diagnostics);
}

internal abstract SyntaxNode GetMemberAccessExpressionSyntax(SyntaxNode invocationExpressionSyntax);

internal abstract SyntaxNode GetDefaultValueExpression(SyntaxNode parameterSyntax);

// Property bag
#pragma warning disable CA1815 // Override equals and operator equals on value types
private readonly struct FixCharCastContext
#pragma warning restore CA1815 // Override equals and operator equals on value types
{
public FixCharCastContext(CodeFixContext codeFixContext, IOperation operation, SuspiciousCastFromCharToIntFixer fixer, CancellationToken cancellationToken)
{
CodeFixContext = codeFixContext;
Operation = operation;
Fixer = fixer;
CancellationToken = cancellationToken;
}

public CodeFixContext CodeFixContext { get; }
public IOperation Operation { get; }
public SuspiciousCastFromCharToIntFixer Fixer { get; }
public CancellationToken CancellationToken { get; }
}

private abstract class OperationFixer
{
protected OperationFixer(IMethodSymbol invokedMethod)
{
InvokedMethod = invokedMethod;
}

public IMethodSymbol InvokedMethod { get; }

public abstract Task<Document> GetChangedDocumentAsync(FixCharCastContext context);
}

/// <summary>
/// Fixes calls to string.Split(char, int, StringSplitOptions) with an implicit char-to-int conversion.
/// </summary>
private sealed class StringSplit_CharInt32StringSplitOptions : OperationFixer
{
private StringSplit_CharInt32StringSplitOptions(IMethodSymbol splitMethod) : base(splitMethod) { }

public static StringSplit_CharInt32StringSplitOptions? Create(Compilation compilation, RequiredSymbols symbols)
{
if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemStringSplitOptions, out var stringSplitOptionsType))
return null;

var splitMethod = symbols.StringType.GetMembers(nameof(string.Split)).OfType<IMethodSymbol>()
.GetFirstOrDefaultMemberWithParameterTypes(symbols.CharType, symbols.Int32Type, stringSplitOptionsType);

return splitMethod is not null ? new StringSplit_CharInt32StringSplitOptions(splitMethod) : null;
}

public override async Task<Document> GetChangedDocumentAsync(FixCharCastContext context)
{
var doc = context.CodeFixContext.Document;
var token = context.CancellationToken;
var invocation = (IInvocationOperation)context.Operation;
var editor = await DocumentEditor.CreateAsync(doc, token).ConfigureAwait(false);

var charTypeExpressionSyntax = editor.Generator.TypeExpression(invocation.SemanticModel.Compilation.GetSpecialType(SpecialType.System_Char));
var arguments = invocation.Arguments.GetArgumentsInParameterOrder();
var elementNodes = new[]
{
arguments[0].Value.Syntax,
arguments[1].Value.Syntax
};
var arrayCreationSyntax = editor.Generator.ArrayCreationExpression(charTypeExpressionSyntax, elementNodes);
var memberAccessSyntax = context.Fixer.GetMemberAccessExpressionSyntax(invocation.Syntax);
var newInvocationSyntax = editor.Generator.InvocationExpression(memberAccessSyntax, arrayCreationSyntax, arguments[2].Syntax);

editor.ReplaceNode(invocation.Syntax, newInvocationSyntax);

return editor.GetChangedDocument();
}
}

/// <summary>
/// Fixes calls to String.Split(string, int, StringSplitOptions) with an implicit char-to-int conversion.
/// </summary>
private sealed class StringSplit_StringInt32StringSplitOptions : OperationFixer
{
private StringSplit_StringInt32StringSplitOptions(IMethodSymbol splitMethod) : base(splitMethod) { }

public static StringSplit_StringInt32StringSplitOptions? Create(Compilation compilation, RequiredSymbols symbols)
{
if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemStringSplitOptions, out var stringSplitOptionsType))
return null;

var splitMethod = symbols.StringType.GetMembers(nameof(string.Split)).OfType<IMethodSymbol>()
.GetFirstOrDefaultMemberWithParameterTypes(symbols.StringType, symbols.Int32Type, stringSplitOptionsType);

return splitMethod is not null ? new StringSplit_StringInt32StringSplitOptions(splitMethod) : null;
}

public override async Task<Document> GetChangedDocumentAsync(FixCharCastContext context)
{
var editor = await DocumentEditor.CreateAsync(context.CodeFixContext.Document, context.CancellationToken).ConfigureAwait(false);
var invocation = (IInvocationOperation)context.Operation;

var stringType = invocation.SemanticModel.Compilation.GetSpecialType(SpecialType.System_String);
var stringTypeExpressionSyntax = editor.Generator.TypeExpression(stringType);

var arguments = invocation.Arguments.GetArgumentsInParameterOrder();
var elementNodes = new[]
{
arguments[0].Value.Syntax,
ConvertCharOperationToString(editor, ((IConversionOperation)arguments[1].Value).Operand)
};
var arrayCreationSyntax = editor.Generator.ArrayCreationExpression(stringTypeExpressionSyntax, elementNodes);

var optionsArgumentSyntax = arguments[2].ArgumentKind is ArgumentKind.DefaultValue ?
CreateEnumMemberAccess((INamedTypeSymbol)arguments[2].Parameter.Type, nameof(StringSplitOptions.None)) :
arguments[2].Syntax;

var splitMemberAccessSyntax = context.Fixer.GetMemberAccessExpressionSyntax(context.Operation.Syntax);
var newInvocationSyntax = editor.Generator.InvocationExpression(splitMemberAccessSyntax, arrayCreationSyntax, optionsArgumentSyntax);

editor.ReplaceNode(context.Operation.Syntax, newInvocationSyntax);

return editor.GetChangedDocument();

// Local functions

SyntaxNode CreateEnumMemberAccess(INamedTypeSymbol enumType, string enumMemberName)
{
RoslynDebug.Assert(editor is not null);

var enumTypeSyntax = editor.Generator.TypeExpressionForStaticMemberAccess(enumType);
return editor.Generator.MemberAccessExpression(enumTypeSyntax, enumMemberName);
}
}
}

/// <summary>
/// Fixes calls to new StringBuilder(int) with an implicit char-to-int conversion.
/// </summary>
private sealed class StringBuilderCtor_Int32 : OperationFixer
{
private StringBuilderCtor_Int32(IMethodSymbol ctor) : base(ctor) { }

public static StringBuilderCtor_Int32? Create(Compilation compilation, RequiredSymbols symbols)
Copy link
Contributor

Choose a reason for hiding this comment

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

TryCreate?

{
if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemTextStringBuilder, out var stringBuilderType))
return null;

var ctor = stringBuilderType.GetMembers(WellKnownMemberNames.InstanceConstructorName).OfType<IMethodSymbol>()
.GetFirstOrDefaultMemberWithParameterTypes(symbols.Int32Type);

return ctor is not null ? new StringBuilderCtor_Int32(ctor) : null;
}

public override async Task<Document> GetChangedDocumentAsync(FixCharCastContext context)
{
var editor = await DocumentEditor.CreateAsync(context.CodeFixContext.Document, context.CancellationToken).ConfigureAwait(false);
var instanceCreation = (IObjectCreationOperation)context.Operation;
var argument = instanceCreation.Arguments[0];

var newArgumentSyntax = ConvertCharOperationToString(editor, ((IConversionOperation)argument.Value).Operand);
var newObjectCreationSyntax = editor.Generator.ObjectCreationExpression(instanceCreation.Constructor.ContainingType, newArgumentSyntax);

editor.ReplaceNode(instanceCreation.Syntax, newObjectCreationSyntax);

return editor.GetChangedDocument();
}
}

private static ImmutableDictionary<IMethodSymbol, OperationFixer> CreateFixerCache(Compilation compilation)
{
var symbols = new RequiredSymbols(compilation);
var builder = ImmutableDictionary.CreateBuilder<IMethodSymbol, OperationFixer>();

AddIfNotNull(StringSplit_CharInt32StringSplitOptions.Create(compilation, symbols));
AddIfNotNull(StringSplit_StringInt32StringSplitOptions.Create(compilation, symbols));
AddIfNotNull(StringBuilderCtor_Int32.Create(compilation, symbols));

return builder.ToImmutable();

// Local functions

void AddIfNotNull(OperationFixer? fixer)
{
if (fixer is not null)
builder!.Add(fixer.InvokedMethod, fixer);
}
}

private static SyntaxNode ConvertCharOperationToString(DocumentEditor editor, IOperation charOperation)
{
if (charOperation is ILiteralOperation charLiteral)
{
return editor.Generator.LiteralExpression(charLiteral.ConstantValue.Value.ToString());
}

var toStringMemberAccessSyntax = editor.Generator.MemberAccessExpression(charOperation.Syntax, nameof(object.ToString));

return editor.Generator.InvocationExpression(toStringMemberAccessSyntax);
}
}
}
Loading