Welcome! Please see the About page for a little more info on how this works.

0 votes
in ClojureScript by
`:warning-handlers` currently takes a function objects, which are not possible to send in when invoking through cljs.main.

To assist build tooling, augment these options to support taking in a symbol that goes through `requiring-resolve` in 1.10.  That way, build tooling can refer to functions on the classpath.

CLJS can steal https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/server.clj#L263-L270

I believe :watch-fn and :watch-error-fn do the right thing already, via https://github.com/clojure/clojurescript/blob/47386d7c03e6fc36dc4f0145bd62377802ac1c02/src/main/clojure/cljs/closure.clj#L80-L109

h4. Desired Behaviour
The {{:warning-handlers}} compiler option is now able to specify functions via their fully qualified namespaced symbols. For example a {{--compile-opts}} with the following {{cljs.edn}}:

{:warning-handlers [some.namespace/warning-handler1
                    some.namespace/warning-handler2]}

will find and resolve warning handler functions {{warning-handler1}} and {{warning-handler2}} so long as they exist and are on the classpath.

h4. Analysis
* As mentioned above, we do have prior work which resolves compiler options: {{:watch-fn}}, {{:watch-error-fn}} and also {{:preprocess}}.
* {{:warning-handlers}} is bound to {{\*cljs-warning-handlers\*}} in several places, but only used from one function: {{cljs.analyzer/warning}}.
* Prior work makes use of private function {{cljs.closure/sym->var}} to resolve.

h4. Approach
Move {{sym->var}} function from {{cljs.closure}} to {{cljs.util}}, make it public, and make {{load-mutex}} a parameter.
Make use of {{sym->var}} from {{cljs.analyzer/warning}} to resolve each warning-handler.

h4. Tests
Add tests to {{cljs.analyzer-api-tests}} to verify success and symbol mis-configuration paths.

h4. Implementation Notes
# Self-hosting ClojureScript is not considered
# {{sym->var}} throws {{:compilation}} phase exceptions, I think this is appropriate for warning-handlers given the choices: https://clojure.org/reference/repl_and_main#_error_printing
# {{sym->var}} includes the associated compiler option keyword within {{ex-info}}'s map. But... in {{cljs.analyzer/warning}}, we are working with the binding {{\*cljs-warning-handlers\*}} rather than the compiler option keyword. I assume making reference to {{:warning-handlers}} in an error from {{cljs.analyzer/warning}} which is really dealing with {{\*cljs-warning-handlers\*}} is acceptable.

h4. Observations
# {{sym->var}} and its usage are a bit odd in that they throw when sym ns is not found and throw when sym is not fully qualified but the error of sym not being resolved is quietly ignored. This seems a bit user cruel to me. For this change, I've introduced and used {{sym->var-checked}} to throw when sym is not resolved. A separate ticket might consider using {{sym->var-checked}} in place of existing calls to {{sym->var}}.
# Because warning handlers are only resolved in {{cljs.analyzer/warning}} the user will not learn of mis-configuration until a warning occurs. This could be considered a pro or con depending on your perspective. Perhaps repeating resolution earlier from a likely path, if only to fail faster, would be helpful. Maybe from {{cljs.cli/load-edn-opts}}?

8 Answers

0 votes
by

Comment made by: lread

Here's my first attempt at this one, let me know what you think.

0 votes
by

Comment made by: mfikes

CLJS-3074.patch passes CI and Canary (/)

0 votes
by
_Comment made by: mfikes_

Oops. I missed that CLJS\-3074 fails in Windows CI (x)

In particular:


FAIL in (with-resolve-warning-handlers-test) (analyzer_api_tests.clj:52)
94expected: (= "H1 :fn-arity\nH2 :fn-arity\n" (with-out-str (ana-api/analyze test-cenv test-env warning-form nil (clojure.edn/read-string "{:warning-handlers [cljs.analyzer-api-tests/resolve-handler1\n                                  cljs.analyzer-api-tests/resolve-handler2]}"))))
95  actual: (not (= "H1 :fn-arity\nH2 :fn-arity\n" "H1 :fn-arity\r\nH2 :fn-arity\r\n"))
96
97


Failing CI build: https://ci.appveyor.com/project/mfikes/clojurescript/builds/24361884
0 votes
by

Comment made by: lread

Oh ho! Thanks Mike, I shall fix shortly.

0 votes
by

Comment made by: lread

CLJS-3074-2.patch replaces CLJS-3074.patch and addresses Windows CI failure. Test now deals with newline differences.

0 votes
by

Comment made by: mfikes

CLJS-3074-2.patch added to Patch Tender (i)

0 votes
by

Comment made by: mfikes

CLJS-3074-2.patch passes CI (/)

0 votes
by
Reference: https://clojure.atlassian.net/browse/CLJS-3074 (reported by gshayban)
...