Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

🐛 Formatter introduces infinite parentheses around certain logic expressions over time #3735

Closed
1 task done
lgarron opened this issue Nov 15, 2022 · 1 comment · Fixed by #3736
Closed
1 task done
Assignees
Labels
A-Formatter Area: formatter L-JavaScript Langauge: JavaScript S-Bug: confirmed Status: report has been confirmed as a valid bug

Comments

@lgarron
Copy link

lgarron commented Nov 15, 2022

Environment information

Rome CLI version 10.0.1

What happened?

Consider the following code, meant to work around #3734 :

export function supported(): boolean {
  return !!(
    // rome-ignore format: Work around https://github.com/rome/tools/issues/3734
    // rome-ignore lint(style/useOptionalChain): Optional chaining creates more complicated ES2019 code
    navigator.credentials &&
    navigator.credentials.create &&
    navigator.credentials.get &&
    window.PublicKeyCredential
  );
}

This successfully prevents the formatter from breaking the // rome-ignore, but it seems Rome's formatter is not idempotent — it adds new parentheses on every formatting invocation. After a few runs of npx rome format you get:

export function supported(): boolean {
  return !!(
    (
    (
    (
    (
    (
    (
    (
    (
    (
    (
    (
    (
    (
    (
    (
    (
    (
    (
    // rome-ignore format: Work around https://github.com/rome/tools/issues/3734
    // rome-ignore lint(style/useOptionalChain): Optional chaining creates more complicated ES2019 code
    navigator.credentials &&
    navigator.credentials.create &&
    navigator.credentials.get &&
    window.PublicKeyCredential
  )
  )
  )
  )
  )
  )
  )
  )
  )
  )
  )
  )
  )
  )
  )
  )
  )
  )
  );
}

This is kind of hilarious when formatting on save, but I presume it's not intended behaviour.

Expected result

Rome's formatter is idempotent — in this case, if it adds parentheses then subsequent invocations keep the exact same parentheses without adding more.

(The parentheses are redundant in this particular case, though.)

Code of Conduct

  • I agree to follow Rome's Code of Conduct
@lgarron lgarron added the S-To triage Status: user report of a possible bug that needs to be triaged label Nov 15, 2022
@lgarron
Copy link
Author

lgarron commented Nov 15, 2022

Note that if you modify the sample code by moving the // rome-ignore format comment one line up, this issue doesn't occur. So I can easily work around it. But I thought I'd file a bug to save others the headache, if this can be fixed.

lgarron added a commit to github/webauthn-json that referenced this issue Nov 15, 2022
lgarron added a commit to github/webauthn-json that referenced this issue Nov 15, 2022
lgarron added a commit to github/webauthn-json that referenced this issue Nov 15, 2022
@MichaReiser MichaReiser added S-Bug: confirmed Status: report has been confirmed as a valid bug A-Formatter Area: formatter L-JavaScript Langauge: JavaScript and removed S-To triage Status: user report of a possible bug that needs to be triaged labels Nov 15, 2022
@MichaReiser MichaReiser self-assigned this Nov 15, 2022
MichaReiser added a commit that referenced this issue Nov 15, 2022
… verbatim argument

## Summary
This PR fixes #3735 by only adding parentheses around the argument if the argument isn't suppressed. This is necessary because the verbatim formatting of the argument includes the parentheses too. Formatting the parentheses in the return/unary formatting and as part of the verbatim results in duplicating the parentheses on every save.

## Test Plan
I added a handful of new tests covering the problematic cases.
MichaReiser added a commit that referenced this issue Nov 15, 2022
… verbatim argument

## Summary
This PR fixes #3735 by only adding parentheses around the argument if the argument isn't suppressed. This is necessary because the verbatim formatting of the argument includes the parentheses too. Formatting the parentheses in the return/unary formatting and as part of the verbatim results in duplicating the parentheses on every save.

## Test Plan
I added a handful of new tests covering the problematic cases.
@ematipico ematipico added this to the 11.0.0 milestone Nov 15, 2022
@MichaReiser MichaReiser moved this to Done in Rome 2022 Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter L-JavaScript Langauge: JavaScript S-Bug: confirmed Status: report has been confirmed as a valid bug
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants