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

Fix S3900 FP: Recognize VB extensions #7047

Merged
merged 3 commits into from
Apr 13, 2023

Conversation

pavel-mikula-sonarsource
Copy link
Contributor

Follow up of #6797

This is a draft, rebase will be needed. Squash S3900-VB commit doesn't need a review, it's in #7045

Copy link
Contributor

@antonioaversa antonioaversa left a comment

Choose a reason for hiding this comment

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

Mostly doubts on the behavior of the rule, by looking at the ITs.

The review took a while due to the fact that a lot of changes appearing in this PR actually belong to another PR (#7045), which is also in the review phase, and the related commit (Squash S3900-VB) had to be taken out from the diff, while reviewing.

I think it would have been easier for this PR to target the branch of #7045, rather than master. This way there would have not been the need of squashing 7045 content into a single commit and asking for the review of the difference, since the full PR diff (and the Changes tab on GitHub) would have reflected the actual content of this PR.

Concerning ITs: it was difficult to only review the difference between the JSON files, as they appear in 7045, and the JSON as they appear here, so I went through all of them in this PR anyway. Most likely some comments will apply to 7045 instead.

@@ -0,0 +1,264 @@
{
"issues": [
{
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 a bit confused by how S3900 is working here.

This corresponds to the line:

sFormula.Replace("(", String.Empty).Length = sFormula.Replace(")", String.Empty).Length

Do you know why only the first sFormula dereference is detected, and not the second, yet the third is (line 169)?

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 = is a comparison operator. It learns NotNull from evaluating the left operand, and by the time it evaluates the right one, it already knows NotNull.
This seems fine to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the rule not reporting on the right side of the comparison, after having learnt NotNull on the left side, is fine to me too. What still confuses me is line 169:

If sFormula.Replace("(", String.Empty).Length = sFormula.Replace(")", String.Empty).Length Then
    'step 2: all code between brackets
    For Each mReg In Regex.Matches(sFormula, "\((.+)\)")
        rResult = EvaluateFormula(mReg.Groups(1).ToString())
        sFormula = sFormula.Replace(mReg.ToString(), rResult.ToString("0.00"))   ' Line 169
    Next

Inside the If, sFormula has ObjectConstraint.NotNull, and it seems to me that there is nothing that should make the engine forget about such constraint. Is it linked to the fact that the loop is executed twice, and sFormula is assigned the result of sFormula.Replace, making sFormula unknown at the second iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

@sonarcloud
Copy link

sonarcloud bot commented Apr 13, 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 Apr 13, 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

Copy link
Contributor

@antonioaversa antonioaversa left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for the clarification, now the functionality is much more clear to me!
One doubt left: #7047 (comment)

@antonioaversa antonioaversa merged commit 1ae5f28 into master Apr 13, 2023
@antonioaversa antonioaversa deleted the Pavel/SE/S3900-VB-Extension branch April 13, 2023 15:23
@martin-strecker-sonarsource martin-strecker-sonarsource added this to the 8.56 milestone Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants