Skip to content

Fix S6966 FPs/FNs: FPs/FNs after peach validation#9222

Merged
mary-georgiou-sonarsource merged 8 commits intomasterfrom
Martin/S6966_FPs
May 3, 2024
Merged

Fix S6966 FPs/FNs: FPs/FNs after peach validation#9222
mary-georgiou-sonarsource merged 8 commits intomasterfrom
Martin/S6966_FPs

Conversation

@martin-strecker-sonarsource
Copy link
Contributor

@martin-strecker-sonarsource martin-strecker-sonarsource commented Apr 29, 2024

S6966 - Awaitable method should be used

See #9179 (comment)

  • Mongo DB: 3868cc1 FP. --> Documented but not fixed.
  • MemoryStream.Flush: Not fixed. Flush and FlushAsync are both "No ops" (see Remark section for both). So Flush is wrong to begin with and FlushAsync is equally wrong. Other Async methods delegate to their sync counterpart and may be worth excluding (WriteAsync, ReadAsync). We can not exclude MemoryStream altogether, because CopyToAsync is a TP (calls WriteAsync of the destination stream). --> No action needed
  • Endless recursion (The speculative resolved method symbol is the enclosing method). --> Fixed and tested d92f3fc

@martin-strecker-sonarsource martin-strecker-sonarsource marked this pull request as ready for review April 30, 2024 09:12
Copy link
Contributor

@mary-georgiou-sonarsource mary-georgiou-sonarsource left a comment

Choose a reason for hiding this comment

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

Some suggestions and I think we miss a UT.

if (original == awaitableRoot && result is ExpressionSyntax resultExpression)
{
result = SyntaxFactory.AwaitExpression(resultExpression);
result = SyntaxFactory.ParenthesizedExpression(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is is possible to add a test that fails if this line is not changed to add parentheses around the replacement?

Copy link
Contributor Author

@martin-strecker-sonarsource martin-strecker-sonarsource May 2, 2024

Choose a reason for hiding this comment

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

No. This is not possible. This is the funny part with SyntaxTree manipulations: You can end up with a syntax tree that is not expressible in code (If you take the transformed syntax tree, convert it to code, and convert it back to a tree, the new tree looks different). The parentheses are not needed in the tree because the tree looks like parentheses are present. The parentheses will be needed for the code fix though.
With the parentheses, debugging is a bit better because the text representation of the syntax tree is better.
I can remove all the trivia and parentheses handling because, for the speculative binding, they are not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a quick comment there to capture this?
On why there are parentheses.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll merge this to make it to the next release. Let's add a comment in one of the next PRs.
I'll keep a task on my calendar to not forget.

@martin-strecker-sonarsource martin-strecker-sonarsource added Sprint: ASP.NET MVC False Positive Rule IS triggered when it shouldn't be. labels May 2, 2024
Copy link
Contributor

@mary-georgiou-sonarsource mary-georgiou-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM - there's couple more suggestions.
There's also a code smell for an unused parameter stayed after refactoring.

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 2, 2024

Quality Gate Passed Quality Gate passed for 'Sonar .NET Java Plugin'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 2, 2024

@martin-strecker-sonarsource martin-strecker-sonarsource enabled auto-merge (squash) May 2, 2024 12:55
auto-merge was automatically disabled May 2, 2024 15:09

Base branch was modified

@mary-georgiou-sonarsource mary-georgiou-sonarsource merged commit bc4c63b into master May 3, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource deleted the Martin/S6966_FPs branch May 3, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

False Positive Rule IS triggered when it shouldn't be.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments