ClojureScript

resolve-var for symbol with dot still wrong

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.7.145
  • Fix Version/s: 1.9.671
  • Component/s: None
  • Labels:
    None

Description

We need to recur on the first segment passing an new additional argument to resolve-var indicating that we should not try to resolve in the current namespace and instead warn.

Activity

David Nolen made changes -
Field Original Value New Value
Fix Version/s Next [ 10355 ]
Affects Version/s 0.0-3115 [ 10352 ]
David Nolen made changes -
Affects Version/s Next [ 10355 ]
Affects Version/s 0.0-3115 [ 10352 ]
Fix Version/s Backlog [ 10356 ]
Fix Version/s Next [ 10355 ]
David Nolen made changes -
Fix Version/s Backlog [ 10362 ]
Fix Version/s Next [ 10356 ]
David Nolen made changes -
Affects Version/s Next [ 10362 ]
Affects Version/s 0.0-3196 [ 10355 ]
Fix Version/s Backlog [ 10363 ]
Fix Version/s Next [ 10362 ]
David Nolen made changes -
Affects Version/s Next [ 10363 ]
Affects Version/s 0.0-3255 [ 10362 ]
Fix Version/s Backlog [ 10365 ]
Fix Version/s Next [ 10363 ]
David Nolen made changes -
Fix Version/s Backlog [ 10366 ]
Fix Version/s Next [ 10365 ]
David Nolen made changes -
Affects Version/s 1.7.28 [ 10366 ]
Affects Version/s 0.0-3269 [ 10363 ]
Fix Version/s Next [ 10450 ]
Fix Version/s 1.7.28 [ 10366 ]
David Nolen made changes -
Affects Version/s Next [ 10450 ]
Affects Version/s 1.7.28 [ 10366 ]
Fix Version/s Backlog [ 10651 ]
Fix Version/s Next [ 10450 ]
David Nolen made changes -
Affects Version/s Next [ 10651 ]
Affects Version/s 1.7.48 [ 10450 ]
Fix Version/s Backlog [ 10653 ]
Fix Version/s Next [ 10651 ]
David Nolen made changes -
Fix Version/s Next [ 10751 ]
Fix Version/s 1.7.228 [ 10653 ]
David Nolen made changes -
Fix Version/s Backlog [ 10851 ]
Fix Version/s Next [ 10751 ]
David Nolen made changes -
Fix Version/s Next [ 11055 ]
Fix Version/s 1.9.293 [ 10851 ]
David Nolen made changes -
Fix Version/s 1.9.655 [ 11055 ]
Fix Version/s Backlog [ 11154 ]
David Nolen made changes -
Fix Version/s 1.9.660 [ 11154 ]
Fix Version/s Backlog [ 11359 ]
Hide
Thomas Heller added a comment -

I just stumbled over this when trying to figure out why process.env.FOO (without js/) produces no warnings and noticed that pretty much all symbols with dots will resolve and sort of work accidentally. They also never produce any warnings.

When trying to fix this I stumbled over a lot of code that sort of relies on this behavior. Most prominently defrecord and exists?.

defrecord can easily be fixed since it uses cljs.core.MapEntry [1] instead of cljs.core/MapEntry. cljs.core.MapEntry resolves to cljs/core.MapEntry and pretty much all dotted symbols will resolve to the first segment turning into the namespace. This works since munge turns / into . later on.

exists? will split some.nested.Thing into checking some, some.nested, some.nested.Thing which again is some, some/nested, some/nested.Thing. I tried fixing this directly in cljs.analyzer/analyze-symbol by properly desugaring some.nested.Thing into (. (. some -nested) -Thing) but this is insufficient since macros can directly call cljs.analyzer/resolve-var with dotted symbols (eg. exists?).

At this point I'm unsure how to fix this. I do think this is worth fixing but it may produce lots of warnings in user code. Is that acceptable?

[1] https://github.com/clojure/clojurescript/blob/6062744a1600479d5b9c641db9fb15cbb

Show
Thomas Heller added a comment - I just stumbled over this when trying to figure out why process.env.FOO (without js/) produces no warnings and noticed that pretty much all symbols with dots will resolve and sort of work accidentally. They also never produce any warnings. When trying to fix this I stumbled over a lot of code that sort of relies on this behavior. Most prominently defrecord and exists?. defrecord can easily be fixed since it uses cljs.core.MapEntry [1] instead of cljs.core/MapEntry. cljs.core.MapEntry resolves to cljs/core.MapEntry and pretty much all dotted symbols will resolve to the first segment turning into the namespace. This works since munge turns / into . later on. exists? will split some.nested.Thing into checking some, some.nested, some.nested.Thing which again is some, some/nested, some/nested.Thing. I tried fixing this directly in cljs.analyzer/analyze-symbol by properly desugaring some.nested.Thing into (. (. some -nested) -Thing) but this is insufficient since macros can directly call cljs.analyzer/resolve-var with dotted symbols (eg. exists?). At this point I'm unsure how to fix this. I do think this is worth fixing but it may produce lots of warnings in user code. Is that acceptable? [1] https://github.com/clojure/clojurescript/blob/6062744a1600479d5b9c641db9fb15cbb
Hide
Thomas Heller added a comment -

FWIW I added a rather hackish workaround for this to shadow-cljs [1] and it already discovered several actual bugs in popular CLJS libraries as well as a couple problems in cljs.core itself. It is by no means a complete summary but these should probably be fixed prior to proceeding with this as it will produce lots of warnings otherwise. Still unsure how to fix this cleanly though.

https://dev.clojure.org/jira/browse/CLJS-2982
https://dev.clojure.org/jira/browse/CLJS-2983
https://dev.clojure.org/jira/browse/CLJS-2984

https://github.com/Day8/re-frame-10x/issues/219
https://github.com/nathanmarz/specter/issues/267

[1] https://github.com/thheller/shadow-cljs/blob/22944d9bba14a8428efbd52463a37eb636017609/src/main/shadow/build/cljs_hacks.cljc#L239-L308

Show
Thomas Heller added a comment - FWIW I added a rather hackish workaround for this to shadow-cljs [1] and it already discovered several actual bugs in popular CLJS libraries as well as a couple problems in cljs.core itself. It is by no means a complete summary but these should probably be fixed prior to proceeding with this as it will produce lots of warnings otherwise. Still unsure how to fix this cleanly though. https://dev.clojure.org/jira/browse/CLJS-2982 https://dev.clojure.org/jira/browse/CLJS-2983 https://dev.clojure.org/jira/browse/CLJS-2984 https://github.com/Day8/re-frame-10x/issues/219 https://github.com/nathanmarz/specter/issues/267 [1] https://github.com/thheller/shadow-cljs/blob/22944d9bba14a8428efbd52463a37eb636017609/src/main/shadow/build/cljs_hacks.cljc#L239-L308
Hide
Thomas Heller added a comment -

Another issue was found in the rrb-vector lib. [1]

The symbol

clojure.core.rrb_vector.rrbt.Vector
technically produces the correct JS results but will never have any analyzer data associated with it. Granted that isn't required in this place it still feels like something that should probably produce a warning.

[1] https://github.com/clojure/core.rrb-vector/blob/88c605a72f1176813ca71d664275d480285f634e/src/main/cljs/clojure/core/rrb_vector/macros.clj#L23-L24

Show
Thomas Heller added a comment - Another issue was found in the rrb-vector lib. [1] The symbol
clojure.core.rrb_vector.rrbt.Vector
technically produces the correct JS results but will never have any analyzer data associated with it. Granted that isn't required in this place it still feels like something that should probably produce a warning. [1] https://github.com/clojure/core.rrb-vector/blob/88c605a72f1176813ca71d664275d480285f634e/src/main/cljs/clojure/core/rrb_vector/macros.clj#L23-L24
Hide
Mike Fikes added a comment -

FWIW, the issue Thomas points to does in fact derail self-hosted ClojureScript, so if we improved warnings around this, that would be good. See CRRBV-16

Show
Mike Fikes added a comment - FWIW, the issue Thomas points to does in fact derail self-hosted ClojureScript, so if we improved warnings around this, that would be good. See CRRBV-16

People

Vote (4)
Watch (2)

Dates

  • Created:
    Updated: