Clojure

defprotocol: invalid method overload syntax getting accepted

Details

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

Description

The compiler accepts this erroneous form:

user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar

Analysis: defprotocol silently assoc's the last list of signatures found for any particular method name, without checking whether the method name was given earlier.

Patch: clj-1056-2.diff

Approach: Modify defprotocol to check whether each method name has already been encountered earlier, and throw an exception if so. The patch also updates the error message for the case of a protocol function with no args specified.

Behavior with patch clj-1056-2.txt:

user=> (defprotocol Bar (m [this]) (m [this arg]))
IllegalArgumentException Function m in protocol Bar was redefined. Specify all arities in single definition.
user=> (defprotocol Foo (m []))
IllegalArgumentException Definition of function m in protocol Foo must take at least one arg.

Screened by: Stuart Halloway

  1. clj-1056-1.txt
    10/Sep/13 11:24 PM
    3 kB
    Andy Fingerhut
  2. clj-1056-2.diff
    22/Oct/13 9:14 AM
    3 kB
    Alex Miller
  3. clj-1056-2.txt
    18/Oct/13 11:08 AM
    3 kB
    Alex Miller

Activity

Hide
Timothy Baldridge added a comment -

Can not reproduce the fist error:

user=> (defprotocol Foo (f ([this]) ([this arg])))
CompilerException java.lang.IllegalArgumentException: Parameter declaration missing, compiling:(NO_SOURCE_PATH:5:1)

But the 2nd one I can reproduce:

