Clojure

Field access via .- in reflective case does not work

Details

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

Description

The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user=> (. ^T t a)  ;; as expected (prefer method)
"method"
user=> (. ^T t -a) ;; as expected (prefer field)
"field"
user> (. t a)      ;; as expected (prefer method)
"method"
user> (. t -a)     ;; WRONG - should return "field"
"method"

Approach: This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). InstanceFieldExpr now takes another flag (requireField) which will be set to true if "-field" and false if "field". InstanceFieldExpr will invoke (or emit) a call to Reflector.invokeNoArgInstanceMember() which now takes the same flag. If the flag is set to true, it first looks only for a field, otherwise it looks for a method and falls back to field which throws an error if necessary. I added a new invokeNoArgInstanceMember() with an arity to match the old arity - existing bytecode compiled on older Clojure versions will be trying to call this arity.

Patch: clj-1363-v3.patch

Screened by:

  1. clj-1363-v1.patch
    20/Feb/14 10:40 AM
    2 kB
    Alex Miller
  2. clj-1363-v2.patch
    21/Feb/14 5:41 AM
    5 kB
    Alex Miller
  3. clj-1363-v3.patch
    27/Feb/14 8:24 AM
    5 kB
    Alex Miller

Activity

Alex Miller made changes -
Field Original Value New Value
Approval Vetted [ 10003 ]
Fix Version/s Release 1.6 [ 10157 ]
Assignee Alex Miller [ alexmiller ]
Labels interop
Alex Miller made changes -
Description The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

{code}
user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user> (. t (a)) ;; works
"method"
user> (.-a t) ;; should return "field"
"method"
{code}


Alex Miller made changes -
Patch Code and Test [ 10002 ]
Description The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

{code}
user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user> (. t (a)) ;; works
"method"
user> (.-a t) ;; should return "field"
"method"
{code}


The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

{code}
user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user> (. t (a)) ;; works
"method"
user> (.-a t) ;; should return "field"
"method"
{code}

*Approach:* This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). I added a case prior to the method check that looks for a field and invokes it instead if found, flipping the preference. If neither is found, it will back into the field case and emit the same error it did before. This is the simplest change I could make to invokeNoArgInstanceMember(), but it does reflectively look up the field twice (once via getField and once in getInstanceField). It would be possible either refactor one of those methods or copy logic from getInstanceField into invokeNoArgInstanceMember() to remove the double look-up but those are all bigger changes. I added a test that uses gen-class to create an object with same named field and method.

The non-reflective case is already handled in the compiler and will already prefer the field over the method.

*Patch:* clj-1363-v1.patch

*Screened by:*
Attachment clj-1363-v1.patch [ 12832 ]
Alex Miller made changes -
Assignee Alex Miller [ alexmiller ]
Stuart Halloway made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

{code}
user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user> (. t (a)) ;; works
"method"
user> (.-a t) ;; should return "field"
"method"
{code}

*Approach:* This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). I added a case prior to the method check that looks for a field and invokes it instead if found, flipping the preference. If neither is found, it will back into the field case and emit the same error it did before. This is the simplest change I could make to invokeNoArgInstanceMember(), but it does reflectively look up the field twice (once via getField and once in getInstanceField). It would be possible either refactor one of those methods or copy logic from getInstanceField into invokeNoArgInstanceMember() to remove the double look-up but those are all bigger changes. I added a test that uses gen-class to create an object with same named field and method.

The non-reflective case is already handled in the compiler and will already prefer the field over the method.

*Patch:* clj-1363-v1.patch

*Screened by:*
The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

{code}
user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user> (. t (a)) ;; works
"method"
user> (.-a t) ;; should return "field"
"method"
{code}

*Approach:* This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). I added a case prior to the method check that looks for a field and invokes it instead if found, flipping the preference. If neither is found, it will back into the field case and emit the same error it did before. This is the simplest change I could make to invokeNoArgInstanceMember(), but it does reflectively look up the field twice (once via getField and once in getInstanceField). It would be possible either refactor one of those methods or copy logic from getInstanceField into invokeNoArgInstanceMember() to remove the double look-up but those are all bigger changes. I added a test that uses gen-class to create an object with same named field and method.

The non-reflective case is already handled in the compiler and will already prefer the field over the method.

*Patch:* clj-1363-v1.patch

*Screened by:* Stuart Halloway. The patch looks correct, but I am concerned about perf and would be willing to refactor to eliminate the double lookup.
Rich Hickey made changes -
Approval Screened [ 10004 ] Incomplete [ 10006 ]
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Description The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

{code}
user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user> (. t (a)) ;; works
"method"
user> (.-a t) ;; should return "field"
"method"
{code}

*Approach:* This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). I added a case prior to the method check that looks for a field and invokes it instead if found, flipping the preference. If neither is found, it will back into the field case and emit the same error it did before. This is the simplest change I could make to invokeNoArgInstanceMember(), but it does reflectively look up the field twice (once via getField and once in getInstanceField). It would be possible either refactor one of those methods or copy logic from getInstanceField into invokeNoArgInstanceMember() to remove the double look-up but those are all bigger changes. I added a test that uses gen-class to create an object with same named field and method.

The non-reflective case is already handled in the compiler and will already prefer the field over the method.

*Patch:* clj-1363-v1.patch

*Screened by:* Stuart Halloway. The patch looks correct, but I am concerned about perf and would be willing to refactor to eliminate the double lookup.
The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

{code}
user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user=> (. ^T t a) ;; as expected (prefer method)
"method"
user=> (. ^T t -a) ;; as expected (prefer field)
"field"
user> (. t a) ;; as expected (prefer method)
"method"
user> (. t -a) ;; WRONG - should return "field"
"method"
{code}

*Approach:* This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). InstanceFieldExpr now takes another flag (fieldPreference) which will be set to true if "-field" and false if "field". InstanceFieldExpr will invoke (or emit) a call to Reflector.invokeNoArgInstanceMember() which now takes the same flag. If the flag is set to true, it first looks for a field, otherwise it looks for a method, finally it will fall back to field which throws an error if necessary. I added a new invokeNoArgInstanceMember() with an arity to match the old arity - existing bytecode compiled on older Clojure versions will be trying to call this arity.

In the case of a reflective field lookup on a dashed field when the field does not exist, we will now look up the field twice (by traversing all fields). This seems like a rare enough case (and an error at that), that it is not worth refactoring the various getField and getInstanceField methods in Reflector.

*Patch:* clj-1363-v2.patch

*Screened by:*
Alex Miller made changes -
Attachment clj-1363-v2.patch [ 12836 ]
Stuart Halloway made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Alex Miller made changes -
Description The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

{code}
user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user=> (. ^T t a) ;; as expected (prefer method)
"method"
user=> (. ^T t -a) ;; as expected (prefer field)
"field"
user> (. t a) ;; as expected (prefer method)
"method"
user> (. t -a) ;; WRONG - should return "field"
"method"
{code}

*Approach:* This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). InstanceFieldExpr now takes another flag (fieldPreference) which will be set to true if "-field" and false if "field". InstanceFieldExpr will invoke (or emit) a call to Reflector.invokeNoArgInstanceMember() which now takes the same flag. If the flag is set to true, it first looks for a field, otherwise it looks for a method, finally it will fall back to field which throws an error if necessary. I added a new invokeNoArgInstanceMember() with an arity to match the old arity - existing bytecode compiled on older Clojure versions will be trying to call this arity.

In the case of a reflective field lookup on a dashed field when the field does not exist, we will now look up the field twice (by traversing all fields). This seems like a rare enough case (and an error at that), that it is not worth refactoring the various getField and getInstanceField methods in Reflector.

*Patch:* clj-1363-v2.patch

*Screened by:*
The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

{code}
user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user=> (. ^T t a) ;; as expected (prefer method)
"method"
user=> (. ^T t -a) ;; as expected (prefer field)
"field"
user> (. t a) ;; as expected (prefer method)
"method"
user> (. t -a) ;; WRONG - should return "field"
"method"
{code}

*Approach:* This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). InstanceFieldExpr now takes another flag (requireField) which will be set to true if "-field" and false if "field". InstanceFieldExpr will invoke (or emit) a call to Reflector.invokeNoArgInstanceMember() which now takes the same flag. If the flag is set to true, it first looks only for a field, otherwise it looks for a method and falls back to field which throws an error if necessary. I added a new invokeNoArgInstanceMember() with an arity to match the old arity - existing bytecode compiled on older Clojure versions will be trying to call this arity.

*Patch:* clj-1363-v3.patch

*Screened by:*
Alex Miller made changes -
Attachment clj-1363-v3.patch [ 12849 ]
Alex Miller made changes -
Resolution Completed [ 1 ]
Approval Screened [ 10004 ] Ok [ 10007 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: