<< Back to previous view

[CLJS-1652] Self-host: Avoid alias so cljs.spec loadable Created: 28/May/16  Updated: 28/May/16  Resolved: 28/May/16

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-1652.patch    
Patch: Code

 Description   

The cljs.spec macro namespace employs the Clojure alias function to establish c as an alias for clojure.core. This prevents this namespace from being compiled as ClojureScript and thus bootstrapped ClojureScript environments cannot load it.

Proposed fix: Simply avoid alias and inline the namespace to form qualified symbols.



 Comments   
Comment by David Nolen [ 28/May/16 8:21 PM ]

fixed https://github.com/clojure/clojurescript/commit/8477f19dcf67a8f305b46f2fd2e793586e027263





[CLJS-1649] Possible issue with in cljs.reader or cljs.core/PersistentHashMap Created: 26/May/16  Updated: 27/May/16  Resolved: 26/May/16

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Rohit Aggarwal Assignee: David Nolen
Resolution: Completed Votes: 1
Labels: None
Environment:

Clojurescript version 1.8.51

Tested this on multiple browsers (Chrome/Safari/Firefox) and node.js versions on Linux and OS X.


Attachments: Text File CLJS-1649-1.patch     Text File CLJS-1649-2.patch     Text File CLJS-1649-3.patch     Text File CLJS-1649.patch     File core.cljs     PNG File ScreenShot.png    

 Description   

See attached source code core.cljs for the code.

I apologize for this very specific code example. Its been extracted from a larger app and this very specific arrangement of things is required to trigger the issue.

Problem

A map being returned by cljs.reader/read-string is behaving like a map in the sense of key lookups.

Walkthrough of the code.

(See attached Snapshot.png for sample output)

Line 7 - declare the edn string

Lines 9-43 - declare an unused map called date-formatters

Lines 45-66 - declare an unused map called COLOURS

Lines 68-77 - this is the real crux of the problem.

