ClojureScript

runtime namespace load order is independent from ordering in ns macro :require form

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: 1.9.908, 1.10.238
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code

Description

The order that namespaces are loaded at runtime is independent of the order that the namespaces are {{require}}d in user code. This seems to affect all optimization levels. This means that developers can't rely on side-effectful code in a given namespace to be run before another namespace that depends on the side effect is loaded.

Reproduction steps:

Running the command

clj --main cljs.main --compile-opts '{:target :nodejs :main a}' --compile a

will compile the a namespace defined in a.cljs. That namespace requires—via the :require form of the ns macro—namespaces b through g in alphabetical order.

After compiling, observe the following:

  • The goog.addDependency call for a.js in the output out/cljs_deps.js file includes its dependencies in an arbitrary order. For example, one run on my machine resulted in:
goog.addDependency("../a.js", ['a'], ['cljs.core', 'e', 'c', 'g', 'b', 'd', 'f']);
  • The console output given by running the compiled code with node out/main.js indicates that the runtime load order is identical to the ordering reflected in out/cljs_deps.js
body of e

body of c

body of g

body of b 

body of d

body of f

Analysis:

It looks like some work to ensure the ordering at compile time was done as part of CLJS-1453(https://dev.clojure.org/jira/browse/CLJS-1453), but it seems that things can get out of order by the time cljs_deps.js is emitted.

It seems that the output in cljs_deps.js for a given ClojureScript file is ultimately determined by the output of cljs.compiler/emit-source. The code there removes duplicate deps in the same way that the 'ns parse method in cljs.analyzer did prior to CLJS-1453.

The attached patch resolves the issue for my narrow use case, but I believe the underlying functions are passing the `:uses` and `:requires` around as maps, so more work may be needed for a full solution.

  1. a.cljs
    24/Feb/19 10:49 PM
    0.1 kB
    Chance Russell
  2. b.cljs
    24/Feb/19 10:49 PM
    0.0 kB
    Chance Russell
  3. c.cljs
    24/Feb/19 10:49 PM
    0.0 kB
    Chance Russell
  4. CLJS-3056.patch
    24/Mar/19 11:58 AM
    3 kB
    Chance Russell
  5. d.cljs
    24/Feb/19 10:49 PM
    0.0 kB
    Chance Russell
  6. deps.edn
    24/Feb/19 10:49 PM
    0.1 kB
    Chance Russell
  7. e.cljs
    24/Feb/19 10:49 PM
    0.0 kB
    Chance Russell
  8. f.cljs
    24/Feb/19 10:49 PM
    0.0 kB
    Chance Russell
  9. g.cljs
    24/Feb/19 10:49 PM
    0.0 kB
    Chance Russell

Activity

Hide
Chance Russell added a comment -

Apologies for cluttering up the ticket with markdown—thought I would be able to clean it up after submission.

Show
Chance Russell added a comment - Apologies for cluttering up the ticket with markdown—thought I would be able to clean it up after submission.
Hide
Mike Fikes added a comment -

CLJS-3056.patch passes CI and Canary

Show
Mike Fikes added a comment - CLJS-3056.patch passes CI and Canary
Hide
Thomas Heller added a comment -

The attached patch still uses the deps map which will only maintain insertion order until the array-map threshold is hit and once it starts using a hash-map things will be not be ordered again. In shadow-cljs I handle this by maintaining an addition :deps vector in addition to the usual :requires map.

Show
Thomas Heller added a comment - The attached patch still uses the deps map which will only maintain insertion order until the array-map threshold is hit and once it starts using a hash-map things will be not be ordered again. In shadow-cljs I handle this by maintaining an addition :deps vector in addition to the usual :requires map.
Hide
Chance Russell added a comment -

That’s probably the best solution—I didn’t want to attempt to modify the type of the maps in the existing code.

I’ll take a look at this this evening—it seems like the patch for CLJS-1453 may have the same problem.

That said, I wonder what the semantics for ordering should be when an ns form contains both :require and :use?

Show
Chance Russell added a comment - That’s probably the best solution—I didn’t want to attempt to modify the type of the maps in the existing code. I’ll take a look at this this evening—it seems like the patch for CLJS-1453 may have the same problem. That said, I wonder what the semantics for ordering should be when an ns form contains both :require and :use?
Hide
Thomas Heller added a comment -

Ah looks like a :deps vector is already maintained in the analyzer parse 'ns and 'ns*. Just need to use that in the cljs.compiler/emit-source fn instead taking the vals of the requires/uses maps.

Show
Thomas Heller added a comment - Ah looks like a :deps vector is already maintained in the analyzer parse 'ns and 'ns*. Just need to use that in the cljs.compiler/emit-source fn instead taking the vals of the requires/uses maps.
Hide
Chance Russell added a comment -

Thomas was right, the :deps vector is present and in the expected order, so I've submitted a new patch that uses that.

One question—what should the ordering of cljs.core and cljs.core.constants be when both are present? Obviously the ordering wasn't guaranteed with the previous code, but I'd like to get that right.

(The test in test-cljs-1882-constants-table-is-sorted would seem to imply that cljs.core should go first.)

Show
Chance Russell added a comment - Thomas was right, the :deps vector is present and in the expected order, so I've submitted a new patch that uses that. One question—what should the ordering of cljs.core and cljs.core.constants be when both are present? Obviously the ordering wasn't guaranteed with the previous code, but I'd like to get that right. (The test in test-cljs-1882-constants-table-is-sorted would seem to imply that cljs.core should go first.)
Hide
Mike Fikes added a comment -

CLJS-3056.patch of 24/Mar/19 12:58 PM passes CI and Canary

Show
Mike Fikes added a comment - CLJS-3056.patch of 24/Mar/19 12:58 PM passes CI and Canary
Hide
David Nolen added a comment - - edited

cljs.core must come before cljs.core.constants as `cljs.core.contants` won't work without the keyword construct being defined. The ordering was most definitely guaranteed otherwise we would have heard a lot of complaints about failing advanced builds.

Show
David Nolen added a comment - - edited cljs.core must come before cljs.core.constants as `cljs.core.contants` won't work without the keyword construct being defined. The ordering was most definitely guaranteed otherwise we would have heard a lot of complaints about failing advanced builds.
Hide
Chance Russell added a comment -

David, when you say "ordering was most definitely guaranteed", are you referring to just cljs.core and cljs.core.constants, or the dependencies as a whole? The current code is {{conj}}ing onto sets, which isn't guaranteed to maintain any ordering AFAIK.

Show
Chance Russell added a comment - David, when you say "ordering was most definitely guaranteed", are you referring to just cljs.core and cljs.core.constants, or the dependencies as a whole? The current code is {{conj}}ing onto sets, which isn't guaranteed to maintain any ordering AFAIK.

People

Vote (1)
Watch (3)

Dates

  • Created:
    Updated: