<< Back to previous view

[CLJS-1432] 1.7.48 assigns same value for '$ and '. symbols under advanced Created: 24/Aug/15  Updated: 27/Aug/15  Resolved: 27/Aug/15

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

Type: Defect Priority: Minor
Reporter: Nikita Prokopov Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs-1432-symbols-collision-2.patch     Text File cljs-1432-symbols-collision.patch    

 Description   
(println :+ :a :$ :. :a$b :a.b :z)
      => :+ :a :. :. :a$b :a$b :z

(println '+ 'a '$ '. 'a$b 'a.b 'z)
      =>  +  a  .  .  a$b  a$b  z

expected:
(not= :$ :.)
(not= '$ '.)

Related:
CLJS-1105
https://github.com/clojure/clojurescript/commit/d92b3004046d1f83ab76a2b94f4129cb16e47848
https://github.com/tonsky/datascript/issues/108



 Comments   
Comment by Nikita Prokopov [ 26/Aug/15 12:09 AM ]

Just wanted to add that this issue breaks DataScript to the point it’s impossible to use (because it relies on both '$ and '.). DataScript cannot be used at all with 1.7.48, 1.7.58, 1.7.107

Comment by David Nolen [ 26/Aug/15 10:38 PM ]

Why not just special case '. here?

Comment by Nikita Prokopov [ 27/Aug/15 3:34 AM ]

David, I’m not sure what is the purpose of this code, e.g. why it transforms symbol/keyword name at all. If this is going to constants table as a key, it can be any string, right?

My solution just eliminates other possible collisions (e.g. :a$b vs :a.b) because I think it is not fun to encounter stuff like this. It takes really huge effort to debug, because it comes from the most unexpected place: on the one hand, language guarantees keywords are only equal to themselves, on the other, it silently treats different keywords/symbols as the same.

If you think my solution is an overkill, or if there are technical reasons it can’t be applied, as I’m not aware of all the nuances, I leave this decision to you.

Other possible solution might be, if encountered kw/sym that might conflict, do not apply this optimization to them at all. We would probably lose some performance, but keep the guarantees.

Comment by David Nolen [ 27/Aug/15 6:22 AM ]

Emitting strings for the constants table defeats minification and dead code elimination.

The bug is a simple source code generation symbol clash that can be avoided with special casing '.. If you can come up with other problematic cases then I'll consider an alternative. Otherwise this is the only patch we will take.

Comment by Nikita Prokopov [ 27/Aug/15 1:10 PM ]

Attached second patch, as you suggested

Comment by David Nolen [ 27/Aug/15 3:31 PM ]

fixed https://github.com/clojure/clojurescript/commit/c5e088786acbdfa5074199f1c5baa9fa7c30f5a6

Comment by Nikita Prokopov [ 27/Aug/15 3:40 PM ]

Thanks David!





[CLJS-1304] Behavior of clojure.string/replace differs from Clojure Created: 09/Jun/15  Updated: 27/Aug/15  Resolved: 27/Aug/15

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

Type: Defect Priority: Major
Reporter: Luke VanderHart Assignee: Andrew Rosa
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-1304-2.patch     Text File CLJS-1304.patch    
Patch: Code and Test

 Description   

When using clojure.string/replace with a function as the replacement value, and using a regular expression with capturing groups, the behavior differs between Clojure and ClojureScript. In Clojure, the replacement fn is passed a tuple of the match and capturing group. In ClojureScript, it is only passed the match value.

In Clojure:

=> (println (str/replace "foobar" #"f(o)o" #(do (println %) "X")))
[foo o]
"Xbar"

In ClojureScript:

=> (println (str/replace "foobar" #"f(o)o" #(do (println %) "X")))
foo
Xbar


 Comments   
Comment by Daniel Woelfel [ 11/Jun/15 3:31 AM ]

If you're looking for a workaround, you can still get the match result from the 2nd arg:

=> (println (clojure.string/replace "foobar" #"f(o)o" (fn [& args] (do (println args) "X"))))
(foo o 0 foobar)
Xbar
Comment by Andrew Rosa [ 13/Jun/15 12:40 PM ]

Implementation and tests for the requested behaviour. Despite of the `vec` not being strictly necessary, I stick with it so the behaviour will be identical on both implementations.

Comment by David Nolen [ 13/Jun/15 5:07 PM ]

Is there any reason here to not just copy the Clojure implementation?

Comment by Andrew Rosa [ 13/Jun/15 5:25 PM ]

Hi David,

Clojure's impl was the first place that I look for, but this feature in special is written against the underlaying Java libraries. You can check it here.

Studying the Clojure implementation I found that there are some of re-* family of functions missing on ClojureScript side, and unfortunately they are heavily inspired on Java's Regex structure (return matchers, find and extract groups). MAYBE we can something like them to ClojureScript, but I don't know the real value there. Anyway, I think that port them or not is subject for another ticket - if you want that I investigate more, I could create it.

Comment by Francis Avila [ 13/Jun/15 5:47 PM ]

I made a proposal some time ago to bring clj and cljs re handling closer together: CLJS-776

Comment by David Nolen [ 13/Jun/15 8:51 PM ]

Andrew, was just looking for rationale. What you've done looks OK to me. People should try the patch and give feedback. Thanks!

Comment by Andrew Rosa [ 13/Jun/15 10:41 PM ]

Hi David, I understand and appreciate your concerns I just want to make clear what I've search and done, sorry if I sound rude - maybe I stepped on my language limitations.

Francis, really this re-* thing is tricky to deal with. I will take a closer look onto your ticket and see if I can add any idea.

Thanks everyone!

Comment by David Nolen [ 27/Aug/15 5:54 AM ]

This patch needs to be rebased to master.

Comment by Andrew Rosa [ 27/Aug/15 9:50 AM ]

Rebased patch against master.

Comment by David Nolen [ 27/Aug/15 11:14 AM ]

fixed https://github.com/clojure/clojurescript/commit/3e146905999f9b9ffb249e139e266f845c4af34c





[CLJS-1430] self-host: .toString is emitted as .toString$ (munged) Created: 18/Aug/15  Updated: 27/Aug/15  Resolved: 27/Aug/15

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

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-1430-2.patch     Text File CLJS-1430-3.patch     Text File CLJS-1430-4.patch     Text File CLJS-1430.patch    
Patch: Code and Test

 Description   

If you evaluate a form like (.toString "a") in bootstrapped mode, the emitted JavaScript will end up with a dollar-sign: "a".toString$().

This can be reproduced via a compile-str unit test involving the form.



 Comments   
Comment by Mike Fikes [ 18/Aug/15 11:29 PM ]

This is because, even though the compiler indicates that there are no reserved words (the empty set for second argument here):

https://github.com/clojure/clojurescript/blob/v1.7/src/main/clojure/cljs/compiler.cljc#L1006

cljs.core/munge is called here

https://github.com/clojure/clojurescript/blob/v1.7/src/main/clojure/cljs/compiler.cljc#L108

to imitate Clojure's munge. But cljs.core/munge introduces the dollar-sign for reserved JavaScript keywords:

https://github.com/clojure/clojurescript/blob/v1.7/src/main/cljs/cljs/core.cljs#L9898

The direct ClojureScript imitation of Clojure's munge is munge-str here

https://github.com/clojure/clojurescript/blob/v1.7/src/main/cljs/cljs/core.cljs#L9882

Calling that (private) method instead fixes the problem.

Comment by Mike Fikes [ 18/Aug/15 11:37 PM ]

The attached file fixes the issue.

On problem with it, though it it has the compiler calling a private function in cljs.core namespace.

Comment by Mike Fikes [ 18/Aug/15 11:44 PM ]

Attaching a 2nd patch which properly updates the latch count in the unit test.

Comment by David Nolen [ 26/Aug/15 10:37 PM ]

This ticket does not explain why toString would get munged - it's not on the reserved list.

Comment by Mike Fikes [ 26/Aug/15 11:04 PM ]

You are right, David. The root cause actually has to do with (js-reserved? "toString") returning true when it should not do so. I suspect this has to do with the use of gobject/containsKey giving a false positive for things in the Object prototype, viz:

cljs.user=> (require '[goog.object :as gobject])
nil
cljs.user=> (gobject/containsKey #js {} "toString")
true

So, I retract my patch which completely glosses over the aspect that js-reserved? is probably the culprit.

Comment by Mike Fikes [ 26/Aug/15 11:53 PM ]

David, based on your feedback, attached a CLJS-1430-3.patch which might be closer to a correct approach in that it fundamentally revises js-reserved? to not return true for things like "toString".

My JavaScript is not strong enough for a feel as to whether the actual implementation in the patch is up to muster, nor whether munging is in the critical path for perf.

Comment by David Nolen [ 27/Aug/15 6:27 AM ]

Ah ok, revise the patch to remove the two tests, instead just use (.hasOwnProperty js-reserved foo)

Comment by Mike Fikes [ 27/Aug/15 7:56 AM ]

CLJS-1430-4.patch updated to use .hasOwnProperty, and ran all tests except Nashorn. Also ran self-host tests.

Comment by David Nolen [ 27/Aug/15 8:23 AM ]

fixed https://github.com/clojure/clojurescript/commit/8b7177a579d52a146d19902a19aa92708a567983





[CLJS-1353] range inconsistent with Clojure if step is 0 Created: 20/Jul/15  Updated: 26/Aug/15  Resolved: 26/Aug/15

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

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

Attachments: Text File CLJS-1353-14-Aug-15.patch     Text File CLJS-1353-15-Aug-15.patch    
Patch: Code and Test

 Description   
cljs.user=> (range 3 9 0)
()

With CLJ-1018 Clojure behaves this way and docstring for Clojure updated:

user=> (take 10 (range 3 9 0))
(3 3 3 3 3 3 3 3 3 3)


 Comments   
Comment by Samuel Miller [ 14/Aug/15 9:57 PM ]

Uploaded potential patch with updated tests.

Clojures implementation can be seen here...

https://github.com/clojure/clojure/blob/41af6b24dd5be8bd62dc2b463bc53b55e18cd1e5/src/jvm/clojure/lang/Range.java#L89

Basically what is boils down to is as long as the start and end are not equal give back the start number if the step is 0 (effectively repeat). Tried to keep the previous code for range the same and added a case for zero that checks for the equality of start and end.

Ran tests on Linux against SpiderMonkey, V8, and Nashorn they passed. Played around in the repl and compared against Clojure and seems to be equivalent.

Comment by Mike Fikes [ 14/Aug/15 11:51 PM ]

With CLJS-1353-14-Aug-15.patch, tests pass for me on OS X for V8, SpiderMonkey, and JavaScriptCore.

I also tried it in bootstrapped ClojureScript in JavaScriptCore on OS X, manually running the tests successfully.

Thinking about perf, it looks like it doesn't really affect the (probably common) code path for positive steps, and only adds one more necessary check to distinguish between negative and zero (which wasn't being done before). In the zero-step branch (which is probably not common at all), there is perhaps a tiny benefit in using == over = since we can assume numeric arguments. (You end up with a direct start === end as opposed to cljs.core._EQ_ being applied.)

There is one other bit of code that employs that pattern: https://github.com/clojure/clojurescript/blob/r1.7.48/src/main/cljs/cljs/core.cljs#L5058

Comment by David Nolen [ 15/Aug/15 5:22 PM ]

Yes the test should be changed to ==. Thanks.

Comment by Samuel Miller [ 15/Aug/15 11:19 PM ]

Thanks for the reviews. I updated the patch to use == instead of =

Comment by David Nolen [ 26/Aug/15 10:41 PM ]

fixed https://github.com/clojure/clojurescript/commit/bbcadde11fa5941fda804faa8fc92a5f33a242cf





[CLJS-1431] load-file doc output missing arglists Created: 21/Aug/15  Updated: 26/Aug/15  Resolved: 26/Aug/15

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: newbie
Environment:

noderepljs


Attachments: Text File CLJS-1431-8-21-15.patch    
Patch: Code

 Description   

The output for `(doc load-file)` is missing the arglists:

$ script/noderepljs 
ClojureScript Node.js REPL server listening on 52087
To quit, type: :cljs/quit
cljs.user=> (doc load-file)
-------------------------
load-file
REPL Special Function
  Sequentially read and evaluate the set of forms contained in the file.

nil

A line reading ([name]) should appear between load-file and REPL Special Function



 Comments   
Comment by Mike Fikes [ 21/Aug/15 7:50 PM ]

Root cause is the use of :arglist instead of :arglists here:
https://github.com/clojure/clojurescript/blob/v1.7/src/main/clojure/cljs/repl.cljc#L1113

Comment by Chris Pickard [ 21/Aug/15 8:44 PM ]

I've attached a patch (CLJS-1431-8-21-15.patch) that address the issue Mike reported

Comment by Mike Fikes [ 21/Aug/15 9:00 PM ]

For me, Chris Pickard's CLJS-1431-8-21-15.patch applies cleanly to master, produces the correct behavior in script/noderepljs, and unit tests pass for V8, SpiderMonkey, and JavaScriptCore (Nashorn skipped).

Comment by David Nolen [ 26/Aug/15 10:31 PM ]

fixed https://github.com/clojure/clojurescript/commit/19bcf10ccedf63f0e3bf0abfb7481f9c347895fc





[CLJS-1433] self-host: cljs.js/*eval-fn* passed nil :cache Created: 24/Aug/15  Updated: 26/Aug/15  Resolved: 26/Aug/15

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

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

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

 Description   

If you load some ClojureScript source that implements a namespace, the cljs.js/*eval-fn* will always be passed a :cache value of nil



 Comments   
Comment by Mike Fikes [ 24/Aug/15 12:24 PM ]

Simple defect—simply need to deref the atom.

Comment by David Nolen [ 26/Aug/15 10:28 PM ]

fixed https://github.com/clojure/clojurescript/commit/5f3c494ce4274177e8e9cb71adcb87b5f9394423





[CLJS-1403] Add script/bootstrap capability for Windows Created: 08/Aug/15  Updated: 26/Aug/15  Resolved: 26/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Peter Stephens Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None
Environment:

All versions of Windows


Attachments: Text File cljs-1403-v2.patch     Text File cljs-1403-v3.patch    

 Description   

Implement a Windows shell script to download and unpack the correct versions of various ClojureScript dependencies.

Proposal: Implement a PowerShell script to do this work. PowerShell has been pre-installed on Windows since version 7 and is available (in various flavors) on Windows down to XP.

1. Downloading of files from the web is a proper command (Invoke-RestMethod) in PowerShell since version 3. Earlier versions of PowerShell could be supported by using Dotnet's WebClient.DownloadFile method.

2. Unzipping files is a little more difficult. But the GUI shell's capability is exposed through COM and this appears to work well from PowerShell.

3. The current dependency versions can be extracted from the /bin/sh script using regex.

See https://gist.github.com/pstephens/b260b1ad8b166489b562 for a rough draft of this script.



 Comments   
Comment by Peter Stephens [ 08/Aug/15 1:15 PM ]

Related issues: CLJS-24 and CLJS-66, both closed.

Comment by Peter Stephens [ 08/Aug/15 9:05 PM ]

Almost have a patch ready to submit... but lot's of the Windows shell wrappers are also broken. Working through the problems so I can hopefully get through the test suite on Windows.

Comment by David Nolen [ 09/Aug/15 10:25 AM ]

Thanks for working on this!

Comment by Peter Stephens [ 11/Aug/15 9:28 PM ]

This patch includes some basic Windows support scripts.
1. A port of script/bootstrap to PowerShell
2. A port of script/test to PowerShell
3. Updated bin/cljsc.bat to match bin/cljsc
4. And a minor tweak to bin/cljsc.clj to write exceptions to stderr and describe the nature of the exception.

Comment by Peter Stephens [ 11/Aug/15 9:46 PM ]

Tested on Windows 10 w/fresh clojurescript clone.

Bootstrap pulled down required dependencies.

Ran the test suite through Nashorn with two (apparently known) failing tests:
FAIL in (test-919-generic-cas) (at <anonymous> (builds/out-adv/core-advanced-test.js:NaN:963)
testing CLJS-919, CAS should on custom atom types
expected: (== (clojure.core/deref a0) 10)
actual: (not (== nil 10))

FAIL in (test-919-generic-cas) (at <anonymous> (builds/out-adv/core-advanced-test.js:NaN:963)
testing CLJS-919, CAS should on custom atom types
expected: (== (clojure.core/deref a1) 20)
actual: (not (== nil 20))

I couldn't find a Jira issue describing these failures but some work was done by Sebastian Bensusan on this: http://www.raynes.me/logs/irc.freenode.net/clojurescript/2015-04-20.txt and https://gist.github.com/bensu/c6302a33e56d5d7eae9e

Comment by David Nolen [ 12/Aug/15 6:49 AM ]

Patch looks good but I could not apply with git am. Did you following the patch instructions here https://github.com/clojure/clojurescript/wiki/Patches?

Also please submit only squashed patches. Thanks!

Comment by Peter Stephens [ 12/Aug/15 9:18 PM ]

Apparently there are encoding and and line ending issues from Windows generated patches using the linked patching instructions. I'll squash the commits and work out the encoding issues. Then I'll test-apply the patch to a fresh clone of master before resubmitting.

Comment by Peter Stephens [ 16/Aug/15 9:59 PM ]

1. Squished the patch. There is now only one commit.
2. Created the patch with Windows git bash shell. So the encoding is in proper ASCII without extra fluff.

NOTE: The batch and PowerShell files are encoded with CRLF... so special care needs to be taken when applying this patch. Here is the command I used:

git am --keep-cr --whitespace=nowarn < cljs-1403-v3.patch

The patch will not apply without keep-cr because the CR characters will be stripped out. The whitespace=nowarn option suppresses warnings but the patch will still apply without this option.

Comment by David Nolen [ 26/Aug/15 10:24 PM ]

fixed https://github.com/clojure/clojurescript/commit/d7740694be49ba38c778deae59a3e047ca881631





Generated at Sat Aug 29 02:26:27 CDT 2015 using JIRA 4.4#649-r158309.