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

[AST] Incorrect parsing of comments inside the callback scope #13040

Open
xeho91 opened this issue Aug 27, 2024 · 2 comments
Open

[AST] Incorrect parsing of comments inside the callback scope #13040

xeho91 opened this issue Aug 27, 2024 · 2 comments

Comments

@xeho91
Copy link
Contributor

xeho91 commented Aug 27, 2024

Describe the bug

Sorry in advance, but I might be bringing a PITA with comments, again.

I noticed while working on updating svelte-ast-print, I've had inline snapshots mismatch (thanks Vitest!).
There was a bug which I've overlooked before and noticed that something was off. I was able to narrow that there's a parsing difference between version svelte@5.0.0-next.190 and svelte@5.0.0-next.191. Regardless, both versions parse it wrongly, including the latest one.

Possibly related PR: #12471

Piece of input inside the <script> tag that gets parsed wrong:

beforeUpdate(() => {
  // determine whether we should auto-scroll
  // once the DOM is updated...
});

afterUpdate(() => {
  // ...the DOM is now in sync with the data
});

svelte@5.0.0-next.190

svelte@5.0.0-next.191

-       beforeUpdate(() => {
-               // determine whether we should auto-scroll
+       beforeUpdate(() => {}); // determine whether we should auto-scroll
-               // once the DOM is updated...
+       // once the DOM is updated...
-       });
-
-       afterUpdate(() => {
-               // ...the DOM is now in sync with the data
-       });
+       afterUpdate(() => {}); // ...the DOM is now in sync with the data
-       beforeUpdate(() => {
+       beforeUpdate(() => {});
-               // determine whether we should auto-scroll
+       // determine whether we should auto-scroll
-               // once the DOM is updated...
+       // once the DOM is updated...
-       });
-
-       afterUpdate(() => {
+       afterUpdate(() => {});
+
-               // ...the DOM is now in sync with the data
+       // ...the DOM is now in sync with the data
-       });
-

Below I will provide a simple reproduction where you can see that the comment inside the callback scope of beforeUpdate() gets wrongly parsed as trailingComments.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE02OywrCMBBFf2WYTS0Uum-14D_oyriIyZQG8yIzFaT03yUI4vIezoG74ew8MQ63DaMOhAOec8YO5Z3r4Bd5IeyQ01pMJUc2xWWZVFTiQk5FYIMHzanQNVstBDvMJQVovm0zqljdf-VwaOE0wVa5kr6Hy-IYTAqBogAvafUWWPQbXGRnCYz2_qHNE9ikTDXb21HFY_87gx2GZN3syOIgZaX9vn8AcUwca90AAAA=

Logs

No response

System Info

next

Severity

annoyance

@dummdidumm
Copy link
Member

Ah yeah, comments ... sigh. The referenced PR improved the situations, but as can be seen here it still has problems with comments inside empty blocks. The problem is that - to my knowledge - there's no real standard on where to put comments on the AST, and for cases like this the semi-standard of leading/trailingComments doesn't work, because this is neither. Eslint uses innerComments for this.
To fix this I think we need to decide on a structure we want to go with and then implement that in esrap accordingly.

@xeho91
Copy link
Contributor Author

xeho91 commented Aug 28, 2024

Aye, so this is very edge case 😅 (I feel sorry for bringing this up).

It can affect 'codemod' ecosystem scope. So, in case someone else is playing around with Svelte AST and will notice this issue... the possible and temporary workaround is very trivial. Don't write lone comments. The ones which cannot be assigned as leadingComment or trailingComment.

Example:

beforeUpdate(() => {
	// This comment should stay inside callback scope 
        console.log(); // 👈 Add any statement, so 👆 comment becomes a `trailingComment`
});

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

No branches or pull requests

2 participants