ClojureScript

defining zero-arity protocol method produces incomprehensible error message

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Environment:
    Clojurescript 0.0-1913

Description

In Clojure:
> (defprotocol P (f []))
IllegalArgumentException Protocol fn: f must take at least on arg [...]

In ClojureScript
> (defprotocol P (f []))
[ 100 lines of incomprehensible stack traces ]

Would be nice if it gave a better error message.

  1. cljs-622-20131104.diff
    04/Nov/13 9:21 AM
    2 kB
    Travis Thieman
  2. protocol-warning-20131017.diff
    17/Oct/13 11:02 AM
    1 kB
    Travis Thieman
  3. protocol-warning-20131017-2.diff
    17/Oct/13 3:59 PM
    2 kB
    Travis Thieman
  4. protocol-warning-20131018.diff
    18/Oct/13 10:05 AM
    1 kB
    Travis Thieman

Activity

Hide
Travis Thieman added a comment -

Patch: 20131017

Moves the exception to the defprotocol macro itself and throws an exception matching Clojure's.

Show
Travis Thieman added a comment - Patch: 20131017 Moves the exception to the defprotocol macro itself and throws an exception matching Clojure's.
Travis Thieman made changes -
Field Original Value New Value
Attachment protocol-warning-20131017.diff [ 12332 ]
Hide
David Nolen added a comment -

It's preferable to throw ex-info, look around the compiler for an example. Also this error should include file, line and column information - again look around for ex-info examples to see what I mean. Thanks!

Show
David Nolen added a comment - It's preferable to throw ex-info, look around the compiler for an example. Also this error should include file, line and column information - again look around for ex-info examples to see what I mean. Thanks!
Hide
Travis Thieman added a comment - - edited

v2 of the 20131017 patch uses ex-info to throw a more descriptive error message. To support this, ana/cljs-file is now bound in closure.clj/compile-file.

Show
Travis Thieman added a comment - - edited v2 of the 20131017 patch uses ex-info to throw a more descriptive error message. To support this, ana/cljs-file is now bound in closure.clj/compile-file.
Travis Thieman made changes -
Attachment protocol-warning-20131017-2.diff [ 12337 ]
Hide
David Nolen added a comment -

The `binding` to setup `cljs.analyzer/cljs-file` is not necessary. If it is necessary you will need to show why it is. `cljs.compiler/compile-file` calls `cljs.compiler/compile-file*` which already does this.

Show
David Nolen added a comment - The `binding` to setup `cljs.analyzer/cljs-file` is not necessary. If it is necessary you will need to show why it is. `cljs.compiler/compile-file` calls `cljs.compiler/compile-file*` which already does this.
Hide
Travis Thieman added a comment -

I encountered this as an issue because of the specific way I was testing compilation. I create a test file and compiled it using "/bin/cljsc test.cljs". This compiles from source files.

However, when it calls compile-file, no output file is specified, instead writing to stdout, e.g. opts contains {:output-to :print} and does not contain :output-file. As a result, the second branch of the if gets called, which compiles a sequence of forms instead of a file. As a result, the cljs-file never gets bound, even though there are input files.

The binding should still be moved to only affect the compile-form-seq branch.

(defn compile-file
  "Compile a single cljs file. If no output-file is specified, returns
  a string of compiled JavaScript. With an output-file option, the
  compiled JavaScript will written to this location and the function
  returns a JavaScriptFile. In either case the return value satisfies
  IJavaScript."
  [^File file {:keys [output-file] :as opts}]
  (binding [ana/*cljs-file* (.getPath ^java.io.File file)]
    (if output-file
      (let [out-file (io/file (output-directory opts) output-file)]
        (compiled-file (comp/compile-file file out-file opts)))
      (compile-form-seq (ana/forms-seq file)))))
Show
Travis Thieman added a comment - I encountered this as an issue because of the specific way I was testing compilation. I create a test file and compiled it using "/bin/cljsc test.cljs". This compiles from source files. However, when it calls compile-file, no output file is specified, instead writing to stdout, e.g. opts contains {:output-to :print} and does not contain :output-file. As a result, the second branch of the if gets called, which compiles a sequence of forms instead of a file. As a result, the cljs-file never gets bound, even though there are input files. The binding should still be moved to only affect the compile-form-seq branch.
(defn compile-file
  "Compile a single cljs file. If no output-file is specified, returns
  a string of compiled JavaScript. With an output-file option, the
  compiled JavaScript will written to this location and the function
  returns a JavaScriptFile. In either case the return value satisfies
  IJavaScript."
  [^File file {:keys [output-file] :as opts}]
  (binding [ana/*cljs-file* (.getPath ^java.io.File file)]
    (if output-file
      (let [out-file (io/file (output-directory opts) output-file)]
        (compiled-file (comp/compile-file file out-file opts)))
      (compile-form-seq (ana/forms-seq file)))))
Hide
Travis Thieman added a comment -

Patch: 20131018

Kept the ex-info throw, but removed the additional fix to closure.clj. I'll make a separate issue for the compilation problem.

Show
Travis Thieman added a comment - Patch: 20131018 Kept the ex-info throw, but removed the additional fix to closure.clj. I'll make a separate issue for the compilation problem.
Travis Thieman made changes -
Attachment protocol-warning-20131018.diff [ 12338 ]
Hide
David Nolen added a comment -

Upon further reflection, it's undesirable for this to throw. We should just convert it into a warning. I need to refactor warnings a bit so patch for this ticket should hold off until I get around to that.

Show
David Nolen added a comment - Upon further reflection, it's undesirable for this to throw. We should just convert it into a warning. I need to refactor warnings a bit so patch for this ticket should hold off until I get around to that.
Hide
Travis Thieman added a comment -

If the exception is not thrown here, a more generic exception is still thrown further down the line after the defprotocol macroexpansion, something about an invalid dot form. Is the plan to also fix that so that (defprotocol P (f [])) would fully compile? If not, what are the reasons for not wanting to throw here?

Show
Travis Thieman added a comment - If the exception is not thrown here, a more generic exception is still thrown further down the line after the defprotocol macroexpansion, something about an invalid dot form. Is the plan to also fix that so that (defprotocol P (f [])) would fully compile? If not, what are the reasons for not wanting to throw here?
Hide
David Nolen added a comment -

CLJS-636 must be addressed first.

Show
David Nolen added a comment - CLJS-636 must be addressed first.
Hide
David Nolen added a comment -

CLJS-636 is resolved can we get an updated patch?

Show
David Nolen added a comment - CLJS-636 is resolved can we get an updated patch?
Hide
Travis Thieman added a comment -

Patch: 20131104

Add warning type :illegal-protocol-method and throw it on zero-arity protocol methods.

Show
Travis Thieman added a comment - Patch: 20131104 Add warning type :illegal-protocol-method and throw it on zero-arity protocol methods.
Travis Thieman made changes -
Attachment cljs-622-20131104.diff [ 12428 ]
Hide
Chas Emerick added a comment -

This patch no longer applies, due to application of the patch from CLJS-627. Can you attach an updated patch?

Show
Chas Emerick added a comment - This patch no longer applies, due to application of the patch from CLJS-627. Can you attach an updated patch?
David Nolen made changes -
Priority Minor [ 4 ] Major [ 3 ]
David Nolen made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
David Nolen made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: