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

Add no-unbalanced-parentheses #3480

Open
alisonailea opened this issue Jul 18, 2018 · 8 comments
Open

Add no-unbalanced-parentheses #3480

alisonailea opened this issue Jul 18, 2018 · 8 comments
Labels
good first issue is good for newcomers status: ask to implement ask before implementing as may no longer be relevant type: new rule an entirely new rule

Comments

@alisonailea
Copy link

alisonailea commented Jul 18, 2018

The Problem

Developers on my team occasionally add extra parenthesis to the end of custom-properties, media-queries, and urls which are not caught by other linting rules and which can cause errors when that CSS is used in other projects.

The Solution

A new rule that checks for extra parenthesis at the end of media queries and custom-properties

Notes

I have a branch in my local clone of Stylelint with this rule already partially written if it is something the community wants I'm happy to finish and submit the PR

Tape test (based off no-extra-semicolons)

"use strict";

const rule = require("..");
const { messages, ruleName } = rule;

testRule(rule, {
  ruleName,
  config: [true],

  accept: [
    {
      code: "a { background: url() }"
    },
    {
      code: "a { background: url('') }"
    },
    {
      code: "a { background: url( ) }"
    },
    {
      code: "a { color: var(--prop) }"
    },
    {
      code: "a { color: (); }"
    },
    {
      code: "a { color: var(--prop);}"
    },
    {
      code: "@media (min-width: var(--prop)) {a { color: var(--prop); }}"
    },
    {
      code: "/* a { color: var(--prop);} */"
    },
    {
      code: "/* a { color: var(--prop)));}; */"
    },
    {
      code: "/* ()comment)) words))(( */"
    },
    {
      code: "a { content: ')'; }"
    },
    {
      code: "a { content: '))'; }"
    },
    {
      code: "a { content: '(\t) )'; }"
    },
    {
      code: ":root { --foo: '('; --bar: '))'; }"
    }
  ],

  reject: [
    {
      code: ")",
      message: messages.rejected,
      line: 1,
      column: 1
    },
    {
      code: " )",
      message: messages.rejected,
      line: 1,
      column: 2
    },
    {
      code: "  )",
      message: messages.rejected,
      line: 1,
      column: 3
    },
    {
      code: "\n)",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "\n\n)",
      message: messages.rejected,
      line: 3,
      column: 1
    },
    {
      code: "\r\n)",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "\r\n\r\n)",
      message: messages.rejected,
      line: 3,
      column: 1
    },
    {
      code: "\r)",
      message: messages.rejected,
      line: 1,
      column: 2
    },
    {
      code: "\r\r)",
      message: messages.rejected,
      line: 1,
      column: 3
    },
    {
      code: "\t)",
      message: messages.rejected,
      line: 1,
      column: 2
    },
    {
      code: "\t\t)",
      message: messages.rejected,
      line: 1,
      column: 3
    },
    {
      code: "a {)}",
      message: messages.rejected,
      line: 1,
      column: 4
    },
    {
      code: "a { )}",
      message: messages.rejected,
      line: 1,
      column: 5
    },
    {
      code: "a {) }",
      message: messages.rejected,
      line: 1,
      column: 4
    },
    {
      code: "a { ) }",
      message: messages.rejected,
      line: 1,
      column: 5
    },
    {
      code: "a {  )}",
      message: messages.rejected,
      line: 1,
      column: 6
    },
    {
      code: "a {)  }",
      message: messages.rejected,
      line: 1,
      column: 4
    },
    {
      code: "a {  )  }",
      message: messages.rejected,
      line: 1,
      column: 6
    },
    {
      code: "a {   )   }",
      message: messages.rejected,
      line: 1,
      column: 7
    },
    {
      code: "a {\n)}",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "a {)\n}",
      message: messages.rejected,
      line: 1,
      column: 4
    },
    {
      code: "a {\n)\n}",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "a {\r\n)}",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "a {)\r\n}",
      message: messages.rejected,
      line: 1,
      column: 4
    },
    {
      code: "a {\r\n)\r\n}",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "a {\t)}",
      message: messages.rejected,
      line: 1,
      column: 5
    },
    {
      code: "a {)\t}",
      message: messages.rejected,
      line: 1,
      column: 4
    },
    {
      code: "a {\t)\t}",
      message: messages.rejected,
      line: 1,
      column: 5
    },
    {
      code: "a { color: var(--prop); })",
      message: messages.rejected,
      line: 1,
      column: 26
    },
    {
      code: "a { color: var(--prop); } )",
      message: messages.rejected,
      line: 1,
      column: 27
    },
    {
      code: "a { color: var(--prop); }\n)",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "a { color: var(--prop); }\n\n)",
      message: messages.rejected,
      line: 3,
      column: 1
    },
    {
      code: "a { color: var(--prop); }\r\n)",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "a { color: var(--prop); }\r\n\r\n)",
      message: messages.rejected,
      line: 3,
      column: 1
    },
    {
      code: "a { color: var(--prop); }\r)",
      message: messages.rejected,
      line: 1,
      column: 19
    },
    {
      code: "a { color: var(--prop); }\r\r)",
      message: messages.rejected,
      line: 1,
      column: 28
    },
    {
      code: "a { color: var(--prop); }\t)",
      message: messages.rejected,
      line: 1,
      column: 27
    },
    {
      code: "a { color: var(--prop); }\t\t)",
      message: messages.rejected,
      line: 1,
      column: 28
    },
    {
      code: ")a { color: var(--prop); }",
      message: messages.rejected,
      line: 1,
      column: 1
    },
    {
      code: ") a { color: var(--prop); }",
      message: messages.rejected,
      line: 1,
      column: 1
    },
    {
      code: ")  a { color: var(--prop); }",
      message: messages.rejected,
      line: 1,
      column: 1
    },
    {
      code: ")\na { color: var(--prop); }",
      message: messages.rejected,
      line: 1,
      column: 1
    },
    {
      code: ")\n\na { color: var(--prop); }",
      message: messages.rejected,
      line: 1,
      column: 1
    },
    {
      code: ")\r\na { color: var(--prop); }",
      message: messages.rejected,
      line: 1,
      column: 1
    },
    {
      code: ")\r\n\r\na { color: var(--prop); }",
      message: messages.rejected,
      line: 1,
      column: 1
    },
    {
      code: ")\ta { color: var(--prop); }",
      message: messages.rejected,
      line: 1,
      column: 1
    },
    {
      code: ")\t\ta { color: var(--prop); }",
      message: messages.rejected,
      line: 1,
      column: 1
    },
    {
      code: "@media ()) { }",
      message: messages.rejected,
      line: 1,
      column: 10
    },
    {
      code: "@media () ) { a { color: var(--prop); } }",
      message: messages.rejected,
      line: 1,
      column: 11
    },
    {
      code: "@media ()  ) { a { color: var(--prop); } }",
      message: messages.rejected,
      line: 1,
      column: 12
    },
    {
      code: "@media (var(--prop))) { a { color: var(--prop); } }",
      message: messages.rejected,
      line: 1,
      column: 21
    },
    {
      code: "@media (var(--prop) )) { a { color: var(--prop); } }",
      message: messages.rejected,
      line: 1,
      column: 22
    },
    {
      code: "@media ( var(--prop) )) { a { color: var(--prop); } }",
      message: messages.rejected,
      line: 1,
      column: 23
    },
    {
      code: "/* comment */)",
      message: messages.rejected,
      line: 1,
      column: 14
    },
    {
      code: "/* comment */ ) /*comment */",
      message: messages.rejected,
      line: 1,
      column: 15
    },
    {
      code: "/* comment */\n)\n/* comment */",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "/* comment */\r\n)\r\n/* comment */",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "/* comment */\t)\t/* comment */",
      message: messages.rejected,
      line: 1,
      column: 15
    },
    {
      code: "a { color: var(--prop)) }",
      message: messages.rejected,
      line: 1,
      column: 23
    },
    {
      code: "a { color: var(--prop) ) }",
      message: messages.rejected,
      line: 1,
      column: 24
    },
    {
      code: "a { color: var(--prop)  ) }",
      message: messages.rejected,
      line: 1,
      column: 25
    },
    {
      code: "a { color: var(--prop)\n) }",
      message: messages.rejected,
      line: 2,
      column: 1
    },

    {
      code: "a { color: var(--prop)\n\n) }",
      message: messages.rejected,
      line: 3,
      column: 1
    },
    {
      code: "a { color: var(--prop)\r\n) }",
      message: messages.rejected,
      line: 2,
      column: 1
    },
    {
      code: "a { color: var(--prop)\r\n\r\n) }",
      message: messages.rejected,
      line: 3,
      column: 1
    },
    {
      code: "a { background: url()) }",
      message: messages.rejected,
      line: 1,
      column: 22
    },
    {
      code: "a { background: url( )) }",
      message: messages.rejected,
      line: 1,
      column: 23
    },
    {
      code: "a { background: url( ) ) }",
      message: messages.rejected,
      line: 1,
      column: 24
    },
    {
      code: "a { background: url('')) }",
      message: messages.rejected,
      line: 1,
      column: 24
    },
    {
      code: "a { background: url(' ') ) }",
      message: messages.rejected,
      line: 1,
      column: 26
    },
    {
      code: "a { @media (min-width: var(--prop))) { color: red; } }",
      message: messages.rejected,
      line: 1,
      column: 36
    },
    {
      code: "a { @media (min-width: var(--prop)) ) { color: red; } }",
      message: messages.rejected,
      line: 1,
      column: 37
    },
    {
      code: "a { @media (min-width: var(--prop) )) { color: red; } }",
      message: messages.rejected,
      line: 1,
      column: 37
    },
    {
      code: "a { @media (min-width: var(--prop ))) { color: red; } }",
      message: messages.rejected,
      line: 1,
      column: 37
    },
    {
      code: "a {\n@media (min-width: var(--prop))) { color: red; } }",
      message: messages.rejected,
      line: 2,
      column: 32
    },
    {
      code: "a {\n@media (\nmin-width: var(--prop))) { color: red; } }",
      message: messages.rejected,
      line: 3,
      column: 24
    },
    {
      code: "a {\n@media (\nmin-width: var(--prop)\n)) { color: red; } }",
      message: messages.rejected,
      line: 4,
      column: 2
    },
  ]
});

testRule(rule, {
  ruleName,
  config: [true],
  syntax: "html",

  accept: [
    {
      code: `<div>
<style>
a {
  color: var(--prop);
}
</style>
</div>`
    },
    {
      code: `<div>
<style>
a {
  background: url();
}
</style>
</div>`
    }
  ],

  reject: [
    {
      code: `<div>
<style>
)
</style>
</div>`,
      message: messages.rejected,
      line: 3,
      column: 1
    },
    {
      code: `<div>
<style>
a {
  color: var(--prop))
}
</style>
</div>`,
      message: messages.rejected,
      line: 4,
      column: 21
    }
  ]
});

testRule(rule, {
  ruleName,
  config: [true],
  syntax: "less",

  accept: [
    {
      code: "a { .mixin(); .mixin2; }"
    },
  ],

  reject: [
    {
      code: "a { .mixin());\ncolor: red; }",
      message: messages.rejected,
      line: 1,
      column: 13
    }
  ]
});
@alexander-akait
Copy link
Member

@alisonailea Sound great idea for rule for me

@hudochenkov hudochenkov added the status: needs discussion triage needs further discussion label Jul 18, 2018
@hudochenkov
Copy link
Member

Looks like a good idea! And it meets our criteria for inclusion. Let's wait for @jeddy3, because he usually sees what other miss :)

Even if it won't make to the core, it would make a good plugin.

@ntwb
Copy link
Member

ntwb commented Jul 19, 2018

Does https://github.com/csstree/stylelint-validator detect this?

Looking at https://csstree.github.io/docs/validator.html many of the examples in the test cases above do throw Parse error: Identifier is expected warnings.

@jeddy3
Copy link
Member

jeddy3 commented Jul 19, 2018

Does https://github.com/csstree/stylelint-validator detect this?

In its current form, it:

  • will in the value of declarations e.g. functions like calc, url and var etc
  • won't in the params of at-rules e.g. media and supports etc

CSSTree throws a parse error for the former.

It looks like the PostCSS parser itself will throw a Unclosed bracket or Unknown word parse error for any extra parenthesis outside of the params of an at-rule or the value of a declaration.

In the VISION we have:

Invalid code is best handled by emerging dedicated tools e.g. csstree - a language parser with syntax validation. As a stop-gap, while these tools mature, provide invalid code rules for the simplest of cases.

As the parser itself covers off most instances, I think this will be one of those "simplest cases" and we can add this rule as a stop-gap for the extra parenthesis in the:

  1. params of at-rules for everyone
  2. value of declarations for users of non-standard syntax

@alisonailea Thank you for offering to contribute this rule. You'll find guidance on how to do this in the Developer Guide.

I think you can strip the tests right back, though. Just focus on standard CSS syntax e.g. extra parens in @media, @support, calc, url and var. You can throw in a couple of SCSS syntax tests for @include and @mixin, though.

  • Name: no-extra-parenthesis
  • Primary option: true
  • Secondary options: none
  • Message: "Unexpected extra parenthesis"
  • Section: "Possible errors" -> "General / Sheet"

@jeddy3 jeddy3 changed the title no-extra-parenthesis Add no-extra-parenthesis Jul 19, 2018
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: new rule an entirely new rule good first issue is good for newcomers and removed status: needs discussion triage needs further discussion labels Jul 19, 2018
@jeddy3
Copy link
Member

jeddy3 commented Jul 19, 2018

Having thought a little more about this, I think we should limit the scope of this rule to unbalanced parentheses. This is the use case @alisonailea originally describes.

The word extra implies redundant, which would broaden the scope of this rule to include balanced but unnecessary parentheses, e.g. calc((10px) + (10px)), and that isn't viable. This wasn't an issue with semicolons, hence the name no-extra-semicolons.

We should also pluralise the rule name to be consistent with no-extra-semicolons:

  • Name: no-unbalanced-parentheses
  • Primary option: true
  • Secondary options: none
  • Message: "Unexpected unbalanced parenthesis"
  • Section: "Possible errors" -> "General / Sheet"

@jeddy3 jeddy3 changed the title Add no-extra-parenthesis Add no-unbalanced-parentheses Jul 19, 2018
@davidmaxwaterman
Copy link

davidmaxwaterman commented Mar 25, 2021

Would this fix the following synthetic case, which just hit me in real code, where I copy/pasted part of a $0.querySelector(....) call I did in chrome devtools as a test, but copied a ')' by mistake and didn't notice?

$ echo ' ) {
  background: red;
}
' | npx stylelint
$

Stylelint didn't flag an error, and the rule didn't apply - took me a while to see that it was a typo.

@jeddy3
Copy link
Member

jeddy3 commented Mar 26, 2021

Would this fix the following synthetic case

PostCSS parses this as a selector of ).

We can expand the scope of the rule to include selectors, so the complete scope becomes:

  • selectors of rules
  • params of at-rules
  • values of declarations

Please consider contributing the rule if you have time.

There are steps on how to add a new rule in the Developer guide.

Copy link
Contributor

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

@github-actions github-actions bot added status: ask to implement ask before implementing as may no longer be relevant and removed status: ready to implement is ready to be worked on by someone labels Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue is good for newcomers status: ask to implement ask before implementing as may no longer be relevant type: new rule an entirely new rule
Development

No branches or pull requests

6 participants