Clojure

when unable to match a method, report arity caller was looking for

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.4
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Vetted

Description

Original motivation: Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

user=> (Long/parseLong)
CompilerException java.lang.NoSuchFieldException: parseLong, compiling:(NO_SOURCE_PATH:1:1) 
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: No matching method: parseLong, compiling:(NO_SOURCE_PATH:2:1)

Incorrectly invoking an instance method with 0 parameters yields a message about fields as well:

user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching field found: setTime for class java.util.Date  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Patch: clj-1130-v5.diff

Approach: Primum non nocere. Error reporting enhanced at the site the errors happen, compiler logic unchanged.

  1. clj-1130-v1.txt
    09/Sep/13 12:36 AM
    4 kB
    Andy Fingerhut
  2. clj-1130-v2.txt
    04/Oct/13 3:56 PM
    4 kB
    Alex Miller
  3. clj-1130-v2.diff
    22/Oct/13 9:11 AM
    4 kB
    Alex Miller
  4. clj-1130-v2-ignore-ws.diff
    04/Nov/13 8:35 AM
    3 kB
    Andy Fingerhut
  5. clj-1130-v3.diff
    06/Dec/13 9:19 PM
    5 kB
    Alex Miller
  6. clj-1130-v4.diff
    31/Jan/14 3:12 PM
    8 kB
    Stuart Halloway
  7. clj-1130-v5.diff
    31/Jan/14 3:18 PM
    4 kB
    Andy Fingerhut

Activity

Hide
Michael Drogalis added a comment -

It looks like it's first trying to resolve a field by name, since field access with / is legal. For example:

user=> (Integer/parseInt)
CompilerException java.lang.NoSuchFieldException: parseInt, compiling:(NO_SOURCE_PATH:1)

user=> (Integer/MAX_VALUE)
2147483647

Would trying to resolve a method before a field fix this?

Show
Michael Drogalis added a comment - It looks like it's first trying to resolve a field by name, since field access with / is legal. For example: user=> (Integer/parseInt) CompilerException java.lang.NoSuchFieldException: parseInt, compiling:(NO_SOURCE_PATH:1) user=> (Integer/MAX_VALUE) 2147483647 Would trying to resolve a method before a field fix this?
Hide
Alex Miller added a comment -

Similarities to CLJ-1248 (there a warning, here an error).

Show
Alex Miller added a comment - Similarities to CLJ-1248 (there a warning, here an error).
Hide
Andy Fingerhut added a comment -

Patch clj-1130-v1.txt changes the error message in a situation when one attempts to invoke a static method with no args, and there is no such 0-arg static method. The message now says that there is no such method with that name and 0 args, rather than that there is no such static field with that name.

Show
Andy Fingerhut added a comment - Patch clj-1130-v1.txt changes the error message in a situation when one attempts to invoke a static method with no args, and there is no such 0-arg static method. The message now says that there is no such method with that name and 0 args, rather than that there is no such static field with that name.
Hide
Alex Miller added a comment -

I updated the patch to simplify it a bit but more importantly to remove the check by exception and instead use the Reflector method that can answer this question.

Show
Alex Miller added a comment - I updated the patch to simplify it a bit but more importantly to remove the check by exception and instead use the Reflector method that can answer this question.
Hide
Andy Fingerhut added a comment -

Alex, thank you for the improvements to the code. It looks better to me.

Show
Andy Fingerhut added a comment - Alex, thank you for the improvements to the code. It looks better to me.
Hide
Rich Hickey added a comment -

due to indentation changes, this patch appears to touch much more than it probably does, making it difficult to approve.

Show
Rich Hickey added a comment - due to indentation changes, this patch appears to touch much more than it probably does, making it difficult to approve.
Hide
Andy Fingerhut added a comment -

Any suggestions on what can be done to make progress here? Would it help to attach a patch made with "-w" option to ignore lines that differ only in whitespace? Provide git diff command line options that do this, after the patch is applied to your local workspace? Make a patch that leaves the indentation 'incorrect' after the change (involuntary shudder)?

Show
Andy Fingerhut added a comment - Any suggestions on what can be done to make progress here? Would it help to attach a patch made with "-w" option to ignore lines that differ only in whitespace? Provide git diff command line options that do this, after the patch is applied to your local workspace? Make a patch that leaves the indentation 'incorrect' after the change (involuntary shudder)?
Hide
Alex Miller added a comment -

The indentation has intentionally changed because the if/else structure has changed. I don't think making the patch incorrect to reduce changes is a good idea.

Show
Alex Miller added a comment - The indentation has intentionally changed because the if/else structure has changed. I don't think making the patch incorrect to reduce changes is a good idea.
Hide
Andy Fingerhut added a comment -

Well, the 'incorrect' was in quotes because I was asking about a proposed patch that had the correct logic, but misleading indentation. Agreed it isn't a good idea, hence the shudder. I'm just brainstorming ideas to make the patch less difficult to approve.

Show
Andy Fingerhut added a comment - Well, the 'incorrect' was in quotes because I was asking about a proposed patch that had the correct logic, but misleading indentation. Agreed it isn't a good idea, hence the shudder. I'm just brainstorming ideas to make the patch less difficult to approve.
Hide
Howard Lewis Ship added a comment -

At some point, you may need to bite the bullet and reformat some of the Clojure code .... Compiler.java had a crazy mix of tabs, spaces, and just completely wrong stuff.

Show
Howard Lewis Ship added a comment - At some point, you may need to bite the bullet and reformat some of the Clojure code .... Compiler.java had a crazy mix of tabs, spaces, and just completely wrong stuff.
Hide
Alex Miller added a comment -

Re-marking screened. Not sure what else to do.

Show
Alex Miller added a comment - Re-marking screened. Not sure what else to do.
Hide
Andy Fingerhut added a comment -

clj-1130-v2-ignore-ws.diff is identical to clj-1130-v2.diff, except it was produced with a command that ignores differences in a line due only to whitespace, i.e.: 'git format-patch master --stdout -w > clj-1130-v2-ignore-ws.diff'

It is not intended as the patch to be applied. It is only intended to make it easier to see that many of the lines in clj-1130-v2.diff are truly only differences in indentation.

Show
Andy Fingerhut added a comment - clj-1130-v2-ignore-ws.diff is identical to clj-1130-v2.diff, except it was produced with a command that ignores differences in a line due only to whitespace, i.e.: 'git format-patch master --stdout -w > clj-1130-v2-ignore-ws.diff' It is not intended as the patch to be applied. It is only intended to make it easier to see that many of the lines in clj-1130-v2.diff are truly only differences in indentation.
Hide
Alex Miller added a comment -

Thanks Andy...

Show
Alex Miller added a comment - Thanks Andy...
Hide
Rich Hickey added a comment -

This patch ignores the fact that method is checked for first above:

if(c != null)
  maybeField = Reflector.getMethods(c, 0, munge(sym.name), true).size() == 0;

Which is why the field code is unconditional. I'm fine with making errors better, but changing logic as well deserves more scrutiny.

Show
Rich Hickey added a comment - This patch ignores the fact that method is checked for first above:
if(c != null)
  maybeField = Reflector.getMethods(c, 0, munge(sym.name), true).size() == 0;
Which is why the field code is unconditional. I'm fine with making errors better, but changing logic as well deserves more scrutiny.
Hide
Alex Miller added a comment -

This patch is intentionally trying to avoid calling StaticFieldExpr in the field code as that is where the (Long/parseLong) case (erroneously calling an n-arity static method with 0 args) will throw a field-oriented exception instead of a method-oriented exception. By adding the extra check here, this case falls through into the method case and throws later on calling StaticMethodExpr instead.

The early check is a check for methods of the specified arity. The later check is for the existence of a field of matching name. Combined, they lead to a better error message.

However, another alternative is to set maybeField in the first check based on field existence, not on invocation arity. That just improves the maybeField informaiton and the existing code then naturally throws the correct exception (and the patch is much simpler).

The similar case for calling n-arity instance methods with 0-arity has the same problem for the same reason:

user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching field found: setTime for class java.util.Date  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Thus we can also adjust the other call that sets maybeField (which now is much less maybe).

I will attach a patch that covers these cases and update the ticket for someone to screen.

Show
Alex Miller added a comment - This patch is intentionally trying to avoid calling StaticFieldExpr in the field code as that is where the (Long/parseLong) case (erroneously calling an n-arity static method with 0 args) will throw a field-oriented exception instead of a method-oriented exception. By adding the extra check here, this case falls through into the method case and throws later on calling StaticMethodExpr instead. The early check is a check for methods of the specified arity. The later check is for the existence of a field of matching name. Combined, they lead to a better error message. However, another alternative is to set maybeField in the first check based on field existence, not on invocation arity. That just improves the maybeField informaiton and the existing code then naturally throws the correct exception (and the patch is much simpler). The similar case for calling n-arity instance methods with 0-arity has the same problem for the same reason:
user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching field found: setTime for class java.util.Date  clojure.lang.Reflector.getInstanceField (Reflector.java:271)
Thus we can also adjust the other call that sets maybeField (which now is much less maybe). I will attach a patch that covers these cases and update the ticket for someone to screen.
Hide
Stuart Sierra added a comment -

Screened. The patch clj-1130-v3.diff works as advertised.

This patch only improves error messages for cases when the type of the
target object is known to the compiler. In reflective calls, the error
messages are still the same.

Example, after this patch, given these definitions:

(def v 42)
(defn untagged-f [] 42)
(defn ^Long tagged-f [] 42)

The following expressions produce new error messages:

(.foo v 1)
;; IllegalArgumentException No matching method found: foo taking 1 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

(.foo (tagged-f))
;; IllegalArgumentException No matching method found: foo taking 0 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

These expressions still use the old error messages:

(.foo v)
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)

(.foo (untagged-f))
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)
Show
Stuart Sierra added a comment - Screened. The patch clj-1130-v3.diff works as advertised. This patch only improves error messages for cases when the type of the target object is known to the compiler. In reflective calls, the error messages are still the same. Example, after this patch, given these definitions:
(def v 42)
(defn untagged-f [] 42)
(defn ^Long tagged-f [] 42)
The following expressions produce new error messages:
(.foo v 1)
;; IllegalArgumentException No matching method found: foo taking 1 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

(.foo (tagged-f))
;; IllegalArgumentException No matching method found: foo taking 0 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)
These expressions still use the old error messages:
(.foo v)
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)

(.foo (untagged-f))
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)
Hide
Rich Hickey added a comment -

Changing the logic to get a different error message is something that needs to be done with great care. This now seems to prefer fields over methods, changing the semantics.

Show
Rich Hickey added a comment - Changing the logic to get a different error message is something that needs to be done with great care. This now seems to prefer fields over methods, changing the semantics.
Hide
Stuart Halloway added a comment -

v4 patch simply enhances error messaages

Show
Stuart Halloway added a comment - v4 patch simply enhances error messaages
Hide
Andy Fingerhut added a comment -

clj-1130-v4.diff has the same patch repeated twice in the file. clj-1130-v5.diff is identical, except deleting the redundant copy.

Show
Andy Fingerhut added a comment - clj-1130-v4.diff has the same patch repeated twice in the file. clj-1130-v5.diff is identical, except deleting the redundant copy.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated: