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 incorrect position for Less in declaration-no-important #5438

Closed
nwalters512 opened this issue Aug 9, 2021 · 9 comments
Closed

Fix incorrect position for Less in declaration-no-important #5438

nwalters512 opened this issue Aug 9, 2021 · 9 comments
Labels
status: wip is being worked on by someone syntax: less relates to Less and Less-like syntax type: bug a problem with a feature or rule upstream relates to an upstream package

Comments

@nwalters512
Copy link

nwalters512 commented Aug 9, 2021

Clearly describe the bug

For files with specifically formatted comments, the declaration-no-import rule reports errors at the incorrect location in the document. Here's one example:

Screen Shot 2021-08-09 at 4 16 32 PM

Which rule, if any, is the bug related to?

I was able to reproduce with declaration-no-import, though I can't rule out that something similar doesn't happen with other rules.

What code is needed to reproduce the bug?

.tab {
  // 'tis a comment with an apostrophe!
  outline-width: 0 !important;
}

/** 'tis another comment with an apostrophe! */

This is about as minimal as I could get and still reproduce it. A few things to note:

  • The problem does not occur if the first comment is changed from a LESS double-slash comment to /* ... */
  • The problem does not occur if the single quote (') is removed from either comment

What stylelint configuration is needed to reproduce the bug?

{
  "rules": {
    "declaration-no-important": true
  }
}

Which version of stylelint are you using?

13.13.1

How are you running stylelint: CLI, PostCSS plugin, Node.js API?

For the reproduction, I invoked the stylelint CLI with stylelint path/to/file.less

Does the bug relate to non-standard syntax (e.g. SCSS, Less etc.)?

Yes, it appears to be related to LESS double-slash comments (// ...)

What did you expect to happen?

I expected the declaration-no-important rule to flag an error on the !important keyword on line 3.

What actually happened (e.g. what warnings or errors did you get)?

The error was flagged on the word "with" on line 2.

@jeddy3 jeddy3 changed the title declaration-no-import reports error at incorrect location for files with certain comments Fix incorrect position for Less in declaration-no-important Aug 10, 2021
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone syntax: less relates to Less and Less-like syntax type: bug a problem with a feature or rule labels Aug 10, 2021
@jeddy3
Copy link
Member

jeddy3 commented Aug 10, 2021

@nwalters512 Thanks for the report, using the template and for the initial sleuthing (especially identifying that a single quote is involved somehow).

I can reproduce the issue for Less using the demo. It doesn't seem to be an issue with the Scss parser. The issue could be with the rule, the upstream parser (postcss-less) or the interplay between these.

I'll label as bug. Please consider contributing if you have time. I believe postcss-less is maintained by a single volunteer who would be grateful for help.

There are steps on how to fix a bug in a rule in the Developer guide.

(Branch from v14 rather than master).

@nwalters512
Copy link
Author

@jeddy3 Thanks for the links to the demo reproduction and ruling out the Scss parser. I'll attempt to contribute a fix - I can target v14, but if the issue ends up being in stylelint, would it be possible to to backport it to v13? If it's in postcss-less, I should be able to move forward without any more work from the stylelint maintainers.

@jeddy3
Copy link
Member

jeddy3 commented Aug 10, 2021

I can target v14, but if the issue ends up being in stylelint, would it be possible to to backport it to v13?

Probably not, I'm afraid. We really only have the capacity to maintain one branch as we've just a handful of volunteers working on stylelint, and so we haven't committed to master in a while. There are quite a few other issues in the 14.0.0 changelog that could also be backported, so if we backport this fix then we open the gates for the others. I think we really only have time to push on with v14, and hopefully once the changelog swells large enough there'll be an impetus to put the finishes touches to 14.0.0 and get it out the door.

@nwalters512
Copy link
Author

Understood! Really appreciate all the effort you and the other volunteers put into this tool.

@nwalters512
Copy link
Author

nwalters512 commented Aug 10, 2021

I think I've narrowed this down to an issue with the postcss-less parser. Considering the following LESS string:

const less = `.a {\n  // '\n  outline-width: 0 !important;\n}\n\n/** ' */`;

When parsed, this produces the following (JSON-stringified) AST:

{
  "raws": {
    "semicolon": false,
    "after": ""
  },
  "type": "root",
  "nodes": [
    {
      "raws": {
        "before": "",
        "between": " ",
        "semicolon": true,
        "after": "\\n"
      },
      "type": "rule",
      "nodes": [
        {
          "raws": {
            "before": "\\n  ",
            "begin": "//",
            "left": " ",
            "right": ""
          },
          "type": "comment",
          "source": {
            "inputId": 0,
            "start": {
              "offset": 7,
              "line": 2,
              "column": 3
            },
            "end": {
              "offset": 7,
              "line": 2,
              "column": 3
            }
          },
          "inline": true,
          "text": "'"
        },
        {
          "raws": {
            "before": "\\n  ",
            "between": ": "
          },
          "type": "decl",
          "source": {
            "inputId": 0,
            "start": {
              "offset": 3,
              "line": 2,
              "column": 3
            },
            "end": {
              "offset": 30,
              "line": 2,
              "column": 30
            }
          },
          "prop": "outline-width",
          "important": true,
          "value": "0"
        }
      ],
      "source": {
        "inputId": 0,
        "start": {
          "offset": 0,
          "line": 1,
          "column": 1
        },
        "end": {
          "offset": 32,
          "line": 3,
          "column": 1
        }
      },
      "selector": ".a"
    },
    {
      "raws": {
        "before": "\\n\\n",
        "left": "",
        "right": " "
      },
      "type": "comment",
      "source": {
        "inputId": 0,
        "start": {
          "offset": 35,
          "line": 5,
          "column": 1
        },
        "end": {
          "offset": 42,
          "line": 5,
          "column": 8
        }
      },
      "text": "* '"
    }
  ],
  "source": {
    "inputId": 0,
    "start": {
      "offset": 0,
      "line": 1,
      "column": 1
    }
  },
  "inputs": [
    {
      "hasBOM": false,
      "css": ".a {\\n  // '\\n  outline-width: 0 !important;\\n}\\n\\n/** ' */",
      "id": "<input css lE58i->"
    },
    {
      "hasBOM": false,
      "css": "\\n  outline-width: 0 !important;\\n}\\n\\n/** ' */",
      "id": "<input css YU0Mvn>"
    }
  ]
}

Notice this node in particular:

{
  "raws": {
    "before": "\\n  ",
    "between": ": "
  },
  "type": "decl",
  "source": {
    "inputId": 0,
    "start": {
      "offset": 3,
      "line": 2,
      "column": 3
    },
    "end": {
      "offset": 30,
      "line": 2,
      "column": 30
    }
  },
  "prop": "outline-width",
  "important": true,
  "value": "0"
}

I would expect source.line to be 3, but it's actually 2. The column and offset values are similarly incorrect. Interestingly, the resulting root node has an inputs array with two elements, and the line/offset values seem to match the second one. This might be a problem upstream in postcss... I'll keep digging.

@nwalters512
Copy link
Author

Ahh, this looks like it's probably relevant: shellscape/postcss-less#154. That explains where the second input comes from!

@jeddy3
Copy link
Member

jeddy3 commented Aug 10, 2021

Ahh, this looks like it's probably relevant: shellscape/postcss-less#154.

Yes, very likely related.

I think I've narrowed this down to an issue with the postcss-less parser.

I'll label as upstream.

v14 is compatible with postcss-less > 4.0.0 (as it uses postcss@8), so you'll want to use that branch to validate any fixes upstream.

@nwalters512
Copy link
Author

I reported this upstream: shellscape/postcss-less#163. Still trying to work on a fix for it!

@nwalters512
Copy link
Author

Aaaaaand my attempt at a fix: shellscape/postcss-less#164

@jeddy3 jeddy3 added status: wip is being worked on by someone and removed status: ready to implement is ready to be worked on by someone labels Aug 12, 2021
@jeddy3 jeddy3 closed this as completed Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone syntax: less relates to Less and Less-like syntax type: bug a problem with a feature or rule upstream relates to an upstream package
Development

No branches or pull requests

2 participants