Skip to content

Fix rvalue error when using arrow functions in {@const} #7206

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
Maritims opened this issue Jan 30, 2022 · 8 comments · Fixed by #7222
Closed

Fix rvalue error when using arrow functions in {@const} #7206

Maritims opened this issue Jan 30, 2022 · 8 comments · Fixed by #7222

Comments

@Maritims
Copy link

Maritims commented Jan 30, 2022

Describe the bug

When creating a markup variable with {@const}, whose value uses an arrow function in some way (for example, finding an item in an array), Svelte will fail with the error:
Assigning to rvalue (Note that you need plugins to import files that are not JavaScript)

This does not happen in version 3.46.2, but does happen in version 3.46.3. Can be verified by modifying the version string in the reproduction url.

There was recently an arrow function related error reported in #7134 - it seems related.

Reproduction

https://svelte.dev/repl/2932dbf792f54f9ab5c7f8365e640905?version=3.46.3

Logs

Assigning to rvalue (Note that you need plugins to import files that are not JavaScript)
     at error (/usr/src/app/node_modules/rollup/dist/shared/rollup.js:160:30)
     at Module.error (/usr/src/app/node_modules/rollup/dist/shared/rollup.js:12427:16)
     at Module.tryParse (/usr/src/app/node_modules/rollup/dist/shared/rollup.js:12815:25)
     at Module.setSource (/usr/src/app/node_modules/rollup/dist/shared/rollup.js:12718:24)
     at ModuleLoader.addModuleSource (/usr/src/app/node_modules/rollup/dist/shared/rollup.js:22215:20)

System Info

Docker image: node-latest - https://github.com/nodejs/docker-node/blob/5cafbd5b0462317bd024bb281af49585013473cd/17/bullseye/Dockerfile
Svelte 3.46.3
Browser: 97.0.4692.71

Severity

blocking an upgrade

@gtm-nayan
Copy link
Contributor

The compiler seems to be getting confused by the overlapping scope of the variables with the same name.

This works fine
https://svelte.dev/repl/716ed2c27e24478aa82b2cd098265c35?version=3.46.3

cc @baseballyama might know more about the changes needed to fix it.

@Maritims
Copy link
Author

The compiler seems to be getting confused by the overlapping scope of the variables with the same name.

This works fine https://svelte.dev/repl/716ed2c27e24478aa82b2cd098265c35?version=3.46.3

cc @baseballyama might know more about the changes needed to fix it.

Thanks @gtm-nayan! I'll use that as a workaround in the meantime, but I hope we can get this sorted out.

@baseballyama
Copy link
Member

For now, the compiler gets confused if you declare a variable in {@const} that same as <script> section.

If the same variable as in the <script> section is declared in the {@const} section, change it to use the variable declared in the {@const} section preferentially.

I will fix it over the next weekend, but until then I will consider for more missing considerations.

(Of course, others are welcome to send PRs more quickly!)

@Maritims
Copy link
Author

@baseballyama I may have misunderstood you, but in this case it's an object (foo) which has a property (nums) which happens to have the same name as a variable defined in the <script> section (nums). It's not a case of the same variable being defined in multiple places.

@baseballyama
Copy link
Member

You are right.
According to compiled bundle.js, the compiler is confused regarding both foo and nums because these are declared in the <script> section also.

{@const bar = foo.flatMap(foo => foo.nums)}
                           ^ this foo

@Maritims
Copy link
Author

You are right. According to compiled bundle.js, the compiler is confused regarding both foo and nums because these are declared in the <script> section also.

{@const bar = foo.flatMap(foo => foo.nums)}
                           ^ this foo

@baseballyama Oh dear, that's most definitely a typo from me. I updated the REPL now: https://svelte.dev/repl/2932dbf792f54f9ab5c7f8365e640905?version=3.46.3

<script>
	let nums = [1,2,3,4,5,6,7,8,9]
	let foos = [{
		nums: [1,2,3]
	}, {
		nums: [2,3,4]
	}, {
		nums: [3,4,5]
	}];
</script>

{#each nums as num}
	{@const bar = foos.flatMap(foo => foo.nums)}
	{bar}<br />
{/each}

The error then becomes Cannot read properties of undefined (reading '0') in 3.46.3, but works as one would expect in 3.46.2.

@gtm-nayan
Copy link
Contributor

gtm-nayan commented Jan 31, 2022

When trying my hands at #7134 I was considering whether it'd be possible to solve it without getting rid of hoisting.

The code generated by the #7134 repl is like this (for v3.46.2)
image

Would moving the declaration of func into get_each_context when it has contextual dependencies fix both these issues?

@baseballyama
Copy link
Member

@gtm-nayan

Thanks for the suggestion!

I'm not sure about it until I actually modify the code, but personally I prefer to always declare func in get_each_context because I want to avoid conditional branching as much as possible.

However, if that is not possible for some reason, then I guess we will implement it as your suggestion.

I think the previous fix contains a flaw for AST parsing process, and I think we can fix it by improving that process.
(But still I didn't deep dive into this issue, so afirstly I need to chek it well.)

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