<< Back to previous view

[CLJS-636] ClojureScript Analyzer/Compiler should have extensible hooks to handle warnings Created: 24/Oct/13  Updated: 04/Nov/13  Resolved: 04/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: Sean Grove Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs, cljsbuild

Attachments: Text File cljs-warning-middleware.patch    
Patch: Code


To make the build process more configurable and stricter/less strict as needed by various projects, the cljs compiler should provide hooks that're invoked whenever a warning is emitted. Eventually these should be exposed through something like cljsbuild.

The attached patch changes the analyzer to emit warnings and emit dynamic warning handlers.

Note sure where the test for this could go though, it doesn't look like there's any infrastructure for the clj side.

Comment by Chas Emerick [ 25/Oct/13 11:24 AM ]

Couple of things:

1. You don't need when-not; if the handlers var is empty or nil, doseq will handle it just fine.
2. The default value of the handlers var should probably be a function that mimics current functionality?
3. I'd like to suggest that all of the relevant data for each warning be passed in addition to the type of warning. i.e. instead of just :dynamic, {:type :dynamic :name (:name ev)}, etc.

Comment by David Nolen [ 25/Oct/13 12:14 PM ]

I agree that the default value of the handler should do what we currently do and that we should pass all information about the warning we have.

Comment by Sean Grove [ 26/Oct/13 12:14 AM ]

Updated to remove (when-not (nil?..., and moves the current behavior into cljs-warning-handlers as the default handler.

It also changes the warning types to introduce more nuanced differences, allowing the default-warning-handler to be entirely self-contained. It relies on different keys being in the map for any given error type when producing the warning message, but it's probably manageable.

It also means there's a neat place to to addition error-type-specific information for handlers to pull out as needed. Right now just enough goes into each type to reproduce the current default behavior, but more could be passed along as needed - it's not always a standard set of data for each type, so maybe this is a good way to go about it.

Comment by Sean Grove [ 26/Oct/13 12:32 AM ]

A few fixes, tests pass, make sure (warning ... is passed the correct warning type, fix no-warn macro.

Comment by David Nolen [ 26/Oct/13 9:58 AM ]

Can we please get a squashed patch? Thanks!

Comment by Sean Grove [ 26/Oct/13 12:28 PM ]

Squashed patched

Comment by David Nolen [ 26/Oct/13 2:22 PM ]

Chas this looks pretty good to me, good enough for lein cljsbuild to use?

Comment by Sean Grove [ 27/Oct/13 11:10 AM ]

Fix default-warning-handler to respect cljs-warnings. Tests still pass, but would like having some clj-side tests to assert behavior around this stuff. Should I open another issue about getting clojure.test setup and tied into the build, or is that uninteresting right now?

Comment by David Nolen [ 27/Oct/13 8:07 PM ]

We already have placeholder test files for the analyzer, compiler, and closure. Feel free to add tests!

Comment by Chas Emerick [ 27/Oct/13 9:50 PM ]

Will review ASAP. Thanks for working on this!

Comment by Chas Emerick [ 28/Oct/13 8:02 AM ]

The patch looks good. I think people are going to want to delegate to default-warning-handler as a fall-through (e.g. to emit the message corresponding to a warning, even if a particular tool is going to choose to treat it as an error), but we can address that later. Once this lands, we'll be able to tie up https://github.com/emezeske/lein-cljsbuild/issues/225. Thanks!

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

Sean, when I run (fn [] (+ a b)) at the REPL (script/repljs) prior to this patch I get warnings about undeclared vars. After applying the patch I no longer get warnings.

Comment by Sean Grove [ 28/Oct/13 9:50 AM ]

Will take a look and add some clj tests to confirm the behavior.

Comment by Sean Grove [ 02/Nov/13 2:46 PM ]

Made sure all references to cljs-warnings were updated appropriately with the new warning types, added some basic tests, changed confirm-var-exists behavior to only call warning if a warning case has been triggered.

Comment by Sean Grove [ 02/Nov/13 2:53 PM ]

There are some bigger-than-expected changes in here, and it'd be nice to get some extra eyes on it to make sure warnings are triggered where expected. I tested a lot of it out in the repl and added some tests, but don't have enough code to exercise all of the paths.

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

Patch looks good to me and it appears to work as advertised

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

fixed, http://github.com/clojure/clojurescript/commit/abd44502079425f76f31c5432e7366ecd33d866a

Generated at Sat Feb 25 17:05:44 CST 2017 using JIRA 4.4#649-r158309.