<< Back to previous view

[CLJ-2055] binding-form spec parses symbol-only maps incorrectly Created: 08/Nov/16  Updated: 09/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

1.9.0-alpha14


Attachments: Text File CLJ-2055-01.patch    
Patch: Code
Approval: Screened

 Description   

The :clojure.core.specs/binding-form spec incorrectly treats some maps as sequential bindings.

Actual:

user=> (s/conform :clojure.core.specs/binding-form '{x y})
[:seq {:elems [[:seq {:elems [[:sym x] [:sym y]]}]]}]

Expected:

user=> (s/conform :clojure.core.specs/binding-form '{x y})
[:map {x y}]

Cause:

When there is no :keys, :strs, or :syms from :clojure.core.specs/map-special-binding, then :clojure.core.specs/seq-binding-form treats a map as sequential.

Proposed fix:

Include an (s/and vector? ...) check. See patch.

Patch: CLJ-2055-01.patch
Screened by: Alex Miller






[CLJ-2046] generate random subsets of or'd required keys in map specs Created: 17/Oct/16  Updated: 17/Oct/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Enhancement Priority: Major
Reporter: David Chelimsky Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: generator, spec

Attachments: Text File map-spec-gen-enhancements.patch    
Patch: Code and Test
Approval: Screened

 Description   

(s/keys :req [(or ::x ::y)]) always generates maps with both ::x and ::y but it should also generate maps with either ::x or ::y.

The attached patch supports arbitrarily deeply nested or and and expressions within the values of :req and :req-un in map specs. It also uses the same 'or' mechanism for :opt and :opt-un keys, thereby replacing the use of clojure.core/shuffle with clojure.test.check.generators/shuffle, ensuring repeatability of the generators.

Patch: map-spec-gen-enhancements.patch

Screened by: Alex Miller






[CLJ-2043] s/form of conformer is broken Created: 14/Oct/16  Updated: 14/Oct/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: spec

Attachments: Text File clj-2043.patch    
Patch: Code
Approval: Screened

 Description   

s/form of s/conformer is wrong:

(s/form (s/conformer str))
=> str

Proposed: Fix the form for conformer to match the conformer call.

Patch: clj-2043.patch






[CLJ-2035] Bad s/form for collection specs Created: 07/Oct/16  Updated: 14/Oct/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Johan Gall Assignee: Alex Miller
Resolution: Unresolved Votes: 4
Labels: spec

Attachments: Text File clj-2035-2.patch     Text File clj-2035.patch    
Patch: Code and Test
Approval: Screened

 Description   

There are several problems with s/form for collection specs (coll-of,map-of,every,every-kv):

1. coll spec forms expose implementation details of building on every:

(s/form (s/coll-of int?))
=> (clojure.spec/every int? :clojure.spec/cpred #object[user$eval16$fn__18 0xd506900 "user$eval16$fn__18@d506900"] :clojure.spec/kind-form nil :clojure.spec/conform-all true)

2. form does not resolve nested spec preds:

(s/def ::a (s/every (s/tuple ::b)))

(s/form ::a)
=> (clojure.spec/every (*s/tuple* :user/b) [ ... ])

(which impacts map-of and coll-of).

3. :kind fn is not resolved

(s/form (s/coll-of int? :kind vector?))
=> (clojure.spec/every int? :clojure.spec/cpred #object[user$eval4$fn__6 0x8fc095 "user$eval4$fn__6@8fc095"] :clojure.spec/kind-form clojure.core/vector? :kind #object[clojure.core$vector_QMARK___6428 0x6596f6ef "clojure.core$vector_QMARK___6428@6596f6ef"] :clojure.spec/conform-all true)

Ignoring the rest of the problems from #1, the :kind should be here but should be the resolved form (clojure.core/vector?).

Patch: clj-2035-2.patch






[CLJ-2030] Auto-create alias namespaces Created: 28/Sep/16  Updated: 06/Dec/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.9

Type: Enhancement Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: keywords, namespace, portability, spec

Attachments: Text File clj-2030-2.patch     Text File clj-2030-3-1.patch     Text File clj-2030-3-2.patch     Text File clj-2030.patch    
Patch: Code and Test
Approval: Screened

 Description   

It is useful to name keywords in namespaces, without creating or requiring those namespaces. When wanting to do that via an ::alias/keyword, the aliased namespace has to actually exist, in order to be aliased.
Currently, in Clojure, this can be achieved dynamically, through a combination of create-ns and alias, Clojurescript requires a dummy file and a :require :as.

Proposals:

1. Extend clojure.core/alias to auto-create missing namespaces
2. Extend clojure.core/alias to accept varargs & {:as kvs}
3. Extend ns to accept (:alias ...) clauses

Patch:

  • clj-2030.patch does 1 (but not 2 or 3), and was screened by SDH
  • clj-2030-2.patch does 1+2 (but not 3)
  • clj-2030-3-1.patch does 1+3 (but not 2)
  • clj-2030-3-2.patch does 1+2+3

Before:

user=> (alias 'parts 'company.domain.parts)
java.lang.Exception: No namespace: company.domain.parts found

After:

user=> (alias 'parts 'company.domain.parts)
nil
user=> ::parts/widget
:company.domain.parts/widget


 Comments   
Comment by Alex Miller [ 28/Sep/16 10:04 PM ]

From original description:

My use case is a simplification of data.xml, which would benefit greatly from a uniform way to alias + auto-create namespaces within the ns clause.

I would like to support the syntax:

(ns foo.bar
  (:alias xh  #xml/ns "http://..<xhtml>.."
          svg #xml/ns "http://..<svg>.."))

{:tag ::xh/div
 :content [{:tag ::svg/g}]}

see https://github.com/bendlas/data.xml/commit/22cbe21181175d302c884b4ec9162bd5ebf336d7

Comment by Alex Miller [ 28/Sep/16 10:08 PM ]

Thanks for filing this, it is something we've looked at a bit already. I simplified the description a bit and moved the use case and syntax to comments. I don't really understand the ns :alias example given in your syntax proposal but I think it's very unlikely we would go that far.

Comment by Herwig Hochleitner [ 29/Sep/16 3:55 AM ]

My syntax example could already be implemented, if alias had the proposed behavior and was available in an ns clause. In the linked commit, I implemented a scheme to encode xml namespaces in clojure namespaces, by using percent-encoding. I could easily provide that reader tag, if clojure and clojurescript provided the proposed extensions to alias and ns.

Comment by Alex Miller [ 29/Sep/16 8:53 AM ]

Yeah, I get it now (sleep!). I think the particular example is distracting to understand the enhancement request though.

Comment by Alex Miller [ 18/Oct/16 9:13 AM ]

moving this to vetted just so we don't lose track of it, but Rich has not actually ok'ed this for 1.9 yet

Comment by Herwig Hochleitner [ 06/Dec/16 10:50 PM ]

added patches for 3





[CLJ-1860] 0.0 and -0.0 compare equal but have different hash values Created: 01/Dec/15  Updated: 17/Nov/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.9

Type: Defect Priority: Minor
Reporter: Patrick O'Brien Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math

Attachments: Text File clj-1860-make-equals-false-for-pos-neg-0.0-v1.patch     Text File CLJ-1860-negative-zero-hash-eq-fix.patch    
Patch: Code and Test
Approval: Screened

 Description   

0.0 and -0.0 compare as equal but have different hash values:

user=> (= 0.0 -0.0)
true
user=> (hash -0.0)
-2147483648
user=> (hash 0.0)
0

This causes problems as the equality/hashing assumption is violated.

user=> #{[1 2 0.0] [1 2 -0.0]}
#{[1 2 -0.0] [1 2 0.0]}

user=> (hash-map 0.0 1 -0.0 2)
{0.0 2}

user=> (hash-map [0.0] 1 [-0.0] 2)
{[0.0] 1, [-0.0] 2}

user=> (array-map [0.0] 1 [-0.0] 2)
{[0.0] 2}

user=> (hash-set [0.0] [-0.0])
#{[0.0] [-0.0]}

Cause: The source of this is due to some differences in Java. Java primitive double 0.0 and -0.0 == but the boxed Double is NOT .equals(). See also: http://docs.oracle.com/javase/7/docs/api/java/lang/Double.html#equals%28java.lang.Object%29

Double equality is checked with == in Clojure, which will report true. Hashing falls through to .hashCode(), which returns different values (but is consistent with the .equals() result on the boxed form).

Approach: While there are times when 0.0 and -0.0 being different are useful (see background below), most Clojure users expect these to compare equal. IEEE 754 says that they should compare as equals as well. So the approach to take here is to leave them as equal but to modify the hash for -0.0 to be the same as 0.0 so that `=` and `hash` are consistent. The attached patch takes this approach.

Patch: CLJ-1860-negative-zero-hash-eq-fix.patch

Screened: Alex Miller

Alternative: Make 0.0 != -0.0. This approach affects a much larger set of code as comparison operators etc may be affected. The patch clj-1860-make-equals-false-for-pos-neg-0.0-v1.patch may be one way to implement this approach, and seems fairly small in the quantity of code affected (2 methods).

Background: https://en.wikipedia.org/wiki/Signed_zero



 Comments   
Comment by Stephen Hopper [ 09/Feb/16 10:45 PM ]

Just to summarize, it seems like this functionality in Clojure is the same as it is in Java:

0.0 == -0.0: true
new Double(0.0).hashCode(): 0
new Double(-0.0).hashCode(): -2147483648
new Double(-0.0).equals(new Double(0.0)): false

I can see pros and cons to both of the aforementioned approaches, as well as just leaving this one be. Does anyone else have any input on this one? Is this issue something we should rectify, or by changing it will we end up creating more problems than we solved?

Comment by Andy Fingerhut [ 10/Feb/16 12:38 AM ]

The Java behavior you demonstrate shows that in this case, they are not equals, so there is no need for the hashCode() values to be the same in order to satisfy the hash consistency property of equals and hashCode.

Clojure currently violates the hash consistency property that should ideally hold between clojure.core/= and clojure.core/hash, for 0.0 and -0.0.

Changing clojure.core/= so it is false would restore the hash consistency property for these values. Keeping (clojure.core/== 0.0 -0.0) true is hopefully something that will be maintained across any change, but that does not violate hash consistency, because that property has nothing to say about the value of clojure.core/==

Comment by Stephen Hopper [ 10/Feb/16 7:10 AM ]

Thanks for the explanation, Andy. That makes sense to me. I'll put together some tests and a possible solution for evaluation.

Comment by Stephen Hopper [ 10/Feb/16 11:20 AM ]

After diving into the source code a bit more, my preference is to modify the hash calculation for -0.0 to be the same as the hash calculation for 0.0. This will restore the hash consistency property without breaking other mathematical operations. Basically, if we update clojure.core/= to return false for (= 0.0 -0.0), we will need to update other functions (clojure.core/<, clojure.core/>, etc.) so that -0.0 and 0.0 still follow basic numerical properties surrounding equality and ordering. I'll add some tests and a possible patch to hash calculation for numbers for consideration.

Comment by Andy Fingerhut [ 10/Feb/16 12:40 PM ]

Someone, perhaps Patrick O'Brien , brought up a preference that making (= 0.0 -0.0) false would help in some applications, e.g. numerical applications involving normal vectors where it was beneficial if (= 0.0 -0.0) was false. I have no knowledge whether this preference will determine what change will be made to Clojure, if any. If someone finds a link to the email discussion that was in one of the Clojure or Clojure Dev Google groups, that would be a useful reference.

Comment by Stephen Hopper [ 11/Feb/16 9:47 PM ]

I've added a pair of patches for review on this one (one contains test updates and the other contains my proposed updates to the hash calculation). I realize that this is different than the approach proposed earlier in the ticket, but I think it should be the preferred approach. As I mentioned earlier, merely changing clojure.core/= to return false for (= 0.0 -0.0) would require also updating other numeric equality functions (clojure.core/<, clojure.core/>, etc.). More importantly, I feel that this behavior would be different than what most Clojure developers would expect.

For this reason, the patches I've updated merely modify the calculation of hashes with Numbers.java for Floats and Doubles for which isZero returns true to return the hashCode for positive 0.0 instead of negative 0.0.

Is this acceptable?

EDIT:
With these patches applied, we get the following behavior in the REPL:

user=> (= 0.0 -0.0)
true
user=> (hash 0.0)
0
user=> (hash -0.0)     
0
user=> (hash (float 0.0))
0
user=> (hash (float -0.0))
0
Comment by Andy Fingerhut [ 12/Feb/16 3:48 AM ]

Stephen, please see here http://dev.clojure.org/display/community/Developing+Patches for the commands used to create patches in the desired format.

Only a screener or Rich can say whether the patch is acceptable in the ways that matter for committing into Clojure.

Comment by Stephen Hopper [ 12/Feb/16 7:42 AM ]

I've corrected the format on my patch files and resubmitted them as one patch. Let me know if you see any issues with this. Also, it's worth pointing out that the first two patch files can be ignored / deleted.

Comment by Andy Fingerhut [ 14/Feb/16 1:30 PM ]

There are also instructions on that page for deleting old attachments, in the section titled "Removing patches", if you wished to do that.

A very minor comment, as the email address you use in your patches is completely up to you (as far as I know), but the one you have in your patch doesn't look like one that others could use to send you a message. If that was intentional on your part, no problem.

Comment by Stephen Hopper [ 14/Feb/16 1:48 PM ]

I've corrected the email address snafoo and removed the outdated patches. Thanks for walking me through this stuff, Andy.

Comment by Steve Miner [ 23/Feb/16 3:43 PM ]

The suggested fix is to make (hash -0.0) return the same as (hash 0.0). With the proposed patch, both will return 0. That happens to be the same as (hash 0). Not wrong at all, but maybe slightly less good than something else. Since you're making a change anyway, why not go the other way and use the -0.0 case as the common result?

I'm thinking that it would be a useful property if the hash of a long N is not equal to the hash of the corresponding (double N). In Clojure 1.8, zero is the only value I could quickly find where the long and the double equivalents have the same hash value.

The patch could be slightly tweaked to make (hash 0.0) and (hash -0.0)

return new Double(-0.0).hashCode()

The only change to the patch is to add the negative sign. The new hash result is -2147483648.

Admittedly this is an edge case, not a real performance issue. People probably don't mix longs and doubles in sets anyway. On the other hand, zeroes are kind of common. Since you're proposing a change, I thought it's worth considering a slight tweak.

Comment by Steve Miner [ 23/Feb/16 3:46 PM ]

Same for the float case, of course.

Comment by Alex Miller [ 24/Feb/16 1:36 PM ]

Looking at this agin, I'm ok with the approach and agree it's definitely a smaller change. Few additional changes needed in the patch and then I'll move it along:

  • Rather than `new Double(0.0).hashCode()` and `new Float(0.0).hashCode()`, move those values into `private static final int` constants and just return them.
  • In the comparison tests, throw -0.0M in there as well
  • Squash the patch into a single commit

Re Steve's suggestion, I do not think it's critical that long and double 0 hash differently and would prefer that double hashes match Java hashCode, so I would veto that suggestion.

Comment by Stephen Hopper [ 24/Feb/16 1:52 PM ]

Thank you, Alex. I'll make those updates.

Comment by Stephen Hopper [ 24/Feb/16 2:43 PM ]

Alex, I've attached the new patch file (CLJ-1860-negative-zero-hash-eq-fix.patch) to address the points from your previous post. Let me know if I missed anything.

Thanks!

Comment by Mike Anderson [ 25/Feb/16 7:44 PM ]

I may be late to the party since I have only just seen this, but I have a strong belief that 0.0 and -0.0 should be == but not =.

Reasons:

  • Anyone doing numerical comparison should use ==, so you want the result to be true
  • Anyone doing value comparison should use =, so you want the result to be false because these are different IEE784 double values. This includes set membership tests etc.

i.e. the Java code is doing it right, and we should be consistent with this.

Comment by Alex Miller [ 25/Feb/16 11:52 PM ]

I think there is consensus that == should be true, so we can set that aside and focus on =.

The reality is that there is no easy way to compare two collections with == (for example comparing [5.0 0.0 1.0] and [5.0 -0.0 1.0]). This is an actual use case that has been problematic for multiple people. While I grant there are use cases where 0.0 and -0.0 are usefully differentiable, I do not know of a real case in the community where this is the desired behavior, so I would rather err on the side of satisfying the intuition of the larger (and currently affected) population.

Also note that the Java code is doing it BOTH ways (primitive doubles are equal, boxed doubles are not), so I think that's a weak argument.

Comment by Alex Miller [ 26/Feb/16 8:44 AM ]

(after sleeping more on this...) It's possible that a better answer here is to expand what can be done with ==. I trust that when Rich looks at this ticket he will have his opinions which may or may not match up to mine and if so, we'll go in a different direction.

Comment by Andy Fingerhut [ 27/Feb/16 10:15 AM ]

Attachment clj-1860-make-equals-false-for-pos-neg-0.0-v1.patch dated Feb 27 2016 is a first cut at implementing a change where = returns false when comparing positive and negative 0.0, float or double.

As far as I can tell, there is no notion of positive and negative 0 for BigDecimal, so no change in behavior there.

Comment by Alex Miller [ 17/Nov/16 4:16 PM ]

Rich says: "0.0=-0.0, make hash the same"





[CLJ-1793] Reducer instances hold onto the head of seqs Created: 05/Aug/15  Updated: 21/Sep/16

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: Release 1.9

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 4
Labels: compiler
Environment:

1.8.0-alpha2 - 1.8.0-alpha4


Attachments: Text File 0001-Clear-this-before-calls-in-tail-position.patch     Text File clj-1793-2.patch     Text File clj-1793-3.patch    
Patch: Code and Test
Approval: Screened

 Description   

(This ticket started life as CLJ-1250, was committed in 1.8.0-alpha2, pulled out after alpha4, and this is the new version that fixes the logic about whether in a tail call as well as addresses direct linking added in 1.8.0-alpha3.)

Problem: Original example was with reducers holding onto the head of a lazy seq:

(time (reduce + 0 (map identity (range 1e8))))    ;; works
(time (reduce + 0 (r/map identity (range 1e8))))  ;; oome from holding head of range

Trickier example from CLJ-1250 that doesn't clear `this` in nested loop:

(let [done (atom false)
      f (future-call
          (fn inner []
            (while (not @done)
              (loop [found []]
                (println (conj found 1))))))]
  (doseq [elem [:a :b :c :done]]
    (println "queue write " elem))
  (reset! done true)
  @f)

Problem: #'reducer closes over a collection in order to reify CollReduce, and the closed-over collection reference is never cleared. When code attempts to reduce over this anonymous transformed collection, it will realize the tail while the head is stored in the closed-over.

Approach: When invoking a method in a tail call, clear 'this' prior to invoking.

The criteria for when a tail call is a safe point to clear 'this':

1) Must be in return position
2) Not in a try block (might need 'this' during catch/finally)
3) Not direct linked

Return position (#1) isn't simply (context == C.RETURN) because loop bodies are always parsed in C.RETURN context

A new dynvar METHOD_RETURN_CONTEXT tracks whether an InvokeExpr in tail position can directly leave the body of the compiled java method. It is set to RT.T in the outermost parsing of a method body and invalidated (set to null) when a loop body is being parsed where the context for the loop expression is not RETURN parsed. Added clear in StaticInvokeExpr as that is now a thing with direct linking again.

Removes calls to emitClearLocals(), which were a no-op.

Patch: clj-1793-3.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 05/Aug/15 12:16 PM ]

The this ref is cleared prior to the println, but the next time through the while loop it needs the this ref to look up the closed over done field (via getfield).

Adding an additional check to the inTailCall() method to not include tail call in a loop addresses this case:

static boolean inTailCall(C context) {
-    return (context == C.RETURN) && (IN_TRY_BLOCK.deref() == null);
+    return (context == C.RETURN) && (IN_TRY_BLOCK.deref() == null) && (LOOP_LOCALS.deref() == null);
}

But want to check some more things before concluding that's all that's needed.

Comment by Alex Miller [ 05/Aug/15 1:36 PM ]

This change undoes the desired behavior in the original CLJ-1250 (new tests don't pass). For now, we are reverting the CLJ-1250 patch in master.

Comment by Ghadi Shayban [ 05/Aug/15 3:12 PM ]

Loop exit edges are erroneously being identified as places to clear 'this'. Only exits in the function itself or the outermost loop are safe places to clear.

Comment by Ghadi Shayban [ 05/Aug/15 8:43 PM ]

Patch addresses this bug and the regression in CLJ-1250.

See the commit message for an extensive-ish comment.

Comment by Alex Miller [ 18/Aug/15 12:33 PM ]

New patch is same as old, just adds jira id to beginning of commit message.

Comment by Rich Hickey [ 24/Aug/15 10:00 AM ]

Not doing this for 1.8, more thought needs to go into whether this is the right solution to the problem. And, what is the problem? This title of this patch is just something to do.

Comment by Alex Miller [ 24/Aug/15 10:21 AM ]

changing to vetted so this is at a valid place in the jira workflow

Comment by Ghadi Shayban [ 24/Aug/15 10:45 AM ]

Rich the original context is in CLJ-1250 which was a defect/problem. It was merged and revert because of a problem in the impl. This ticket is the continuation of the previous one, but unfortunately the title lost the context and became approach-oriented and not problem-oriented. Blame Alex. (I kid, it's an artifact of the mutable approach to issue management.)

Comment by Nicola Mometto [ 23/Mar/16 7:34 AM ]

Just a note that the original ticket for this issue had 10 votes

Comment by Nicola Mometto [ 30/Mar/16 8:50 AM ]

The following code currently eventually causes an OOM to happen, the patch in this ticket correctly helps not holding onto the collection and doesn't cause memory to run infinitely

Before patch:

user=> (defn range* [x] (cons x (lazy-seq (range* (inc x)))))
#'user/range*
user=> (reduce + 0 (eduction (range* 0)))
OutOfMemoryError Java heap space  clojure.lang.RT.cons (RT.java:660)

After patch:

user=> (defn range* [x] (cons x (lazy-seq (range* (inc x)))))
#'user/range*
user=> (reduce + 0 (eduction (range* 0)))
;; runs infinitely without causing OOM
Comment by Alex Miller [ 08/Sep/16 5:14 PM ]

Refreshed patch to apply to master. No semantic changes, attribution retained.





Generated at Sun Dec 11 02:10:11 CST 2016 using JIRA 4.4#649-r158309.