Line 70 - convert an edn string to a cljs datastructure using {{cljs.reader/read-string}. The datastructure is a map and more specifically a PersistentHashMap. Its called hm-data.

Line 71 - convert the hash-map from previous line to another map using (into {} hm-map). This datastructure is called am-data

Line 72 - print the type of hm-data. Sample output cljs.core/PersistentHashMap

Line 73 - print the type of am-data. Sample output cljs.core/PersistentArrayMap

Line 74-75 - compare the set of keys of the two data structures. result is true

Line 76 - Check if :version key is present in am-data. Sample output true

Line 77 - Check if :version key is present in hm-data. Sample output false

This is the problem. We already established from line 74-75 that both maps have the same keys. But Lines 76 and 77 are contradicting that fact.

Also key looks on the hm-data fail which succeed for am-data.

How to get the expected behaviour

  • Remove unused cljs.pprint require in the ns macro. We are not using this. So it shouldn't have any effect on the problem imho.
  • Remove unused date-formatters, COLOURS. Again we are not using this and it shouldn't have any effect on the working of the problem imho.
  • Remove a few key-vals from either of date-formatters, COLOURS. Even removing a single pair of key-vals would work. Again we are not using this and it shouldn't have any effect on the working of the problem.

If we were to make the initial edn string to a smaller one, the program would also work as expected. But with the original string, the above three things would also make it work.



 Comments   
Comment by Rohit Aggarwal [ 26/May/16 3:25 AM ]

I've tried using cljs.tools.reader/read-string from tools.reader library and it has the same behaviour.

Comment by Rohit Aggarwal [ 26/May/16 4:18 AM ]

Sorry the problem should state:

Problem

A map being returned by cljs.reader/read-string is not behaving like a map in the sense of key lookups.

Comment by Saurabh Kapoor [ 26/May/16 5:19 AM ]

I used http://app.klipse.tech/ (on Chrome) to quickly replicate the defect by pasting the attached file. I find that the output is as mentioned in the defect: Line 77 returns false.

Also, once the program is run on Klipse, I can no longer use that session to compile any programs. The following error is shown:

#error {:message "ERROR", :data {:tag :cljs/analysis-error}, :cause #object[TypeError TypeError: Cannot read property 'call' of undefined]}

Could it be that some global state gets changed for the worse?

Comment by Thomas Heller [ 26/May/16 11:06 AM ]

This seems to be related to other hashing issues we have seen before.

eg.
http://dev.clojure.org/jira/browse/CLJS-830
http://dev.clojure.org/jira/browse/CLJS-1380

I added a little snippet of code that would print the hash of the :version keyword. The version created by reader/read-string is different from a standard code one.

[:version 425292698]
[:read :version 430259761]
(defn run-ver
  [f]
  (let [data  (.readFileSync fs f "utf8")
        proj  (reader/read-string data)]
    (doseq [k (keys proj)]
      (prn [:read k (hash k)]))
    (println (keys proj))
    (println (:version proj))
    (if (:version proj)
      (println ":version key is present")
      (println ":version key is not present!!"))))

(defn main []
  (prn [:version (hash :version)])
  (run-ver "data.edn"))

This explains why array-map works since it only compares the keys and does not check the hash.

Comment by Rohit Aggarwal [ 26/May/16 12:37 PM ]

Continuing on the investigation by Thomas Heller, the issue seems to be in cljs.core/hash-string which uses a javascript object as a cache. If I replace that version with a version which doesn't use a cache, the code works as expected!

Comment by Rohit Aggarwal [ 26/May/16 1:04 PM ]

Alternatively, replacing hash-string with the following also fixes the problem:

(defn hash-string [k]
  (when (> string-hash-cache-count 255)
    (set! string-hash-cache (js-obj))
    (set! string-hash-cache-count 0))
  (if (nil? k)
    0
    (let [h (aget string-hash-cache k)]
      (if (number? h)
        h
        (add-to-string-hash-cache k)))))
Comment by Rohit Aggarwal [ 26/May/16 1:41 PM ]

The above solution works because javascript stores keys in an object as a string.

To highlight this:

(set! cljs.core/string-hash-cache (js-obj))
(set! cljs.core/string-hash-cache-count 0)
(println (hash-string "null"))
(println (hash-string nil))

produces

3392903
3392903

whereas,

(set! cljs.core/string-hash-cache (js-obj))
(set! cljs.core/string-hash-cache-count 0)
(println (hash-string nil))
(println (hash-string "null"))

produces

0
0

With the proposed replacement function, the hash-string function produces the right answer, namely 3392903 for "null" and 0 for nil irrespective of the order of operation.

Comment by Rohit Aggarwal [ 26/May/16 1:54 PM ]

I am attaching proposed fix to the issue along with a description of the problem.

Comment by Rohit Aggarwal [ 26/May/16 1:57 PM ]

Updated patch with better message grammar.

Comment by Rohit Aggarwal [ 26/May/16 3:33 PM ]

Updated the patch with a test which checks if the hash-string of nil is 0 and of "null" is non-zero.

Comment by Rohit Aggarwal [ 26/May/16 3:39 PM ]

I didn't upload the right file earlier.

It should be fixed now

Comment by David Nolen [ 26/May/16 3:43 PM ]

fixed https://github.com/clojure/clojurescript/commit/9887580fc731e3194e1619ba603d30e2548cc768

Comment by Rohit Aggarwal [ 26/May/16 4:55 PM ]

thank you!

Comment by Rohit Aggarwal [ 27/May/16 7:18 AM ]

For future reference, the reason why the patch fixes the problem:

  1. When the system starts cache contains nil as "null" and its value is 0.
  2. We compute hash of :version taking that 0 into account.
  3. Loads of operations such that the cache is reset.
  4. We calculate the hash of "null" as it is in a set. Now the value in the cache of "null" is something other than 0.
  5. We create a new :version taking the new value of "null" cache entry and try to compare its hash to earlier one and they do not match.




[CLJS-1645] Bug in closure/compiled-file Created: 24/May/16  Updated: 26/May/16  Resolved: 26/May/16

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Mike Kaplinskiy Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File patch.diff    
Patch: Code

 Description   

It's currently referencing the wrong namespace on a ::keyword.



 Comments   
Comment by David Nolen [ 26/May/16 3:17 PM ]

this is not a bug, it's intentional





[CLJS-1647] Parallel-build catches all compile exceptions Created: 24/May/16  Updated: 26/May/16  Resolved: 26/May/16

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Juho Teperi Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-1647.patch    

 Description   

compile-task used by parallel-build catches all the exceptions from -compile: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/closure.clj#L826-L828. The exceptions are just printed. This means that build tooling will be unable to control how to show these errors the the user (e.g. Figwheel or Boot-cljs HUD).

Currently if multiple exceptions are found parallel, all would be printed out.

So that parallel-build would work similar to normal build, it should throw a single exception. I don't think it matters too much which exception this is, if multiple occur at the same time.

I propose following fix:

  • remove debug-prn printting the exception
  • save the exception to failed atom
  • check the failed atom state at parallel-compile-sources and throw the exception if it exists


 Comments   
Comment by Juho Teperi [ 24/May/16 3:37 PM ]

Tested the patch with boot-cljs and figwheel and it works.

Comment by David Nolen [ 26/May/16 3:12 PM ]

https://github.com/clojure/clojurescript/commit/fc827fa5b0f1b290523f304fd84cf0a6c2a7bfa2





Generated at Mon May 30 23:04:34 CDT 2016 using JIRA 4.4#649-r158309.