<< Back to previous view

[CLJS-622] defining zero-arity protocol method produces incomprehensible error message Created: 16/Oct/13  Updated: 02/Dec/14  Resolved: 02/Dec/14

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: George Fraser Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Clojurescript 0.0-1913

Attachments: File cljs-622-20131104.diff     File protocol-warning-20131017-2.diff     File protocol-warning-20131017.diff     File protocol-warning-20131018.diff    


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.

Comment by Travis Thieman [ 17/Oct/13 11:02 AM ]

Patch: 20131017

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

Comment by David Nolen [ 17/Oct/13 2:30 PM ]

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!

Comment by Travis Thieman [ 17/Oct/13 3:59 PM ]

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.

Comment by David Nolen [ 17/Oct/13 4:13 PM ]

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.

Comment by Travis Thieman [ 17/Oct/13 4:26 PM ]

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
  [^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)))))
Comment by Travis Thieman [ 18/Oct/13 10:05 AM ]

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.

Comment by David Nolen [ 18/Oct/13 10:23 AM ]

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.

Comment by Travis Thieman [ 18/Oct/13 10:42 AM ]

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?

Comment by David Nolen [ 28/Oct/13 8:51 AM ]

CLJS-636 must be addressed first.

Comment by David Nolen [ 04/Nov/13 8:22 AM ]

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

Comment by Travis Thieman [ 04/Nov/13 9:21 AM ]

Patch: 20131104

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

Comment by Chas Emerick [ 19/Mar/14 9:41 AM ]

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

Comment by David Nolen [ 02/Dec/14 8:08 AM ]

fixed https://github.com/clojure/clojurescript/commit/0ea0ca0c73f827f56436d738e8fca1a9c9b99f2c

Generated at Tue Sep 19 15:51:31 CDT 2017 using JIRA 4.4#649-r158309.