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.

Sign up for GitHub

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

Jump to bottom

SE: Add Plus- and MinusOperation for literals #7174

Merged
merged 6 commits into from
May 11, 2023
Merged

SE: Add Plus- and MinusOperation for literals #7174

merged 6 commits into from
May 11, 2023

Conversation

Tim-Pohlmann
Copy link
Contributor

Part 1 of #7141

@github-actions github-actions bot added this to In progress in Best Kanban May 4, 2023
Best Kanban automation moved this from In progress to Done May 4, 2023
@Tim-Pohlmann Tim-Pohlmann moved this from Done to In progress in Best Kanban May 4, 2023
@Tim-Pohlmann Tim-Pohlmann added the Sprint: SE Short-lived* label for epic MMF-3077 *troll label May 4, 2023
@Tim-Pohlmann Tim-Pohlmann reopened this May 5, 2023
@Tim-Pohlmann
Copy link
Contributor Author

I added elementary tests only because the engine is still very limited. We need to add more in-depth tests later when it is easier to create NumberConstraints with different ranges.

@Tim-Pohlmann Tim-Pohlmann force-pushed the Tim/PlustMinusOperation branch 2 times, most recently from 0750b3d to d048521 Compare May 5, 2023 09:45
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban May 5, 2023
@Tim-Pohlmann Tim-Pohlmann marked this pull request as ready for review May 5, 2023 13:21
@pavel-mikula-sonarsource
Copy link
Contributor

I added elementary tests only because the engine is still very limited. We need to add more in-depth tests later when it is easier to create NumberConstraints with different ranges.

Can you add it as a checkbox on the issue itself? We can park the card back to "ToDo" if the ranges are not ready in time.

pavel-mikula-sonarsource
pavel-mikula-sonarsource requested changes May 5, 2023
View reviewed changes
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

Round 1 :)

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Binary.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Binary.cs Outdated Show resolved Hide resolved
.../tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Binary.cs Outdated Show resolved Hide resolved
.../tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Binary.cs Outdated Show resolved Hide resolved
.../tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Binary.cs Outdated Show resolved Hide resolved
.../tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Binary.cs Outdated Show resolved Hide resolved
.../tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Binary.cs Outdated Show resolved Hide resolved
@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban May 5, 2023
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban May 8, 2023
@Tim-Pohlmann Tim-Pohlmann moved this from Review in progress to In progress in Best Kanban May 8, 2023
@Tim-Pohlmann
Copy link
Contributor Author

I had to rewrite part of the code because I realized that we need to swap the order of the operators for substractions.

@Tim-Pohlmann Tim-Pohlmann mentioned this pull request May 9, 2023
Closed
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban May 9, 2023
pavel-mikula-sonarsource
pavel-mikula-sonarsource requested changes May 10, 2023
View reviewed changes
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

One more round of polishing of the collateral stuff

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/NumberConstraint.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Constraints/NumberConstraintTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Constraints/NumberConstraintTest.cs Outdated Show resolved Hide resolved
...rAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/LocksReleasedAllPaths.Monitor.Loops.vb
Comment on lines +16 to +17
Monitor.Enter(Other) ' FN, exploration stopped after loop
If Condition Then Monitor.Exit(Other)
Copy link
Contributor

Choose a reason for hiding this comment

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

Every lost Noncompliant should have these two lines too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add it. Can you elaborate on your reason, though? I am trying to understand why the comment is insufficient for the other cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

To demonstrate, that we stop exploring whatever follows. There are two problems:

  • 1st related to the content of the For loop. And that is FN for one reason.
  • 2nd related to the fact that we don't explore code after For loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The two cases that did not have the additional two lines are not FNs or FPs, though. They are just regular Noncompliant cases.

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/ProgramState.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Binary.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Binary.cs Show resolved Hide resolved
@pavel-mikula-sonarsource pavel-mikula-sonarsource moved this from Review in progress to In progress in Best Kanban May 10, 2023
@Tim-Pohlmann Tim-Pohlmann moved this from In progress to Review in progress in Best Kanban May 10, 2023
@sonarcloud
Copy link

sonarcloud bot commented May 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented May 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

pavel-mikula-sonarsource
pavel-mikula-sonarsource approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-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

@pavel-mikula-sonarsource pavel-mikula-sonarsource merged commit 81627e0 into master May 11, 2023
27 checks passed
Best Kanban automation moved this from Review in progress to Validate Peach May 11, 2023
@pavel-mikula-sonarsource pavel-mikula-sonarsource deleted the Tim/PlustMinusOperation branch May 11, 2023 08:26
@pavel-mikula-sonarsource pavel-mikula-sonarsource moved this from Validate Peach to Done in Best Kanban May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavel-mikula-sonarsource pavel-mikula-sonarsource pavel-mikula-sonarsource approved these changes

Assignees

@Tim-Pohlmann Tim-Pohlmann

Labels
Sprint: SE Short-lived* label for epic MMF-3077 *troll
Projects
Best Kanban
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Tim-Pohlmann @pavel-mikula-sonarsource