thheller / shadow-cljs

ClojureScript compilation made easy
https://github.com/thheller/shadow-cljs
Eclipse Public License 1.0
2.23k stars 173 forks source link

Watch mode: which file triggered a build affects build outcome #1149

Closed simonacca closed 9 months ago

simonacca commented 9 months ago

Hello there, I've encountered a bug where the exact same codebase is sometimes built successfully and sometimes not. Specifically this happens in watch mode; the build initially succeeds, then when I touch a particular file, the build fails, and when I touch another one, the build succeeds again.

As the code that leads to this is very specific, I've produced a repo containing the exact conditions that make this happen.

To the best of my knowledge the code in question is valid and should indeed compile.

Thank you for looking into this! Simon

simonacca commented 9 months ago

Btw this also happens when the namespaces are not single-segment

thheller commented 9 months ago

I can reproduce it. No immediate idea what might be the cause though. Will look into it.

simonacca commented 9 months ago

Looking at the build output, the only difference (other than var numbering) is the following:

- (touch core)    SHADOW_ENV.evalLoad("core.js", true , "goog.provide(\x27core\x27);\nmodule3.f2().module0$Proto$method_1$arity$1(null);\n");
+ (touch module0) SHADOW_ENV.evalLoad("core.js", true , "goog.provide(\x27core\x27);\nmodule0.method_1(module3.f2());\n");

Even more curious, the output produced with touch module0.cljs seems to be the correct one :thinking:

thheller commented 9 months ago

The first one is the more optimized one. It knows that the record implements the protocol, so it is just called directly. What I don't know is why it warns, as the record is not redefined, only the protocol is. I'm not sure where the compiler stores that info though, maybe hot-reload just deletes it. Will look into it later.

thheller commented 9 months ago

Warning should be gone in 2.25.5.

Here is a recap of what is happening. On regular compilation the namespace ordering ensures that module2 is compiled before core. core however has no direct declared dependency on module2. So, when you touch module0 it and all its direct dependents are "reset" and recompiled. That would be core and module2. Since they are not dependent on each other they get compiled at the same time due to parallel compilation. Thus it can get to the call in core before the record is known.

This requires quite a bit of special circumstances to even get to this warning in the first place. The warning however is not needed, as it can just back off the specialized protocol invocation and use the dispatch function instead. It'll just do that now, instead of warning. The generated code is functionally correct either way.

An alternate fix would have been adding module2 to the dependencies of core before module0, but that seemed like it should be unnecessary as it would have been only necessary for this single special case.

simonacca commented 9 months ago

Amazing 🤩 Thank you for the fix and for the writeup too, I appreciate learning about how shadow-cljs does its magic!