Clojure

Compiler macro for clojure.core/instance? disregards lexical shadows on class names

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Summary

The compiler tries to emit jvm native instanceof expressions for direct clojure.core/instance? calls.
For that, it tries to resolve its first argument as a class name. However, it disregards lexical bindings when doing that.
This is incongruent to the default implementation in core.clj

Patches

[Stu] All three patches should be applied IMO.

  • 0002 makes instance? respect lexical bindings
  • 0003 makes instance?'s compiled form check arity, consistent with higher-order behavior
  • 0001 has a minimal test for both 0002 and 0003.

Data

Test case

user=> (let [Long String] (instance? Long "abc"))
false
;; expected true as in
user=> (let [Long String] (apply instance? [Long "abc"]))
true

Culprit method

https://github.com/clojure/clojure/blob/4ccb10edbe66eae81336a4c0972050d9759b0bf7/src/jvm/clojure/lang/Compiler.java#L3578

List Discussion

https://groups.google.com/d/topic/clojure/mf25OlFRpa8/discussion

Tangent

This was discovered because the same compiler macro also omits the arity check implicit in the default definition. This could also conveniently be fixed when touching that method:

user=> (instance? String)
false
;; expected
user=> (apply instance? [String])
ArityException Wrong number of args (1) passed to: core$instance-QMARK-  clojure.lang.AFn.throwArity (AFn.java:437)

EDIT elaborated on ticket title and description; added tangent

Activity

Herwig Hochleitner made changes -
Field Original Value New Value
Description The compiler tries to emit instanceof expressions for instance? calls.
For that, it tries to resolve its first argument as a class name. However, it disregards lexical bindings when doing that.

Test case:
{code}
user=> (let [Long String] (instance? Long "abc"))
false
{code}

Culprit:
https://github.com/clojure/clojure/blob/4ccb10edbe66eae81336a4c0972050d9759b0bf7/src/jvm/clojure/lang/Compiler.java#L3578

Discussion:
https://groups.google.com/d/topic/clojure/mf25OlFRpa8/discussion
h1. Summary
The compiler tries to emit jvm native *instanceof* expressions for direct *clojure.core/instance?* calls.
For that, it tries to resolve its first argument as a class name. However, it disregards lexical bindings when doing that.
This is incongruent to the default implementation in core.clj

h1. Data

h2. Test case
{code}
user=> (let [Long String] (instance? Long "abc"))
false
;; expected true as in
user=> (let [Long String] (apply instance? [Long "abc"]))
true
{code}

h2. Culprit method
https://github.com/clojure/clojure/blob/4ccb10edbe66eae81336a4c0972050d9759b0bf7/src/jvm/clojure/lang/Compiler.java#L3578

h2. List Discussion
https://groups.google.com/d/topic/clojure/mf25OlFRpa8/discussion

h1. Tangent
This was discovered because the same compiler macro also omits the arity check implicit in the default definition. This could also conveniently be fixed when touching that method:

{code}
user=> (instance? String)
false
;; expected
user=> (apply instance? [String])
ArityException Wrong number of args (1) passed to: core$instance-QMARK- clojure.lang.AFn.throwArity (AFn.java:437)
{code}

{quote}
*EDIT* elaborated on ticket title and description; added tangent
{quote}
Summary Direct calls to instance? disregard shadowed class names Compiler macro for clojure.core/instance? disregards lexical shadows on class names
Hide
Herwig Hochleitner added a comment -

Attached patches test and fix issue + tangent

Show
Herwig Hochleitner added a comment - Attached patches test and fix issue + tangent
Hide
Herwig Hochleitner added a comment - - edited

Note: Patch 0003 just adds the arity check, hence is optional, but if it's omitted from the patchset, the corresponding test from patch 0001 will fail.

Show
Herwig Hochleitner added a comment - - edited Note: Patch 0003 just adds the arity check, hence is optional, but if it's omitted from the patchset, the corresponding test from patch 0001 will fail.
Andy Fingerhut made changes -
Patch Code and Test [ 10002 ]
Hide
Stuart Halloway added a comment -

Summarizing the decisions in these patches:

  • instance? and apply instance? should be consistent
  • they should check arity (matching apply instance? existing behavior)
  • they should allow lexical shadowing of the class argument (matching apply instance? existing behavior)

It is possible (although unlikely) that existing code relies on the current eccentric behavior of instance?. I think it would be fair to categorize programs relying on this behavior as buggy, but that is easy for me to say.

Show
Stuart Halloway added a comment - Summarizing the decisions in these patches:
  • instance? and apply instance? should be consistent
  • they should check arity (matching apply instance? existing behavior)
  • they should allow lexical shadowing of the class argument (matching apply instance? existing behavior)
It is possible (although unlikely) that existing code relies on the current eccentric behavior of instance?. I think it would be fair to categorize programs relying on this behavior as buggy, but that is easy for me to say.
Stuart Halloway made changes -
Approval Triaged [ 10120 ]
Stuart Halloway made changes -
Comment [ Do you think you're dealing with a lot of economic related difficulties? If yes how to overcome these individuals properly is not known to your account while you possess eliminated out of fund. In such circumstance you can not head over to your current family members and enquire for cash for anyone who is working someplace. In their normal time period this will become noticeable that you should hunt for many trustworthy method to obtain dollars from and acquire cash aid devoid of proceeding long distance and regarding throughout lengthy means of acceptance. http://www.online12monthloans24h.co.uk/ Next, you shouldn't be concerned whatsoever and test immediate 12 calendar month financial products seeing that below this system you can get the quantity you'll need instantly and for you don't should anyplace just like financial institution or perhaps any office on the loan companies. It is possible to acquire the income through this system on the web and for total one year. ]
Rich Hickey made changes -
Approval Triaged [ 10120 ] Vetted [ 10003 ]
Rich Hickey made changes -
Fix Version/s Release 1.6 [ 10157 ]
Stuart Halloway made changes -
Assignee Stuart Halloway [ stu ]
Stuart Halloway made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description h1. Summary
The compiler tries to emit jvm native *instanceof* expressions for direct *clojure.core/instance?* calls.
For that, it tries to resolve its first argument as a class name. However, it disregards lexical bindings when doing that.
This is incongruent to the default implementation in core.clj

h1. Data

h2. Test case
{code}
user=> (let [Long String] (instance? Long "abc"))
false
;; expected true as in
user=> (let [Long String] (apply instance? [Long "abc"]))
true
{code}

h2. Culprit method
https://github.com/clojure/clojure/blob/4ccb10edbe66eae81336a4c0972050d9759b0bf7/src/jvm/clojure/lang/Compiler.java#L3578

h2. List Discussion
https://groups.google.com/d/topic/clojure/mf25OlFRpa8/discussion

h1. Tangent
This was discovered because the same compiler macro also omits the arity check implicit in the default definition. This could also conveniently be fixed when touching that method:

{code}
user=> (instance? String)
false
;; expected
user=> (apply instance? [String])
ArityException Wrong number of args (1) passed to: core$instance-QMARK- clojure.lang.AFn.throwArity (AFn.java:437)
{code}

{quote}
*EDIT* elaborated on ticket title and description; added tangent
{quote}
h1. Summary
The compiler tries to emit jvm native *instanceof* expressions for direct *clojure.core/instance?* calls.
For that, it tries to resolve its first argument as a class name. However, it disregards lexical bindings when doing that.
This is incongruent to the default implementation in core.clj

h2. Patches

[Stu] All three patches should be applied IMO.

* 0002 makes {{instance?}} respect lexical bindings
* 0003 makes {{instance?}}'s compiled form check arity, consistent with higher-order behavior
* 0001 has a minimal test for both 0002 and 0003.

h1. Data

h2. Test case
{code}
user=> (let [Long String] (instance? Long "abc"))
false
;; expected true as in
user=> (let [Long String] (apply instance? [Long "abc"]))
true
{code}

h2. Culprit method
https://github.com/clojure/clojure/blob/4ccb10edbe66eae81336a4c0972050d9759b0bf7/src/jvm/clojure/lang/Compiler.java#L3578

h2. List Discussion
https://groups.google.com/d/topic/clojure/mf25OlFRpa8/discussion

h1. Tangent
This was discovered because the same compiler macro also omits the arity check implicit in the default definition. This could also conveniently be fixed when touching that method:

{code}
user=> (instance? String)
false
;; expected
user=> (apply instance? [String])
ArityException Wrong number of args (1) passed to: core$instance-QMARK- clojure.lang.AFn.throwArity (AFn.java:437)
{code}

{quote}
*EDIT* elaborated on ticket title and description; added tangent
{quote}
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: