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

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

Open
agabrys opened this issue Dec 18, 2017 · 12 comments
Labels

Comments

@agabrys
Copy link

agabrys commented Dec 18, 2017

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

@racodond
Copy link
Owner

Hi @agabrys,

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

Thank you

David

@agabrys
Copy link
Author

agabrys commented Dec 18, 2017

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.

@racodond
Copy link
Owner

OK. Thanks!

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

@agabrys
Copy link
Author

agabrys commented Dec 18, 2017

I've tested the same code on:

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

and I see the same problem:

false-positive

@racodond
Copy link
Owner

Thanks! I'll investigate and keep you posted.

@racodond
Copy link
Owner

@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?

@agabrys
Copy link
Author

agabrys commented Dec 24, 2017

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?

@racodond
Copy link
Owner

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

@agabrys
Copy link
Author

agabrys commented Dec 26, 2017

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 agabrys closed this as completed Dec 26, 2017
@agabrys
Copy link
Author

agabrys commented Dec 29, 2017

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

@agabrys
Copy link
Author

agabrys commented Jan 4, 2018

@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.

@racodond
Copy link
Owner

racodond commented Jan 8, 2018

@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 racodond added bug and removed question labels Jan 8, 2018
@racodond racodond added this to the 4.14 milestone Jan 8, 2018
@racodond racodond reopened this Jan 8, 2018
@racodond racodond removed this from the 4.14 milestone Mar 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants