Using expression containing Math.random() more than once wrongly assumes all instances should have same result

[parallaxshiftdj]

Describe the bug

An expression is used here twice, but because it contains Math.random() it shouldn't be assumed that the same result will occur every time it is used.

Reproduction

v5.19.0 (Desired outcome)
https://svelte.dev/playground/hello-world?version=5.19.0#H4sIAAAAAAAACo3PvW7DIBQF4FdBVFFwFNnJ0IW4kbJUXfoEdQdirgMqBgQ3PxbyuxeiVF06dP107jmQqBUjUE7fwBhHri4YSRhIjSAruqaDNhAp_0gUJ19yBbI_rg7e1_ECBosdRYS_vHcWwWKuoekJRK_IIQQxse1mU82d7bCV-kIiTgZelug8T-xdoKqDsNKNrCIrUqI1uld9A8m21bzYGRjwP8Hlvm1y_b6zqSnbZdC297FsZTiVJ3gXNWpnORnK6a7YUfRfp-DOVnISHnbVEhUn9XOA8Q4K9Enhr-T-tvmpz59HuCHlGM4wf87fUt4xEm4BAAA=

v5.19.1+ (Undesired outcome)
https://svelte.dev/playground/hello-world?version=5.19.1#H4sIAAAAAAAACo3PvW7DIBQF4FdBVFFwFNnJ0IW4kbJUXfoEdQdirgMqBgQ3PxbyuxeiVF06dP107jmQqBUjUE7fwBhHri4YSRhIjSAruqaDNhAp_0gUJ19yBbI_rg7e1_ECBosdRYS_vHcWwWKuoekJRK_IIQQxse1mU82d7bCV-kIiTgZelug8T-xdoKqDsNKNrCIrUqI1uld9A8m21bzYGRjwP8Hlvm1y_b6zqSnbZdC297FsZTiVJ3gXNWpnORnK6a7YUfRfp-DOVnISHnbVEhUn9XOA8Q4K9Enhr-T-tvmpz59HuCHlGM4wf87fUt4xEm4BAAA=

Logs

System Info

System:
    OS: Windows 10 10.0.19045
    CPU: (4) x64 Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz
    Memory: 1.40 GB / 7.73 GB
  Binaries:
    Node: 20.11.1 - C:\Program Files\nodejs\node.EXE     
    pnpm: 8.9.0 - ~\AppData\Local\pnpm\pnpm.EXE
  Browsers:
    Edge: Chromium (127.0.2651.74)

Severity

annoyance

[trueadm]

@Rich-Harris Looks like a recent regression relating to the work you did on the compilation of the template?

[Rich-Harris]

Ha, yep. Not totally sure what a good fix would be other than just reverting the duplicate expression detection, which would be a shame... either way a workaround in the meantime is to make a meaningless change to one of the expressions:

-{(Math.random(i) * 100).toFixed(1)}
+{(0, Math.random(i) * 100).toFixed(1)}

[Conduitry]

I'm torn about this. What sorts of practical situations - besides ones that eventually call Math.random() - do we think might also result in this issue? People already shouldn't be calling side-effect-y things in their template - which Math.random() definitely is, because it changes the internal state of the RNG and you get a different number the next time. If we say "no, you need to always be calling pure functions from the template, and if you need to work around that, here is how you trick the duplicate expression detection", what else are we closing ourselves off from supporting? People have long asked for something like a stable ID generator - but that's also going to be stateful.

If we can get away with it, I'd love to push people towards "all template expressions need to be pure, otherwise undefined behavior will happen" and "if you need to do something stateful, do it in your <script> block where you can control exactly when it happens" - but I don't know whether that's too hard-line of a position to be taking.

[trueadm]

@Conduitry We could probably do this and document it for Svelte 6, but this might very well be a breaking change now. Whilst people shouldn't be doing side-effects in the template, there's no documentation saying this other than the error they'll get from mutating state, which only happens in runes mode.

[webJose]

Maybe I'm rusty on the terms, but I think the correct terminology is "math.random() is a pure function, but non-deterministic". It is pure because it doesn't have side effects (unsure about which side effects Conduitry is referring to), but its return value is different on every call.

UPDATE: Ah, ok. The internal state that gets seeded on every call. Gotcha.

If my memory is serving me well here regarding terms, the issue is not about side effects, but about determinism, and sure, math.random() is probably not something that is likely seen in production, but there are other non-deterministic expressions, such as Date.now.

[trueadm]

Maybe I'm rusty on the terms, but I think the correct terminology is "math.random() is a pure function, but non-deterministic". It is pure because it doesn't have side effects (unsure about which side effects Conduitry is referring to), but its return value is different on every call.

Math.random() is definitely not a pure function, just like Date.now() is not. The definition of a pure function is given it's inputs, it should always return the same outputs. In fact, a non-deterministic function is always defined as non-pure in functional computing – such as reading a file on the system.

[webJose]

Ah, it seems that the term "pure function" also refers to a "deterministic function", but not the other way around. 👍

[webJose]

For the issue at hand, however, the problem seems that the new algorithm assumes that all expressions are deterministic. Internal state changes don't really bother the algorithm in its current form, as I understand the matter.

Since it is probably impossible to define and exclude every non-deterministic expression out there, I think the change will probably have to be rolled back. The workaround provided by Rich is solid, but it's not very maintainable.

[webJose]

How about adding a per-component option to tell the compiler to disable Rich's new feature?

[Rich-Harris]

Absolutely not

[adiguba]

Hello,

I found a strange behavior :

• {(Math.random()*10).toFixed(20)} is detected as duplicate expression.
• But not {(Math.random()).toFixed(20)} or {Math.random()}, which produce different values.

Demo : https://svelte.dev/playground/hello-world?version=5.19.4#H4sIAAAAAAAACpWQTQuCQBCG_4rMyY1Q67hIEER06dQxO2ztSAvrruhohvjfW7OLIX1c369nmBaMyBA47FBr691soaXno1SEksEcUqWxBH5sge55n-sFp79a6zwPyho19dpZlDilX6whNORm4OBsrxa6Qo8nJq70KjEJxVqtWn8v6BoUwkib-Wy2iFhAdqsalP4yYl0cutCf4TgcAGaj0hQLd8PALr_Afyd_xo4Qo97b6KT3nHHvI2wIOBUVdqfuATG2_g6wAQAA

Otherwise, although pure functions are preferable, It might be useful to be able to disable this detection in very specific cases like this one.

A special template syntax seems appropriate to me.
Something like {@unpure expression} or {@unsafe expression}