user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar
user=> Bar
{:on-interface user.Bar, :on user.Bar, :sigs {:m {:doc nil, :arglists ([this arg]), :name m}}, :var #'user/Bar, :method-map {:m :m}, :method-builders {#'user/m #<user$eval71$fn_72 user$eval71$fn_72@1a2b53fb>}}
user=>

Notice that :arglists only has one entry

Vetting

Show
Timothy Baldridge added a comment - Can not reproduce the fist error: user=> (defprotocol Foo (f ([this]) ([this arg]))) CompilerException java.lang.IllegalArgumentException: Parameter declaration missing, compiling:(NO_SOURCE_PATH:5:1) But the 2nd one I can reproduce: user=> (defprotocol Bar (m [this]) (m [this arg])) Bar user=> Bar {:on-interface user.Bar, :on user.Bar, :sigs {:m {:doc nil, :arglists ([this arg]), :name m}}, :var #'user/Bar, :method-map {:m :m}, :method-builders {#'user/m #<user$eval71$fn_72 user$eval71$fn_72@1a2b53fb>}} user=> Notice that :arglists only has one entry Vetting
Timothy Baldridge made changes -
Field Original Value New Value
Approval Vetted [ 10003 ]
Affects Version/s Release 1.5 [ 10150 ]
Hide
Alex Miller added a comment -

Moving back to Triaged as Rich has not vetted.

Show
Alex Miller added a comment - Moving back to Triaged as Rich has not vetted.
Alex Miller made changes -
Approval Vetted [ 10003 ] Triaged [ 10120 ]
Alex Miller made changes -
Labels protocols
Rich Hickey made changes -
Approval Triaged [ 10120 ] Vetted [ 10003 ]
Fix Version/s Release 1.6 [ 10157 ]
Hide
Andy Fingerhut added a comment -

Patch clj-1056-1.txt changes defprotocol to throw an exception if the same method name appears more than once.

Without this patch, the last set of signatures for a method name "wins", silently overriding any earlier ones.

Show
Andy Fingerhut added a comment - Patch clj-1056-1.txt changes defprotocol to throw an exception if the same method name appears more than once. Without this patch, the last set of signatures for a method name "wins", silently overriding any earlier ones.
Andy Fingerhut made changes -
Attachment clj-1056-1.txt [ 12254 ]
Andy Fingerhut made changes -
Patch Code and Test [ 10002 ]
Andy Fingerhut made changes -
Description The compiler accepts both of these erroneous forms which while silly, are not imposible to come up with.

(defprotocol Foo (f ([this]) ([this arg])))
(defprotocol Bar (m [this]) (m [this arg]))
The compiler accepts both of these erroneous forms which while silly, are not imposible to come up with.

(defprotocol Foo (f ([this]) ([this arg])))
(defprotocol Bar (m [this]) (m [this arg]))

*Analysis*: defprotocol silently assoc's the last list of signatures found for any particular method name, without checking whether the method name was given earlier.

*Patch*: clj-1056-1.txt

*Approach*: Modify defprotocol to check whether each method name has already been encountered earlier, and throw a CompilerException with file, line, and column number if so.

Behavior without this patch:

{code}
java -cp clojure-1.6.0-master-SNAPSHOT.jar clojure.main
Clojure 1.6.0-master-SNAPSHOT
user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar
{code}

Behavior with patch clj-1056-1.txt:

{code}
user=> (defprotocol Bar (m [this]) (m [this arg]))
CompilerException java.lang.IllegalArgumentException: Multiple arities for protocol fn: m must all be defined together, compiling:(NO_SOURCE_PATH:1:1)
{code}
Stuart Halloway made changes -
Assignee Stuart Halloway [ stu ]
Alex Miller made changes -
Assignee Stuart Halloway [ stu ] Alex Miller [ alexmiller ]
Stuart Halloway made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Hide
Alex Miller added a comment -

Updated patch to improve error messages and to switch from CompilerException (which is not used outside the Compiler) back to IllegalArgumentException.

I think it would be a good idea to have a general Exception that could carry file/line/col info that could be used by Compiler but also other errors (the EdnReader and LispReader both have their own variants). But I think that should be a separate enhancement, which I have filed as CLJ-1280.

Show
Alex Miller added a comment - Updated patch to improve error messages and to switch from CompilerException (which is not used outside the Compiler) back to IllegalArgumentException. I think it would be a good idea to have a general Exception that could carry file/line/col info that could be used by Compiler but also other errors (the EdnReader and LispReader both have their own variants). But I think that should be a separate enhancement, which I have filed as CLJ-1280.
Alex Miller made changes -
Description The compiler accepts both of these erroneous forms which while silly, are not imposible to come up with.

(defprotocol Foo (f ([this]) ([this arg])))
(defprotocol Bar (m [this]) (m [this arg]))

*Analysis*: defprotocol silently assoc's the last list of signatures found for any particular method name, without checking whether the method name was given earlier.

*Patch*: clj-1056-1.txt

*Approach*: Modify defprotocol to check whether each method name has already been encountered earlier, and throw a CompilerException with file, line, and column number if so.

Behavior without this patch:

{code}
java -cp clojure-1.6.0-master-SNAPSHOT.jar clojure.main
Clojure 1.6.0-master-SNAPSHOT
user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar
{code}

Behavior with patch clj-1056-1.txt:

{code}
user=> (defprotocol Bar (m [this]) (m [this arg]))
CompilerException java.lang.IllegalArgumentException: Multiple arities for protocol fn: m must all be defined together, compiling:(NO_SOURCE_PATH:1:1)
{code}
The compiler accepts this erroneous form:

{code}
user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar
{code}

*Analysis*: defprotocol silently assoc's the last list of signatures found for any particular method name, without checking whether the method name was given earlier.

*Patch*: clj-1056-2.txt

*Approach*: Modify defprotocol to check whether each method name has already been encountered earlier, and throw an exception if so. The patch also updates the error message for the case of a protocol function with no args specified.

Behavior with patch clj-1056-2.txt:

{code}
user=> (defprotocol Bar (m [this]) (m [this arg]))
IllegalArgumentException Function m in protocol Bar was redefined. Specify all arities in single definition.
user=> (defprotocol Foo (m []))
IllegalArgumentException Definition of function m in protocol Foo must take at least one arg.
{code}

*Screened by:* Stuart Halloway
Attachment clj-1056-2.txt [ 12339 ]
Alex Miller made changes -
Labels protocols errormsgs protocols
Alex Miller made changes -
Description The compiler accepts this erroneous form:

{code}
user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar
{code}

*Analysis*: defprotocol silently assoc's the last list of signatures found for any particular method name, without checking whether the method name was given earlier.

*Patch*: clj-1056-2.txt

*Approach*: Modify defprotocol to check whether each method name has already been encountered earlier, and throw an exception if so. The patch also updates the error message for the case of a protocol function with no args specified.

Behavior with patch clj-1056-2.txt:

{code}
user=> (defprotocol Bar (m [this]) (m [this arg]))
IllegalArgumentException Function m in protocol Bar was redefined. Specify all arities in single definition.
user=> (defprotocol Foo (m []))
IllegalArgumentException Definition of function m in protocol Foo must take at least one arg.
{code}

*Screened by:* Stuart Halloway
The compiler accepts this erroneous form:

{code}
user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar
{code}

*Analysis*: defprotocol silently assoc's the last list of signatures found for any particular method name, without checking whether the method name was given earlier.

*Patch*: clj-1056-2.diff

*Approach*: Modify defprotocol to check whether each method name has already been encountered earlier, and throw an exception if so. The patch also updates the error message for the case of a protocol function with no args specified.

Behavior with patch clj-1056-2.txt:

{code}
user=> (defprotocol Bar (m [this]) (m [this arg]))
IllegalArgumentException Function m in protocol Bar was redefined. Specify all arities in single definition.
user=> (defprotocol Foo (m []))
IllegalArgumentException Definition of function m in protocol Foo must take at least one arg.
{code}

*Screened by:* Stuart Halloway
Attachment clj-1056-2.diff [ 12368 ]
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: