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.diff
    22/Oct/13 9:11 AM
    4 kB
    Alex Miller
  3. clj-1130-v2.txt
    04/Oct/13 3:56 PM
    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?
Alex Miller made changes -
Field Original Value New Value
Labels feedback errormsgs
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).
Alex Miller made changes -
Approval Triaged [ 10120 ]
Rich Hickey made changes -
Approval Triaged [ 10120 ] Vetted [ 10003 ]
Fix Version/s Release 1.6 [ 10157 ]
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.
Andy Fingerhut made changes -
Attachment clj-1130-v1.txt [ 12249 ]
Andy Fingerhut made changes -
Patch Code and Test [ 10002 ]
Andy Fingerhut made changes -
Description My code inadventently invoked a method that expected a single parameter, but passed no parameters.

The exception:
{noformat}
Exception in thread "main" java.lang.NoSuchFieldException: getService, compiling:(com/annadaletech/nexus/services/registry.clj:168)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6462)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6443)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.access$100(Compiler.java:37)
at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5883)
....
Caused by: java.lang.NoSuchFieldException: getService
at java.lang.Class.getField(Class.java:1520)
at clojure.lang.Compiler$StaticFieldExpr.<init>(Compiler.java:1180)
at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:923)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
... 78 more
{noformat}

That threw me for a bit; really, it looks like the compiler is attempting to access a field and invoke it as an IFn. However, a proper message would be "getService() requires 1 parameter, not 0" (or something to that effect). Even "invalid number of parameters for SmashRuntime/getService()" would be better.
My code inadventently invoked a method that expected a single parameter, but passed no parameters.

The exception:
{noformat}
Exception in thread "main" java.lang.NoSuchFieldException: getService, compiling:(com/annadaletech/nexus/services/registry.clj:168)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6462)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6443)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.access$100(Compiler.java:37)
at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5883)
....
Caused by: java.lang.NoSuchFieldException: getService
at java.lang.Class.getField(Class.java:1520)
at clojure.lang.Compiler$StaticFieldExpr.<init>(Compiler.java:1180)
at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:923)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
... 78 more
{noformat}

That threw me for a bit; really, it looks like the compiler is attempting to access a field and invoke it as an IFn. However, a proper message would be "getService() requires 1 parameter, not 0" (or something to that effect). Even "invalid number of parameters for SmashRuntime/getService()" would be better.

*Analysis*:

The compiler would guess that any attempt to invoke a static method with 0 args was an attempt to access a static field, which in the (. <class> <method/field-name>) syntax appear identical.

*Patch*: clj-1130-v1.txt

*Approach*: The guess that the form is trying to access a static field is still done the same as before. However, with the patch it will now check for a field with the specified name, and if there is none, the compiler will assume that it is a static method name instead.

The error message for attempting to call a static method now gives more details about the erroneous attempt, including the class name and that there is no such method that takes <n> arguments, where <n> is the number of arguments in the attempted method call.

Before patch:

{code}
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)
{code}

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
{code}
Alex Miller made changes -
Assignee Alex Miller [ alexmiller ]
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.
Alex Miller made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description My code inadventently invoked a method that expected a single parameter, but passed no parameters.

The exception:
{noformat}
Exception in thread "main" java.lang.NoSuchFieldException: getService, compiling:(com/annadaletech/nexus/services/registry.clj:168)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6462)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6443)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.access$100(Compiler.java:37)
at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5883)
....
Caused by: java.lang.NoSuchFieldException: getService
at java.lang.Class.getField(Class.java:1520)
at clojure.lang.Compiler$StaticFieldExpr.<init>(Compiler.java:1180)
at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:923)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
... 78 more
{noformat}

That threw me for a bit; really, it looks like the compiler is attempting to access a field and invoke it as an IFn. However, a proper message would be "getService() requires 1 parameter, not 0" (or something to that effect). Even "invalid number of parameters for SmashRuntime/getService()" would be better.

*Analysis*:

The compiler would guess that any attempt to invoke a static method with 0 args was an attempt to access a static field, which in the (. <class> <method/field-name>) syntax appear identical.

*Patch*: clj-1130-v1.txt

*Approach*: The guess that the form is trying to access a static field is still done the same as before. However, with the patch it will now check for a field with the specified name, and if there is none, the compiler will assume that it is a static method name instead.

The error message for attempting to call a static method now gives more details about the erroneous attempt, including the class name and that there is no such method that takes <n> arguments, where <n> is the number of arguments in the attempted method call.

Before patch:

{code}
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)
{code}

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
{code}
Invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

*Cause:* This particular path in the Compiler covers both static field access and 0 argument static method invocation and in this case results in a confusing exception.

*Patch:* clj-1130-v1.txt

*Approach:* The guess that the form is trying to access a static field is still done the same as before. However, with the patch it will now check for a field with the specified name, and if there is none, the compiler will assume that the user is really trying to invoke a static method.

The error message for attempting to call a static method now gives more details about the erroneous attempt, including the class name and that there is no such method that takes <n> arguments, where <n> is the number of arguments in the attempted method call.

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
{code}

*Screened by:* Alex Miller. Question: Is the performance impact prohibitive to add this reflective check for the existence of a static field when compiling 0-arg calls to static methods or static fields? It would be a bigger change (I think) but it might alternately be possible to do this check only in the case of a failure.
Attachment clj-1130-v2.txt [ 12292 ]
Assignee Alex Miller [ alexmiller ]
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.
Alex Miller made changes -
Description Invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

*Cause:* This particular path in the Compiler covers both static field access and 0 argument static method invocation and in this case results in a confusing exception.

*Patch:* clj-1130-v1.txt

*Approach:* The guess that the form is trying to access a static field is still done the same as before. However, with the patch it will now check for a field with the specified name, and if there is none, the compiler will assume that the user is really trying to invoke a static method.

The error message for attempting to call a static method now gives more details about the erroneous attempt, including the class name and that there is no such method that takes <n> arguments, where <n> is the number of arguments in the attempted method call.

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
{code}

*Screened by:* Alex Miller. Question: Is the performance impact prohibitive to add this reflective check for the existence of a static field when compiling 0-arg calls to static methods or static fields? It would be a bigger change (I think) but it might alternately be possible to do this check only in the case of a failure.
Invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

*Cause:* This particular path in the Compiler covers both static field access and 0 argument static method invocation and in this case results in a confusing exception.

*Patch:* clj-1130-v2.diff

*Approach:* The guess that the form is trying to access a static field is still done the same as before. However, with the patch it will now check for a field with the specified name, and if there is none, the compiler will assume that the user is really trying to invoke a static method.

The error message for attempting to call a static method now gives more details about the erroneous attempt, including the class name and that there is no such method that takes <n> arguments, where <n> is the number of arguments in the attempted method call.

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
{code}

*Screened by:* Alex Miller. Question: Is the performance impact prohibitive to add this reflective check for the existence of a static field when compiling 0-arg calls to static methods or static fields? It would be a bigger change (I think) but it might alternately be possible to do this check only in the case of a failure.
Attachment clj-1130-v2.diff [ 12364 ]
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.
Rich Hickey made changes -
Approval Screened [ 10004 ] Incomplete [ 10006 ]
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.
Alex Miller made changes -
Approval Incomplete [ 10006 ] Screened [ 10004 ]
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.
Andy Fingerhut made changes -
Attachment clj-1130-v2-ignore-ws.diff [ 12427 ]
Andy Fingerhut made changes -
Description Invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

*Cause:* This particular path in the Compiler covers both static field access and 0 argument static method invocation and in this case results in a confusing exception.

*Patch:* clj-1130-v2.diff

*Approach:* The guess that the form is trying to access a static field is still done the same as before. However, with the patch it will now check for a field with the specified name, and if there is none, the compiler will assume that the user is really trying to invoke a static method.

The error message for attempting to call a static method now gives more details about the erroneous attempt, including the class name and that there is no such method that takes <n> arguments, where <n> is the number of arguments in the attempted method call.

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
{code}

*Screened by:* Alex Miller. Question: Is the performance impact prohibitive to add this reflective check for the existence of a static field when compiling 0-arg calls to static methods or static fields? It would be a bigger change (I think) but it might alternately be possible to do this check only in the case of a failure.
Invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

*Cause:* This particular path in the Compiler covers both static field access and 0 argument static method invocation and in this case results in a confusing exception.

*Patch:* clj-1130-v2.diff
*Patch with whitespace differences ignored, intended only for review purposes, not for committing:* clj-1130-v2-ignore-ws.diff

*Approach:* The guess that the form is trying to access a static field is still done the same as before. However, with the patch it will now check for a field with the specified name, and if there is none, the compiler will assume that the user is really trying to invoke a static method.

The error message for attempting to call a static method now gives more details about the erroneous attempt, including the class name and that there is no such method that takes <n> arguments, where <n> is the number of arguments in the attempted method call.

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
{code}

*Screened by:* Alex Miller. Question: Is the performance impact prohibitive to add this reflective check for the existence of a static field when compiling 0-arg calls to static methods or static fields? It would be a bigger change (I think) but it might alternately be possible to do this check only in the case of a failure.
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.
Rich Hickey made changes -
Approval Screened [ 10004 ] Incomplete [ 10006 ]
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.
Alex Miller made changes -
Attachment clj-1130-v3.diff [ 12512 ]
Alex Miller made changes -
Description Invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

*Cause:* This particular path in the Compiler covers both static field access and 0 argument static method invocation and in this case results in a confusing exception.

*Patch:* clj-1130-v2.diff
*Patch with whitespace differences ignored, intended only for review purposes, not for committing:* clj-1130-v2-ignore-ws.diff

*Approach:* The guess that the form is trying to access a static field is still done the same as before. However, with the patch it will now check for a field with the specified name, and if there is none, the compiler will assume that the user is really trying to invoke a static method.

The error message for attempting to call a static method now gives more details about the erroneous attempt, including the class name and that there is no such method that takes <n> arguments, where <n> is the number of arguments in the attempted method call.

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
{code}

*Screened by:* Alex Miller. Question: Is the performance impact prohibitive to add this reflective check for the existence of a static field when compiling 0-arg calls to static methods or static fields? It would be a bigger change (I think) but it might alternately be possible to do this check only in the case of a failure.
Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

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

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

*Cause:* The decision about whether to check for field access is done based on the arity of the dot invocation so the call to a field and a 0-arg method invocation are conflated.

*Patch:* clj-1130-v3.diff

*Approach:* Instead of checking field/method based on the arity of the dot invocation, do a check for a field of that name. This avoids going down field-checking paths for 0-arity method calls and yields a better exception for those cases. Error message was improved to list class name and the arity

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching method found: setTime taking 0 args for class java.util.Date clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)
{code}

*Screened by:*
Summary Invoking a static method with the wrong number of arguments results in a NoSuchFieldException, rather than a proper message that the arguments could not be matched to the method Invoking an n-arity method with 0 args results in a field-related exception
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
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)
Stuart Sierra made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

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

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

*Cause:* The decision about whether to check for field access is done based on the arity of the dot invocation so the call to a field and a 0-arg method invocation are conflated.

*Patch:* clj-1130-v3.diff

*Approach:* Instead of checking field/method based on the arity of the dot invocation, do a check for a field of that name. This avoids going down field-checking paths for 0-arity method calls and yields a better exception for those cases. Error message was improved to list class name and the arity

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching method found: setTime taking 0 args for class java.util.Date clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)
{code}

*Screened by:*
Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

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

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

*Cause:* The decision about whether to check for field access is done based on the arity of the dot invocation so the call to a field and a 0-arg method invocation are conflated.

*Patch:* clj-1130-v3.diff

*Approach:* Instead of checking field/method based on the arity of the dot invocation, do a check for a field of that name. This avoids going down field-checking paths for 0-arity method calls and yields a better exception for those cases. Error message was improved to list class name and the arity

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching method found: setTime taking 0 args for class java.util.Date clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)
{code}

*Screened by:* Stuart Sierra
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.
Rich Hickey made changes -
Approval Screened [ 10004 ] Incomplete [ 10006 ]
Fix Version/s Release 1.6 [ 10157 ]
Stuart Halloway made changes -
Assignee Stuart Halloway [ stu ]
Stuart Halloway made changes -
Issue Type Defect [ 1 ] Enhancement [ 4 ]
Stuart Halloway made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Description Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

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

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

*Cause:* The decision about whether to check for field access is done based on the arity of the dot invocation so the call to a field and a 0-arg method invocation are conflated.

*Patch:* clj-1130-v3.diff

*Approach:* Instead of checking field/method based on the arity of the dot invocation, do a check for a field of that name. This avoids going down field-checking paths for 0-arity method calls and yields a better exception for those cases. Error message was improved to list class name and the arity

After patch:

{code}
user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 0 args, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: Class java.lang.Long has no method parseLong taking 3 args, compiling:(NO_SOURCE_PATH:2:1)
user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching method found: setTime taking 0 args for class java.util.Date clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)
{code}

*Screened by:* Stuart Sierra
Original motivation: Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

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

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

*Patch:* clj-1130-v4.diff

*Approach:* Primum non nocere. Error reporting enhanced at the site the errors happen, compiler logic unchanged.
Summary Invoking an n-arity method with 0 args results in a field-related exception when unable to match a method, report arity caller was looking for
Hide
Stuart Halloway added a comment -

v4 patch simply enhances error messaages

Show
Stuart Halloway added a comment - v4 patch simply enhances error messaages
Stuart Halloway made changes -
Attachment clj-1130-v4.diff [ 12735 ]
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.
Andy Fingerhut made changes -
Attachment clj-1130-v5.diff [ 12736 ]
Alex Miller made changes -
Description Original motivation: Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

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

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

*Patch:* clj-1130-v4.diff

*Approach:* Primum non nocere. Error reporting enhanced at the site the errors happen, compiler logic unchanged.
Original motivation: Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

{code}
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)
{code}

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

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

*Patch:* clj-1130-v5.diff

*Approach:* Primum non nocere. Error reporting enhanced at the site the errors happen, compiler logic unchanged.
Alex Miller made changes -
Priority Minor [ 4 ] Major [ 3 ]

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated: