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

CalculationsShouldNotOverflow.SyntaxKindWalker reduce allocations and evaluations in the hot path #8103

Closed
martin-strecker-sonarsource opened this issue Sep 27, 2023 · 0 comments · Fixed by #8133
Assignees
Labels
Area: C# C# rules related issues. Area: CFG/SE CFG and SE related issues. Type: False Negative Rule is NOT triggered when it should be.
Projects
Milestone

Comments

@martin-strecker-sonarsource
Copy link
Contributor

There are two things wrong with the walker:

if (!IsUnchecked && !HasOverflow)
{
IsUnchecked = node.IsAnyKind(SyntaxKind.UncheckedStatement, SyntaxKind.UncheckedExpression);
HasOverflow = node.IsAnyKind(
SyntaxKind.AddExpression,
SyntaxKind.AddAssignmentExpression,
SyntaxKind.MultiplyExpression,
SyntaxKind.MultiplyAssignmentExpression,
SyntaxKind.SubtractExpression,
SyntaxKind.SubtractAssignmentExpression,
SyntaxKind.PostDecrementExpression,
SyntaxKind.PostIncrementExpression,
SyntaxKind.PreDecrementExpression,
SyntaxKind.PreIncrementExpression);
base.Visit(node);
}

Flawed logic

IsUnchecked is reset too early here/at the wrong position or the IsUnchecked field is not needed at all. It should be reset after the unchecked block more like so:

        public override void Visit(SyntaxNode node)
        {
            if (!IsUnchecked && !HasOverflow)
            {
                HasOverflow = node.IsAnyKind(
                    SyntaxKind.AddExpression,
                    SyntaxKind.AddAssignmentExpression,
                    SyntaxKind.MultiplyExpression,
                    SyntaxKind.MultiplyAssignmentExpression,
                    SyntaxKind.SubtractExpression,
                    SyntaxKind.SubtractAssignmentExpression,
                    SyntaxKind.PostDecrementExpression,
                    SyntaxKind.PostIncrementExpression,
                    SyntaxKind.PreDecrementExpression,
                    SyntaxKind.PreIncrementExpression);
                base.Visit(node);
            }
        }

        public override void VisitCheckedExpression(CheckedExpressionSyntax node) // same for VisitCheckedStatement
        {
            IsUnchecked = node.IsKind(SyntaxKind.UncheckedExpression); // or dont' call base.VisitCheckedExpression
            base.VisitCheckedExpression(node);
            IsUnchecked = false; // this should actually be [CompilationOptions.CheckOverflow](https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.compilationoptions.checkoverflow?view=roslyn-dotnet-4.6.0)
        }

Nested checked/unchecked blocks are potentially wrong (for this to work, IsUnchecked must be a counter).

Array allocation in the hot path

Visit is performance-sensitive and allocates a params array on each invocation. The IsAnyKind should be replaced by a node.Kind() is SyntaxKind.AddExpression or SyntaxKind.AddAssignmentExpression or ... expression:

image

Also affected:

  • SonarAnalyzer.SymbolicExecution.Roslyn.RuleChecks.CSharp.ConditionEvaluatesToConstant+SyntaxKindWalker.Visit
  •  SonarAnalyzer.SymbolicExecution.Roslyn.RuleChecks.CSharp.NullPointerDereference+SyntaxKindWalker.Visit
@martin-strecker-sonarsource martin-strecker-sonarsource added Type: Bug Exceptions and blocking issues during analysis. Area: CFG/SE CFG and SE related issues. Area: C# C# rules related issues. Type: False Negative Rule is NOT triggered when it should be. and removed Type: Bug Exceptions and blocking issues during analysis. labels Sep 27, 2023
@github-actions github-actions bot added this to Review in progress in Best Kanban Oct 4, 2023
@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Oct 13, 2023
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban Oct 16, 2023
@github-actions github-actions bot moved this from Review in progress to Review approved in Best Kanban Oct 17, 2023
@martin-strecker-sonarsource martin-strecker-sonarsource changed the title CalculationsShouldNotOverflow.SyntaxKindWalker contains contradicting conditions CalculationsShouldNotOverflow.SyntaxKindWalker reduce allocations and evaluations in the hot path Oct 19, 2023
Best Kanban automation moved this from Review approved to Validate Peach Oct 19, 2023
@martin-strecker-sonarsource martin-strecker-sonarsource moved this from Validate Peach to Done in Best Kanban Oct 25, 2023
@andrei-epure-sonarsource andrei-epure-sonarsource added this to the 9.13 milestone Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Area: CFG/SE CFG and SE related issues. Type: False Negative Rule is NOT triggered when it should be.
Projects
Best Kanban
  
Done
3 participants