sonar-css-plugin icon indicating copy to clipboard operation
sonar-css-plugin copied to clipboard

scss:unknown-at-rules creates false positives for @if rules when conditions contains "&&"

Open agabrys opened this issue 7 years ago • 12 comments

Hello, The plugin reported false positives for @if rules when conditions contains && characters.

unknown-rule

Environment:

  • CSS / SCSS / Less plugin: 3.1
  • SonarQube: 5.6.7

Example file: https://github.com/agabrys/sonarqube-falsepositives/blob/master/src/main/sass/d20171218/unknown-at-rules.scss

Project: https://github.com/agabrys/sonarqube-falsepositives

Build: mvn clean package sonar

Best Regards

agabrys avatar Dec 18 '17 08:12 agabrys

Hi @agabrys,

Could you please upgrade to the latest version (4.13) and tell me if you can reproduce?

Thank you

David

racodond avatar Dec 18 '17 08:12 racodond

Hi David, Thank you for fast answer.

Yes, I'll check it and let you know tomorrow. I didn't know that the latest version is 4.13 because the latest version available in SonarQube Update Center is 3.1.

agabrys avatar Dec 18 '17 09:12 agabrys

OK. Thanks!

As a side note, all my plugins are no longer available in the Update Center: CSS, JSON, Cucumber Gherkin, Java Properties.

racodond avatar Dec 18 '17 09:12 racodond

I've tested the same code on:

  • CSS / SCSS / Less plugin: 4.13
  • SonarQube: 6.7

and I see the same problem:

false-positive

agabrys avatar Dec 18 '17 21:12 agabrys

Thanks! I'll investigate and keep you posted.

racodond avatar Dec 18 '17 22:12 racodond

@agabrys: Indeed the && operator is not supported by the plugin. Only the and operator is. I can't find anything about an && operator at http://sass-lang.com/documentation/file.SASS_REFERENCE.html. Is it really supported by SCSS?

racodond avatar Dec 22 '17 16:12 racodond

I changed the example code to:

$variable1: true;
$variable2: false;

.queries {
    &-ok {
        @if $variable1 == $variable2 {
            color: blue;
        } @else {
            color: red;
        }
    }
    &-fp {
        @if $variable1 && $variable2 {
            color: blue;
        } @else {
            color: red;
        }
    }
    &-ok2 {
        @if $variable1 and $variable2 {
            color: blue;
        } @else {
            color: red;
        }
    }
}

SonarQube created issues only in &-fp block. I checked a generated file and it contains:

.queries-ok {
  color: red; }
.queries-fp {
  color: blue; }
.queries-ok2 {
  color: red; }

SCSS treats true && false as true. I checked also false && true and the result is the same: true. It means that the && operator is not supported but unfortunately it doesn't break the compilation process.

The rule works correctly but I think the message is misleading. Maybe this rule should ignore that code and the "SCSS parser failure" rule should raise an issue? What do you think?

agabrys avatar Dec 24 '17 12:12 agabrys

Hi @agabrys,

Thanks for your feedback!

SCSS treats true && false as true. I checked also false && true and the result is the same: true. It means that the && operator is not supported but unfortunately it doesn't break the compilation process.

Indeed, it would be nicer if the SCSS parser failed while encountering this kind of syntax. This is something that you could make the SCSS team aware of.

The rule works correctly but I think the message is misleading.

It might be misleading but a prerequisite of the SonarQube plugin is that the files to parse should be syntactically valid.

Maybe this rule should ignore that code and the "SCSS parser failure" rule should raise an issue? What do you think?

@if $variable1 && $variable2 {
    color: blue;
}

Strictly speaking, this piece of code is valid CSS code. So, I don't really want to flag it as invalid syntax and raise an issue against "SCSS parser failure". Also, I don't really want to tweak the "scss:unknown-at-rules" rule to not raise this specific issue because strictly CSS-speaking this finding is kind of correct.

David

racodond avatar Dec 26 '17 17:12 racodond

I understand your point of view and I agree with it 👍 I'll let SCSS team know about this issue.

Thank you, I'm closing the ticket.

agabrys avatar Dec 26 '17 19:12 agabrys

I created an issue in the SASS tracking system: sass/sass#2429

agabrys avatar Dec 29 '17 13:12 agabrys

@racodond Natalie Weizenbaum from SASS team wrote::

This is valid syntax—true && false is parsed as a space-separated list containing true, &, &, and false. It may be worth adding a warning for &&, though, since it certainly looks confusing.

So the rule has an issue, but different than I thought at the beginning.

agabrys avatar Jan 04 '18 21:01 agabrys

@agabrys: Thanks for the follow-up! It is really a corner case but I'll try to look at it as soon as I get some free time.

racodond avatar Jan 08 '18 14:01 racodond