ClojureScript

ClojureScript Analyzer/Compiler should have extensible hooks to handle warnings

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code

Description

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.

Activity

Hide
Chas Emerick added a comment -

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.

Show
Chas Emerick added a comment - 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.
Hide
David Nolen added a comment -

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.

Show
David Nolen added a comment - 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.
Hide
Sean Grove added a comment -

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.

Show
Sean Grove added a comment - 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.
Sean Grove made changes -
Field Original Value New Value
Attachment cljs-warning-middlewares.patch [ 12403 ]
Hide
Sean Grove added a comment -

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

Show
Sean Grove added a comment - A few fixes, tests pass, make sure (warning ... is passed the correct warning type, fix no-warn macro.
Sean Grove made changes -
Attachment cljs-warning-middlewares-2.patch [ 12405 ]
Attachment cljs-warning-middlewares-2.patch [ 12404 ]
Sean Grove made changes -
Attachment cljs-warning-middlewares.patch [ 12403 ]
Sean Grove made changes -
Attachment cljs-warning-middlewares-2.patch [ 12405 ]
Hide
David Nolen added a comment -

Can we please get a squashed patch? Thanks!

Show
David Nolen added a comment - Can we please get a squashed patch? Thanks!
Sean Grove made changes -
Attachment cljs_warning_handlers_in_analyzer.patch [ 12391 ]
Sean Grove made changes -
Attachment cljs-warning-middlewares-2.patch [ 12404 ]
Hide
Sean Grove added a comment -

Squashed patched

Show
Sean Grove added a comment - Squashed patched
Sean Grove made changes -
Attachment cljs-warning-middlewares.patch [ 12406 ]
Hide
David Nolen added a comment -

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

Show
David Nolen added a comment - Chas this looks pretty good to me, good enough for lein cljsbuild to use?
Sean Grove made changes -
Attachment cljs-warning-middlewares.patch [ 12406 ]
Hide
Sean Grove added a comment -

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?

Show
Sean Grove added a comment - 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?
Sean Grove made changes -
Attachment cljs-warning-middlewares.patch [ 12407 ]
Hide
David Nolen added a comment -

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

Show
David Nolen added a comment - We already have placeholder test files for the analyzer, compiler, and closure. Feel free to add tests!
Hide
Chas Emerick added a comment -

Will review ASAP. Thanks for working on this!

Show
Chas Emerick added a comment - Will review ASAP. Thanks for working on this!
Hide
Chas Emerick added a comment -

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!

Show
Chas Emerick added a comment - 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!
Hide
David Nolen added a comment -

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.

Show
David Nolen added a comment - 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.
Hide
Sean Grove added a comment -

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

Show
Sean Grove added a comment - Will take a look and add some clj tests to confirm the behavior.
Sean Grove made changes -
Attachment cljs-warning-middlewares.patch [ 12407 ]
Hide
Sean Grove added a comment -

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.

Show
Sean Grove added a comment - 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.
Sean Grove made changes -
Attachment cljs-warning-middleware.patch [ 12422 ]
Hide
Sean Grove added a comment -

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.

Show
Sean Grove added a comment - 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.
Sean Grove made changes -
Attachment cljs-warning-middleware.patch [ 12422 ]
Sean Grove made changes -
Attachment cljs-warning-middleware.patch [ 12423 ]
Hide
David Nolen added a comment -

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

Show
David Nolen added a comment - Patch looks good to me and it appears to work as advertised
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: