Skip to content

Svelte 5: Incorrect IfBlock Start Position in Modern Mode Parsing #11975

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

Closed
YusukeHirao opened this issue Jun 9, 2024 · 5 comments · Fixed by #12342
Closed

Svelte 5: Incorrect IfBlock Start Position in Modern Mode Parsing #11975

YusukeHirao opened this issue Jun 9, 2024 · 5 comments · Fixed by #12342
Milestone

Comments

@YusukeHirao
Copy link

Describe the bug

While experimenting with Svelte v5.0.0-next, I encountered a bug in the svelte/compiler parse function when parsing in modern mode. The issue arises with the IfBlock start position when there is a space between { and #if.

Code Example:

{    #if variable}
...
{/if}

Parsed Output:

{
    "type": "IfBlock",
    "elseif": false,
    "start": 4,
    "end": 26,
    ....
}

In this example, the start position of the IfBlock is incorrectly set to 4. It should be 0 (the position of {).

Expected Behavior:
The start position should be 0 if there is a space between { and #if. If a space is considered a syntax error, an exception should be thrown indicating a parse error.

Additional Context:
When there is no space between { and #if, the start position is correctly set to 0:

{#if variable}
...
{/if}

Parsed Output:

{
    "type": "IfBlock",
    "elseif": false,
    "start": 0,
    "end": 26,
    ....
}

This behavior suggests that the space handling between { and #if might not be correctly implemented.

Additional Information:
If the space between { and #if is syntactically incorrect, an exception indicating a parse error should be thrown instead of silently misplacing the start position.

Reproduction

Environment:

  • Svelte version: v5.0.0-next.152 (I initially discovered this issue in Svelte v5.0.0-next.148 and it persists in v5.0.0-next.152)
  • Node version: v22.2.0

Steps to Reproduce:

  1. Use the svelte/compiler parse function in modern mode.
  2. Parse the following code:
    {    #if variable}
    ...
    {/if}
  3. Observe the start position of the IfBlock in the parsed output.

Reproduction Code: https://jqctdp.csb.app/

Logs

No response

System Info

System:
    OS: macOS 14.3.1
    CPU: (8) arm64 Apple M1
    Memory: 92.45 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 22.2.0 - ~/.volta/tools/image/node/22.2.0/bin/node
    Yarn: 1.22.21 - ~/.volta/tools/image/yarn/1.22.21/bin/yarn
    npm: 10.7.0 - ~/.volta/tools/image/node/22.2.0/bin/npm
  Browsers:
    Chrome: 125.0.6422.142
    Chrome Canary: 127.0.6529.0
    Edge: 125.0.2535.92
    Firefox: 126.0
    Firefox Nightly: 112.0a1
    Safari: 17.3.1

Severity

annoyance

@Rich-Harris
Copy link
Member

I wonder if we should just make { #if a parsing error in v5. Is there any reason to allow it?

@YusukeHirao
Copy link
Author

Hi Rich,

Thank you for your response. I agree that making { #if a parsing error in v5 could be a viable solution. This would help maintain consistency and avoid potential parsing issues. If there's no specific reason to allow the space between { and #if, enforcing a stricter syntax would be beneficial for error prevention.

By the way, I am the developer of Markuplint, which utilizes the Svelte parser. Whether it outputs accurate location data or parsing errors, I will follow the Svelte compiler's approach.

@dummdidumm
Copy link
Member

I wonder if we should just make { #if a parsing error in v5. Is there any reason to allow it?

I remember some people writing it like that for some reason (someone opened an issue in language tools about this syntax not working a while back).
I don't see why we should continue to allow it if it makes live harder for us. Should only error in runes mode though.

@Rich-Harris
Copy link
Member

I don't see why we should continue to allow it if it makes live harder for us

I'm actually more concerned about consistency within the ecosystem

@dummdidumm dummdidumm added this to the 5.0 milestone Jun 12, 2024
@dummdidumm
Copy link
Member

Found the related issue: sveltejs/language-tools#1638 seems like a very small group of people prefer writing code like this? Anyway I'd be ok with disallowing it.

dummdidumm added a commit that referenced this issue Jul 8, 2024
disallow characters between `{` and `#` / `:` / `@` in runes mode

closes #11975
Rich-Harris pushed a commit that referenced this issue Jul 8, 2024
disallow characters between `{` and `#` / `:` / `@` in runes mode

closes #11975
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants