<< Back to previous view

[CLJS-861] In Clojurescript IE8 cljs.reader can't read numbers Created: 18/Sep/14  Updated: 22/Sep/14

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

Type: Defect Priority: Major
Reporter: Zubair Quraishi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: patch
Environment:

IE8 Web Browser (used in my company)


Attachments: PDF File Clojure_CA.pdf     File reader.cljs    
Patch: Code

 Description   

This change is made because IE8 always returns 0 when cljs.reader reads an integer.

This is caused because the matching pattern in most browsers returns nil for unmatched patterns but IE8 returns "" instead of nil



 Comments   
Comment by Zubair Quraishi [ 18/Sep/14 10:33 PM ]

https://github.com/clojure/clojurescript/pull/39#issuecomment-56069471

https://github.com/zubairq/clojurescript/commit/7ec0144c53b47a831f8696f9eaf97cbcfe06b2e7

https://github.com/JulianBirch/cljs-ajax/issues/46

Comment by David Nolen [ 19/Sep/14 11:25 AM ]

Please e-sign the CA here: http://clojure.org/contributing, thanks!

Comment by Zubair Quraishi [ 19/Sep/14 12:14 PM ]

Ok, I e-signed it and I got back a PDF. Do I have to send the PDF or attach it anywhere?

Comment by David Nolen [ 19/Sep/14 3:16 PM ]

You are now listed as a contributor. I need a properly formatted patch please follow these instructions: https://github.com/clojure/clojurescript/wiki/Patches. Thanks.

Comment by Zubair Quraishi [ 20/Sep/14 6:26 AM ]

I'm not sure what I have to do different from the patch I already submitted? I made a branch, ie8-fix-cljs-reader and made the change to:

https://github.com/zubairq/clojurescript/blob/ie8-fix-cljs-reader/src/cljs/cljs/reader.cljs

https://github.com/zubairq/clojurescript/commit/7ec0144c53b47a831f8696f9eaf97cbcfe06b2e7

I used "CLJS-NNN: TICKET TITLE" as the title for the ticket, "CLJS-854: Fix for IE8 cljs.reader to read integers correctly"

and then I did "git format-patch master --stdout > cljs_ticket_number.patch" before I pushed the change, with "git format-patch master --stdout > cljs_CLJS-854.patch".

Am I missing a step or have a made a mistake, or do I just need to do the same thing again?

Comment by Francis Avila [ 20/Sep/14 3:15 PM ]

You never actually attached the patch file to this ticket.

Comment by Zubair Quraishi [ 22/Sep/14 2:08 AM ]

Ok, sorry, I just added the source file. What is the file extension of the patch file? (My first patch ever to a project so I am new to this - I am not a developer anymore so please forgive me not knowing this as I may be a bit out of date in my knowledge)

Comment by Francis Avila [ 22/Sep/14 7:14 AM ]

The file you created with git format-patch is the file you must attach. That is the only reason that command is run. From what you have written here the file is probably called {{cljs_CLJS-854.patch}}. Pushing your change anywhere is unnecessary--all we need is the patch file.





[CLJS-862] re-pattern is inconsistent with Clojure Created: 19/Sep/14  Updated: 19/Sep/14

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

Type: Defect Priority: Major
Reporter: Joel Holdbrooks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 862.patch    

 Description   

(re-pattern #"foo") fails with "TypeError: re-find must match against a string." This is because, unlike Clojure, there is no check to see if the argument, s, is a RegExp first.



 Comments   
Comment by Joel Holdbrooks [ 19/Sep/14 4:08 PM ]

Patch attached.





[CLJS-854] In Clojurescript IE8 cljs.reader can't read numbers Created: 10/Sep/14  Updated: 18/Sep/14

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

Type: Defect Priority: Major
Reporter: Zubair Quraishi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

In Chrome and Firefox:

(-> "{:s \"string\", :l 42, :k :keyword, :d 1.337}" (cljs.reader/read-string) (prn-str))

returns

{:s "string", :d 1.337, :k :keyword, :l 42}

:but in IE8 it returns:

{:s "string", :d 1.337, :k :keyword, :l 0}

See this thread:

https://github.com/JulianBirch/cljs-ajax/issues/46



 Comments   
Comment by Thomas Heller [ 10/Sep/14 7:37 AM ]

FWIW I just tested this on IE8 via the console: cljs.reader.read_string("42") => 0

Any Integer seems to fail.

Comment by Zubair Quraishi [ 10/Sep/14 7:58 AM ]

What do I do now with this JIRA issue to find out if it can be resolved? Do I need to assign it to someone, or am I supposed to fix it myself, I'm not really sure.

Comment by Nicola Mometto [ 10/Sep/14 12:12 PM ]

cljs.reader has no relation whatsoever with tools.reader, the title is misleading

Comment by Thomas Heller [ 10/Sep/14 12:30 PM ]

@Zubair: you could attempt a fix, I guess the error is somewhere in here:

https://github.com/clojure/clojurescript/blob/master/src/cljs/cljs/reader.cljs#L112

Too busy at the moment to take a closer look myself.

Comment by Francis Avila [ 10/Sep/14 1:09 PM ]

Actually I'd start with the regex that matches int. It has a non-capturing group in it which historically has had some cross-browser inconsistencies. See if that regex by itself matches int literals in IE or not.

Unfortunately I don't have any IEs to test.

Comment by Zubair Quraishi [ 10/Sep/14 2:47 PM ]

Ok, thanks, I could do that. Do you know if there is a guide which can show me how to do this. I saw the official guide here:

https://github.com/clojure/clojurescript/wiki/Windows-Setup

But it seems to use Clojure 1.3. Is this still the way to set it up on Windows?

Comment by Zubair Quraishi [ 10/Sep/14 2:48 PM ]

Or do I just set up and amend clojure reader on its own?

Comment by Zubair Quraishi [ 10/Sep/14 3:11 PM ]

OK, I'm looking in the reader now, so did you mean the line:

(def int-pattern (re-pattern "^([-+]?)(?0)|([1-9][0-9]*)|0[xX]([0-9A-Fa-f]+)|0([0-7]+)|([1-9][0-9]?)[rR]([0-9A-Za-z]+))(N)?$"))

Comment by Zubair Quraishi [ 16/Sep/14 2:51 AM ]

I am still looking at this and trying to find the error cases. Even 0 on its own fails in IE8. See:

http://connecttous.co/connecttous/connecttous.html

: and click on login to see how your browser parses different numbers:

I am still trying to track down the problem area, and currently looking around here:

(defn read-number
  [reader     initch]

  (loop [
         buffer   (gstring/StringBuffer. initch)
         ch       (read-char reader)
         ]
    
    (if (or (nil? ch) (whitespace? ch) (macros ch))
      
      (do
        (unread reader ch)
        (let [s (.toString buffer)]
          (or (match-number s)
              (reader-error reader "Invalid number format [" s "]"))))
      
      (recur (do (.append buffer ch) buffer) (read-char reader)))))
Comment by Zubair Quraishi [ 17/Sep/14 3:17 AM ]

Changing:

(defn- match-int
  [s]
  (let [groups (re-matches* int-pattern s)
        zero (aget groups 2)]
    (if-not (nil? zero)
      0
      (let [a (cond
               (aget groups 3) (array (aget groups 3) 10)
               (aget groups 4) (array (aget groups 4) 16)
               (aget groups 5) (array (aget groups 5) 8)
               (aget groups 6) (array (aget groups 7)
                                      (js/parseInt (aget groups 6) 10))
               :else (array nil nil))
            n (aget a 0)
            radix (aget a 1)]
        (when-not (nil? n)
          (let [parsed (js/parseInt n radix)]
            (if (identical? "-" (aget groups 1))
              (- parsed)
              s)))))))
              ;parsed)))))))

:gives the following result in Chrome:

Parsing {:s "string", :l 42, :k :keyword, :d 1.337}
{:s "string", :l "42", :k :keyword, :d 1.337}
Parsing {:l 42}
{:l "42"}
Parsing 42
"42"

:gives the following result in IE8:

Parsing {:s "string", :l 42, :k :keyword, :d 1.337}
{:s "string", :l 0, :k :keyword, :d 1.337}
Parsing {:l 42}
{:l 0}
Parsing 42
0

:which means that "(defn- match-int [s]" is being passed 0 in IE8. This is called from here:

(defn- match-number
  [s]
  (cond
   (re-matches* int-pattern s) (match-int s)
   (re-matches* ratio-pattern s) (match-ratio s)
   (re-matches* float-pattern s) (match-float s)))

:which means that defn read-number is passing in the wrong text:

(defn read-number
  [reader  initch]
  (loop [buffer (gstring/StringBuffer. initch)
         ch (read-char reader)]
    (if (or (nil? ch) (whitespace? ch) (macros ch))
      (do
        (unread reader ch)
        (let [s (.toString buffer)]
          (or (match-number s)
              (reader-error reader "Invalid number format [" s "]"))))
      (recur (do (.append buffer ch) buffer) (read-char reader)))))

: So more investigation needed by me

Comment by Thomas Heller [ 17/Sep/14 4:50 AM ]

My guess would be that this check "if-not (nil? zero)" fails, terminates early and never actually gets to the parsing bits.

Comment by Zubair Quraishi [ 17/Sep/14 5:13 AM ]

I first thought it may be

if-not (nil? zero)

failing, but you can see that I changed match-int to return the input

s)))))))
;parsed)))))))

So this means that before

if-not (nil? zero)
is even called, match-int is getting the wrong input

Does that make sense, or have I missed something?

Comment by Thomas Heller [ 17/Sep/14 6:00 AM ]

The branch that returns the unparsed value is never reached when the if is true, the else branch (let and your change) have no effect in this case.

Comment by Zubair Quraishi [ 18/Sep/14 3:30 AM ]

Yes, you are right, that code was never called. I changed the definition of match-int to

(defn- match-int
  [s]
  (let [groups (re-matches* int-pattern s)
        zero (aget groups 2)]
    groups))

and in Chrome the result is

Parsing {:s "string", :l 42, :k :keyword, :d 1.337}
{:s "string", :l #js ["42" "" nil "42"], :k :keyword, :d 1.337}
Parsing {:l 42}
{:l #js ["42" "" nil "42"]}
Parsing 42
#js ["42" "" nil "42"]

and in IE8 the result is

Parsing {:s "string", :l 42, :k :keyword, :d 1.337}
{:s "string", :l #js ["42" "" "" "42"], :k :keyword, :d 1.337}
Parsing {:l 42}
{:l #js ["42" "" "" "42"]}
Parsing 42
#js ["42" "" "" "42"]
Comment by Zubair Quraishi [ 18/Sep/14 3:31 AM ]

So my next move will be to search for "" and replace it will nil, and try that

Comment by Zubair Quraishi [ 18/Sep/14 3:43 AM ]

Something like:

(defn- match-int
  [s]
  (let [
        groups2 (re-matches* int-pattern s)
        groups  (into [] (map (fn[x] (if (= x "") nil x)) groups2))
        zero    (aget groups 2)]
    (if-not (nil? zero)
      0
      (let [a (cond
               (aget groups 3) (array (aget groups 3) 10)
               (aget groups 4) (array (aget groups 4) 16)
               (aget groups 5) (array (aget groups 5) 8)
               (aget groups 6) (array (aget groups 7)
                                      (js/parseInt (aget groups 6) 10))
               :else (array nil nil))
            n (aget a 0)
            radix (aget a 1)]
        (when-not (nil? n)
          (let [parsed (js/parseInt n radix)]
            (if (identical? "-" (aget groups 1))
              (- parsed)
              parsed)))))))
Comment by Zubair Quraishi [ 18/Sep/14 3:43 AM ]

where:

groups2  (re-matches* int-pattern s)
groups   (into [] (map (fn[x] (if (= x "") nil x)) groups2))

has been changed

Comment by Zubair Quraishi [ 18/Sep/14 11:52 AM ]

Ok, it seems to work in IE8 now. I have created the change and made a pull request:

https://github.com/clojure/clojurescript/pulls

https://github.com/zubairq/clojurescript/commit/1217ae69e80bf2b2093de6d46ad4b71242fdf0ca

(defn- match-int
   [s]
   (let [groups (re-matches* int-pattern s)
-        zero (aget groups 2)]
+        ie8-fix  (aget groups 2)
+        zero     (if (= ie8-fix "") nil ie8-fix)]
     (if-not (nil? zero)
       0
       (let [a (cond
Comment by Zubair Quraishi [ 18/Sep/14 12:47 PM ]

Resubmitted the patch as:

https://github.com/zubairq/clojurescript/commit/7ec0144c53b47a831f8696f9eaf97cbcfe06b2e7

Comment by Nicola Mometto [ 18/Sep/14 1:13 PM ]

Zubair, as per David's comment on your first PR (https://github.com/clojure/clojurescript/pull/39#issuecomment-56069471), clojurescript doesn't take pull requests, please attach the patch in this ticket





[CLJS-860] redundant analysis of source files in JARs Created: 18/Sep/14  Updated: 18/Sep/14

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Source files in JARs are analyzed once then analyzed again when copied to the output directory.






[CLJS-850] No warning for unresolved vars when using aliased namespace Created: 04/Sep/14  Updated: 18/Sep/14  Resolved: 18/Sep/14

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

Type: Defect Priority: Major
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   
(ns foo
  (:require [cljs.core.async :as a]))

(defn bar []
 (a/go
   ...))

This code compiles fine, and then throws an error at runtime, cljs.core.async.a.go.call is not defined.



 Comments   
Comment by David Nolen [ 18/Sep/14 12:26 AM ]

CLJS-858





[CLJS-702] warn if protocol signature doesn't matched declared Created: 27/Nov/13  Updated: 18/Sep/14  Resolved: 18/Sep/14

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

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


 Comments   
Comment by David Nolen [ 18/Sep/14 12:25 AM ]

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





[CLJS-701] warn if protocol used multiple times in a deftype Created: 27/Nov/13  Updated: 18/Sep/14  Resolved: 18/Sep/14

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None





[CLJS-704] warn if type extended to same protocol multiple times Created: 27/Nov/13  Updated: 17/Sep/14  Resolved: 17/Sep/14

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

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


 Comments   
Comment by David Nolen [ 17/Sep/14 5:58 PM ]

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





[CLJS-859] Downloads for bootstrap script should happen over HTTPS. Created: 17/Sep/14  Updated: 17/Sep/14  Resolved: 17/Sep/14

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

Type: Enhancement Priority: Major
Reporter: David Kinzer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bootstrap, https

Attachments: Text File bootstrap-over-https.patch    
Patch: Code

 Description   

Currently all downloads for the clojurescript bootstrap process are happening over HTTP. If this is switched to HTTPS it adds a layer of security at no real cost.



 Comments   
Comment by David Nolen [ 17/Sep/14 12:20 PM ]

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





[CLJS-857] change deftype*/defrecord* special forms to include their inline methods decls Created: 15/Sep/14  Updated: 16/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-change-deftype-defrecord-special-forms-to-include-th.patch    
Patch: Code

 Description   

Currently the deftype* and defrecord* special forms don't include their inline method declarations, this means that code like:

(deftype foo [x] Object (toString [_] x))

is macroexpanded into something that looks like:

(deftype* foo [x] pmask) (extend-type foo Object (toString [_] x)) ...

The issue with this approach is that we want to treat "x" in the extend-type expression as if it was inside the lexical scope of the deftype*, to do so the analyzer attaches a metadata flag to the extend-type call with the deftype* local fields.

This is not ideal and complicates needlessly the analyzer, I propose to simply move the extend-type call as a parameter to the deftype* special form, this way no special handling for local fields will be needed:

(deftype foo [x] pmask (extend-type foo Object (toString [_] x)))

since the extend-type code is effectively inside the lexical scope of the deftype.

Everything said above applies to defrecord* aswell, this patch implements what I proposed and refactors a bit the code in the analyzer to remove the code duplication in the defrecord* and deftype* parse implementations.

In addition, the current approach was unfeasable to implement for tools.analyzer.js so taking this patch would mean making cljs.analyzer and tools.analyzer.js special-form compatible.






[CLJS-855] Combinatorial code explosion with nested functions of unknown arity Created: 12/Sep/14  Updated: 16/Sep/14  Resolved: 16/Sep/14

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

Type: Defect Priority: Minor
Reporter: Brendan Younger Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None
Environment:

ClojureScript 0.0-2322


Attachments: File test-advanced.js     File test-whitespace.js    

 Description   

The following code, when compiled with :advanced optimizations leads to 32 repetitions of the inner number 2. On less trivial examples, it leads to multi-megabyte outputs very quickly. I have attached the compiler output for :whitespace and :advanced for the latest version of the CLJS compiler.

(def good {:a {:b {:c {:d {:e 1}}}}})

(def a identity)
(def b identity)

(def bad (apply (fn [x y] (x (y (x (y (x 2)))))) [a b]))


 Comments   
Comment by Francis Avila [ 12/Sep/14 4:48 PM ]

Is this more a non-native vs native function issue than an arity knowledge issue? Does whitespace compilation explode the same way if compiled with :static-fns true?

A solution might be to only emit the `x.call(null, arg)` form if we don't know whether `x` is a cljs function at compile time. However the arity dispatch wrapper function is not optimizable by v8 right now because of how it uses `arguments` so it would be a performance regression in the non-native case.

But another thing to consider is that google advance compilation output is intended to be compressed: test-whitespace.js and test-advanced.js both gzip to exactly 379 bytes. So maybe this is a non-problem?

Comment by Brendan Younger [ 12/Sep/14 10:46 PM ]

This is an unknown arity issue, not one of native versus non-native. The indirection introduced by the explicit apply function leads to the duplication of code. Compiling with :static-fns doesn't materially change anything with :whitespace or :advanced compilation.

Whether or not gzip is able to undo the code explosion, the browser still has to parse O(2^{nesting depth}) characters which means the parsing time is increased in proportion and, likely, the internal representation of the javascript is as well. For a mobile phone with constrained memory and processing power, this is a big deal. I would argue that any code duplication leading to an exponential increase in code size is a serious defect.

Comment by David Nolen [ 15/Sep/14 12:22 PM ]

Brendan do you have an actual example where this issue matters in practice? Thanks.

Comment by micha niskin [ 16/Sep/14 5:58 PM ]

David, I have seen this with Hoplon applications. Hoplon code tends to have the sort of deeply nested expressions you see there, because the DOM tree can be both quite broad and quite deep.

In a real application we found that when attempting to compile with advanced optimizations GClosure would generate gigabytes and gigabytes of JavaScript and never finish. Without optimizations the application would take five minutes or more to load in FireFox and Safari (Chrome, for some reason, was okay, which threw us off initially.)

In fact, I ended up writing a macro to perform a sort of SSA unfolding and flattening in order to work around the issue. The macro basically transforms this:

(a (b (c (d) (e) (f))))

into this:

(let [x1 (d)
      x2 (e)
      x3 (f)
      x4 (c x1 x2 x3)
      x5 (b x4)
      x6 (a x5)]
  x6)

The macro is here: https://github.com/tailrecursion/hoplon/blob/5.10.23/src/tailrecursion/hoplon.clj#L101-L105 and it's applied automatically to the contents of the <body> of the document here: https://github.com/tailrecursion/hoplon/blob/5.10.23/src/tailrecursion/hoplon.clj#L119-L124

When I wrap large expressions with this macro the problem is mitigated. It's obviously a filthy hack and not an optimal solution by any means, but we can't make real applications without it.

Comment by David Nolen [ 16/Sep/14 6:46 PM ]

Micha thanks for clarifying the issue further. I see the problem and I think we can do the SSA bit when we don't have arity information.

Comment by David Nolen [ 16/Sep/14 8:36 PM ]

fixed https://github.com/clojure/clojurescript/commit/1f1f56e4f77618b6b51ef9e381a3cb0bd1046705





[CLJS-858] resolve-existing-var does not work for vars from other namespaces Created: 16/Sep/14  Updated: 16/Sep/14  Resolved: 16/Sep/14

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None


 Comments   
Comment by David Nolen [ 16/Sep/14 6:29 PM ]

fixed https://github.com/clojure/clojurescript/commit/877b1ab2e6bb1c09d1988348d6cb384f8ba16414





Generated at Tue Sep 23 09:34:15 CDT 2014 using JIRA 4.4#649-r158309.