[Bug] 2.0.1 negative range error

NullVoxPopuli commented 3 months ago

🐞 Describe the Bug

Run on this file:

import { ExternalLink, service } from 'ember-primitives';
import { colorScheme } from 'ember-primitives/color-scheme';

import { Footer } from './footer';
import { Nav } from './nav';
import { Prose } from './prose';

function removeAppShell() {
  document.querySelector('#initial-loader')?.remove();
}

function isDark() {
  return colorScheme.current === 'dark';
}

const ReportingAnIssue = <template>
  <ExternalLink href="https://github.com/universal-ember/ember-primitives/issues/new">
    reporting an issue
  </ExternalLink>
</template>;

<template>
  {{#let (service "selected") as |page|}}
    {{#if page.prose}}

      {{(removeAppShell)}}

      {{#if (isDark)}}
        <link
          rel="stylesheet"
          href="https://cdn.jsdelivr.net/npm/highlight.js@11.8.0/styles/atom-one-dark.css"
        />
      {{else}}
        <link
          rel="stylesheet"
          href="https://cdn.jsdelivr.net/npm/highlight.js@11.8.0/styles/atom-one-light.css"
        />
      {{/if}}

      <main id="layout">
        <Nav />
        <section>
          <Prose />
        </section>
      </main>
      <Footer />
    {{else if page.hasError}}
      <h1>Oops!</h1>
      {{page.error}}

      <br />
      <br />
      If you have a GitHub account (and the time),
      <ReportingAnIssue />
      would be most helpful! 🎉
    {{/if}}
  {{/let}}
</template>

😕 Actual Behavior

docs-app:lint:prettier:fix: app/components/layout.gts
docs-app:lint:prettier:fix: [error] app/components/layout.gts: RangeError: Invalid count value: -11
docs-app:lint:prettier:fix: [error]     at String.repeat (<anonymous>)
docs-app:lint:prettier:fix: [error]     at preprocessTemplateRange (file://<.pnpm>/prettier-plugin-ember-template-tag@2.0.1_prettier@3.2.5/node_modules/prettier-plugin-ember-template-tag/dist/prettier-plugin-ember-template-tag.js:70008:40)
docs-app:lint:prettier:fix: [error]     at preprocess (file://<.pnpm>/prettier-plugin-ember-template-tag@2.0.1_prettier@3.2.5/node_modules/prettier-plugin-ember-template-tag/dist/prettier-plugin-ember-template-tag.js:70071:12)
docs-app:lint:prettier:fix: [error]     at Object.parse (file://<.pnpm>/prettier-plugin-ember-template-tag@2.0.1_prettier@3.2.5/node_modules/prettier-plugin-ember-template-tag/dist/prettier-plugin-ember-template-tag.js:70079:26)
docs-app:lint:prettier:fix: [error]     at parse4 (file://<.pnpm>/prettier@3.2.5/node_modules/prettier/index.mjs:22117:24)
docs-app:lint:prettier:fix: [error]     at async coreFormat (file://<.pnpm>/prettier@3.2.5/node_modules/prettier/index.mjs:22607:7)
docs-app:lint:prettier:fix: [error]     at async formatWithCursor (file://<.pnpm>/prettier@3.2.5/node_modules/prettier/index.mjs:22809:14)
docs-app:lint:prettier:fix: [error]     at async formatFiles (file://<.pnpm>/prettier@3.2.5/node_modules/prettier/internal/cli.mjs:6673:18)
docs-app:lint:prettier:fix: [error]     at async main (file://<.pnpm>/prettier@3.2.5/node_modules/prettier/internal/cli.mjs:7081:5)
docs-app:lint:prettier:fix: [error]     at async Module.run (file://<.pnpm>/prettier@3.2.5/node_modules/prettier/internal/cli.mjs:7027:5)
docs-app:lint:prettier:fix: app/components/nav.gts

🤔 Expected Behavior

no error

🌍 Environment

prettier version: 3.2.5
prettier-plugin-ember-template-tag version: - 2.0.1
ember-template-imports version (if applicable): - 3.4.2
content-tag version (if applicable): - nay
eslint-plugin-ember version (if applicable): - ^12

➕ Additional Context

Add any other context about the problem here.

gitKrystan commented 3 months ago

This will be the hack / line that triggers it: https://github.com/gitKrystan/prettier-plugin-ember-template-tag/blob/main/src/parse/preprocess.ts#L83

MichalBryxi commented 3 months ago

Just got bitten by this. I cut down Preston's example to something that is really small (for testing), can't be (according to my experiements) really trimmed down and still reproduces given issue:

<template>
                Testing line, incorrectly indented.
  {{#if true}}
    {{#if true}}
      <link href="/////styles/" />
    {{else}}
      <link href="/////styles/" />
    {{/if}}
  {{/if}}
</template>

This throws an error:

["ERROR" - 17:59:31] Error formatting document.
["ERROR" - 17:59:31] Invalid count value: -1
RangeError: Invalid count value: -1
    at String.repeat (<anonymous>)

MichalBryxi commented 3 months ago

I tried to see if there is a set of unit tests that I could use to isolate the reason for this issue, but did not find one. I'm happy to write test coverage for these from scratch, but I can't figure out how to manually construct the arguments for the preprocessTemplateRange function? IDK how can one build template: Template,?

MichalBryxi commented 3 months ago

Ah found it. I can export and then use:

import { preprocess } from '../../src/parse/index.js';

cah-brian-gantzler commented 3 months ago

Other Devs at my company were reporting this, it was working fine for me. After rm -rf node_modules and then npm i I now get the issue if that adds any context.

MichalBryxi commented 3 months ago

The issue will be probably around this line where we add escaping for forward slashes. That explains why my test case above needed that incredible amount of forward slashes to fail properly (<link href="/////styles/" />).

I assume we want those \\/ because single forward slash would do something wrong in the output format?
If that is so, I'd guess we should count the number of those occurences and add them to the spaces calculation?

MichalBryxi commented 3 months ago

Ok I think I got it working for my isolated set of unit tests: https://github.com/gitKrystan/prettier-plugin-ember-template-tag/pull/259
Now question also is why do we want to add the extra spaces? Because following: <template>hi</template> will be translated as: {/*hi */}. I'd assume it is to preserve the overall length of the file and not shuffle things around? Sadly if I'm right with the / => \\/ translation, this won't be reliably possible as the resulting code can be longer than the original, so it might move things around.
My PR will probably fail on pretty much every other test as the snapshots won't match now.
I'd appreciate someone's 👀 to tell me what should be the correct direction?

cah-brian-gantzler commented 3 months ago

Thanks for your efforts. Your fix seems to indicate the issue was in the / in the links. The code that is throwing this error for me has no links. But does have nested {{#if}} and an {{unless}}, is it possible the / in the closing {{/if}} as also causing the issue?

cah-brian-gantzler commented 3 months ago

Removing ifs one at a time what seems to trigger it for me is (in my real code, haven't created a test app)

{{#if}}
    {{#if}}
    {{/if}}
    {{#if}}
    {{/if}}
{{/if}}

Removing one of the two nested nested ifs seems to get by

MichalBryxi commented 3 months ago

Ah yes, any forward slash will count. So even those in closing ifs.

MichalBryxi commented 3 months ago

Therefore the minimal reproduction is: <template>////////////////</template> :)

MichalBryxi commented 3 months ago

The ' '.repeat(spaces) there seems to be there to make sure that:

"<template>${foo1}</template>".length === "{/*${foo2}${magic_amount_of_spaces}*/}".length

But unfortunatelly due to different "languages" on the left and right side it's given that $foo1 != $foo2.
And due to the $foo1 expansion of / into \\/ in $foo2 it can happen that $foo1.length < $foo2.length.
Therefore I think that trying to construct the $magic_amount_of_spaces to make the first code snippet work is not going to work.

I'm going to try to alter my PR to remove this functionality altogether. And try to run the plugin locally to see what will happen. Hope it's not a show-stopper.

MichalBryxi commented 3 months ago

Ok there seems to be a pickle.

I can't remove the / => \\/ functionality, because following template:

<template>/* comment */</template>

will cause following error: SyntaxError: Unterminated regular expression..

Also if I try to remove the ' '.repeat(spaces) then the convertAst function will explode as things won't be where they are expected to be. So not sure if there is.a good, simple solution?

MichalBryxi commented 3 months ago

I did a quick experiement to see what will happen if instead of / => \\/ we just replace the forward slash with random non-offending character. i.e: / => Q and so far my test environment does not seem to crash and also formats correctly all the edge cases I put into tests. If we just want to "hold" the space of the template contents, then we could just fill whole with spaces and call it a day? I mean there might be other traps somewhere down the road, so why risk it?

MichalBryxi commented 3 months ago

PR https://github.com/gitKrystan/prettier-plugin-ember-template-tag/pull/259 IMO fixes this issue. There is only one drawback in replacing / with arbitrary non-slash-forward-character. But to my knowledge this causes least amount of code churn (and possible bugs). Not the prettiest, but works.

gitKrystan commented 2 months ago

The fix for this now lives in https://github.com/gitKrystan/prettier-plugin-ember-template-tag/releases/tag/v2.0.2. Thanks again to @MichalBryxi for the PR! ❤️

lionelB commented 2 months ago

thanks !

gitKrystan / prettier-plugin-ember-template-tag