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

Svelte compiler removes semicolon after while, changing logic #6884

Closed
mustafa0x opened this issue Oct 28, 2021 · 9 comments · Fixed by Rich-Harris/code-red#66
Closed

Svelte compiler removes semicolon after while, changing logic #6884

mustafa0x opened this issue Oct 28, 2021 · 9 comments · Fixed by Rich-Harris/code-red#66
Labels
bug compiler Changes relating to the compiler

Comments

@mustafa0x
Copy link
Contributor

mustafa0x commented Oct 28, 2021

function fn() {
    let i = 1;
    while (++i < 100);
    return i;
}

Compiles to:

function fn() {
    let i = 1;
    while (++i < 100) // No semicolon!
    return i;
}

causing fn() to give 2 instead of 100.

@rmunn
Copy link
Contributor

rmunn commented Oct 28, 2021

That's how Javascript works. The while statement expects either a block between braces like while (condition) { doSomething(); doSomethingElse(); } or else a single statement (while (condition) doSomething();). The semicolon after the while in your first example is an empty statement, and return i then runs after the while loop is finished. The while in your second example is doing the equivalent of while (++i < 100) { return i; }, and that loop will only run once because the return ends the loop.

This is not a Svelte bug; it's compiling the Javascript exactly as it should. Try running that Javasript in Node and you'll see the same result.

@mustafa0x
Copy link
Contributor Author

mustafa0x commented Oct 28, 2021

Those aren't examples; the first is before Svelte compiles, and the second is Svelte's output. I've updated the title to make it more explicit.

@mustafa0x mustafa0x changed the title Semicolon removed after while, changing logic Svelte compiler removes semicolon after while, changing logic Oct 28, 2021
@Prinzhorn
Copy link
Contributor

Can confirm, this is broken. Screenshot from REPL, the semicolon is gone:

Selection_997

And if you download it and run npm run dev it will yield 2 and not 100. I don't know why the REPL itself shows 100 though. It probably does some other magic.

@bluwy bluwy added bug compiler Changes relating to the compiler labels Oct 28, 2021
@rmunn
Copy link
Contributor

rmunn commented Oct 28, 2021

Interestingly, if you replace while (++i < 100); with while (++i < 100) { }, that compiles correctly.

I don't know why the REPL builds it differently. If someone more familiar than I am with the REPL can figure out what's different about the REPL setup vs. how the skeleton Svelte-Kit app is set up, that might lead to a clue about the cause of this bug.

@Conduitry
Copy link
Member

I would guess it is because of the loopGuardTimeout option, which the REPL sets when running user code (although not in the code that's displayed in the right panel) and this would result in the while loop now having a body.

In any case, the bug is likely in https://github.com/Rich-Harris/code-red and would need to be fixed there.

@rmunn
Copy link
Contributor

rmunn commented Oct 29, 2021

I don't think the bug is in code-red. Check https://svelte.dev/repl/fa45d9b0d17e47b8bb772633e4615ee2?version=3.44.0 where I load the code-red module and have it turn those two examples into an AST. Open up the console and dig into the "with semicolon" and "without semicolon" AST objects that were logged. The "with semicolon" one has a WhileStatement whose body is an EmptyStatement, which looks correct to me. The "without semicolon" one has a WhileStatement whose body is a ReturnStatement, which again is the correct AST for that Javascript.

As an aside, the fact that I could just write import { b, x } from 'code-red'; in the Svelte REPL and have it Just Work™ made testing this so easy. I'm very grateful for that "auto-load NPM packages from unpkg.com" feature in the REPL.

@Conduitry
Copy link
Member

It looks like the issue is in code-red's print function, not in the AST that's generated. If you run both of those ASTs through print, you get the return as part of the while statement body for both of them. The WhileStatement handler in https://github.com/Rich-Harris/code-red/blob/master/src/print/handlers.js probably need special handling for node.body is missing.

There are other handlers that are suffering from this same thing, it looks like. ForStatement also has this problem. ifs are also affected, although you'd have to be insane to write code like if (foo); bar();. At this point, it'd probably be good to take a sweep through that whole file.

@Conduitry
Copy link
Member

This should be fixed now in 3.44.1.

@mustafa0x
Copy link
Contributor Author

Thanks @tanhauhau @Conduitry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug compiler Changes relating to the compiler
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants