ClojureScript

Move incorrect *analyze-deps* check

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
  • Patch:
    Code

Description

A new analyze-deps check was added recently [1] which is at the wrong location. When running with analzye-deps false (which shadow-build always does) it will also disable all macro loading. The attached patch moves this check to the "correct" position.

[1] https://github.com/clojure/clojurescript/commit/08805fd519a46ca80aa95b9a50af8c8293df0d3f

Activity

Hide
Thomas Heller added a comment -

Wait ... this new behavior completely disables all warnings for shadow-build since it runs with analyze-deps false.

I don't think this flag should affect the warnings at all. Either a new flag should be introduced to suppress the warnings or the warning code should be moved to a analyzer pass which can be toggled on demand. Moving more side effects out of parsing is probably a good idea anyways (see CLJS-948).

Willing to provide a patch for either case.

Show
Thomas Heller added a comment - Wait ... this new behavior completely disables all warnings for shadow-build since it runs with analyze-deps false. I don't think this flag should affect the warnings at all. Either a new flag should be introduced to suppress the warnings or the warning code should be moved to a analyzer pass which can be toggled on demand. Moving more side effects out of parsing is probably a good idea anyways (see CLJS-948). Willing to provide a patch for either case.
Hide
David Nolen added a comment -

I need wrap up some things around REPL support before this can be addressed. Happy to look at it when things have settled down.

Show
David Nolen added a comment - I need wrap up some things around REPL support before this can be addressed. Happy to look at it when things have settled down.
Hide
Thomas Heller added a comment -

Ok, I'll keep an eye of HEAD and provide a new patch if required.

The issue is pretty straightforward though. If cljs.analyzer/parse-ns should not load macros it should bind cljs.analyzer/load-macros to false, currently analzyze-deps sort of hijacks that flag because of (and analyze-deps load-macros).

Show
Thomas Heller added a comment - Ok, I'll keep an eye of HEAD and provide a new patch if required. The issue is pretty straightforward though. If cljs.analyzer/parse-ns should not load macros it should bind cljs.analyzer/load-macros to false, currently analzyze-deps sort of hijacks that flag because of (and analyze-deps load-macros).
Hide
David Nolen added a comment -

I don't see a problem for your use case other than the API defaults have changed. Why can't you just set analyze-deps to true and load-macros to false?

Show
David Nolen added a comment - I don't see a problem for your use case other than the API defaults have changed. Why can't you just set analyze-deps to true and load-macros to false?
Hide
Thomas Heller added a comment -

because (and true false) equals false.

Show
Thomas Heller added a comment - because (and true false) equals false.
Hide
David Nolen added a comment - - edited

Sorry not understanding your point. You say you want to analyze deps but you don't want to analyze macros. Then false is the right thing in your case. Not wanting to analyze deps but wanting to load macros doesn't make sense to me.

Show
David Nolen added a comment - - edited Sorry not understanding your point. You say you want to analyze deps but you don't want to analyze macros. Then false is the right thing in your case. Not wanting to analyze deps but wanting to load macros doesn't make sense to me.
Hide
Thomas Heller added a comment -

I use a completely different approach to compiling in dependency order than CLJS does. Basically I do one discovery pass (extracts goog.provide/require from JS files, parse NS of CLJS file) then sort everything and compile top-down (dependency order). analyze with analyze-deps is recursive and compiles bottom-up. Since we were talking about parallel compile, bottom-up doesn't work parallel.

Also since I control the loop that does the file compilation I can compile in memory without touching files (analyze-deps ALWAYS uses anaylze-file).

Given that I can obviously run with analyze-deps on the second pass through since I compile in dep order the deps are already compiled. I just don't want it to ever enter analyze-deps because it doesn't need to.

Also your change didn't change API defaults it broke the API.

load-macros was meant to to be able to skip the require of macros (since I don't need them on the first pass)
analyze-deps used to control if dependencies should be compiled, now it also controls warnings?

Show
Thomas Heller added a comment - I use a completely different approach to compiling in dependency order than CLJS does. Basically I do one discovery pass (extracts goog.provide/require from JS files, parse NS of CLJS file) then sort everything and compile top-down (dependency order). analyze with analyze-deps is recursive and compiles bottom-up. Since we were talking about parallel compile, bottom-up doesn't work parallel. Also since I control the loop that does the file compilation I can compile in memory without touching files (analyze-deps ALWAYS uses anaylze-file). Given that I can obviously run with analyze-deps on the second pass through since I compile in dep order the deps are already compiled. I just don't want it to ever enter analyze-deps because it doesn't need to. Also your change didn't change API defaults it broke the API. load-macros was meant to to be able to skip the require of macros (since I don't need them on the first pass) analyze-deps used to control if dependencies should be compiled, now it also controls warnings?
Hide
David Nolen added a comment -

Well there's not a true API broken here since none of this stuff is officially anything we support.

analyze-deps never controlled whether dependencies should be compiled, it only performs analysis in dependency order so that warning & optimization information can be properly propagated. It falls out of not necessarily compiling files in dependency order. And I believe the current bottom-up behavior still has utility for analysis tools that aren't involved in building.

Still I see the value of keeping these two bits separate in your case now.

Show
David Nolen added a comment - Well there's not a true API broken here since none of this stuff is officially anything we support. analyze-deps never controlled whether dependencies should be compiled, it only performs analysis in dependency order so that warning & optimization information can be properly propagated. It falls out of not necessarily compiling files in dependency order. And I believe the current bottom-up behavior still has utility for analysis tools that aren't involved in building. Still I see the value of keeping these two bits separate in your case now.
Hide
Thomas Heller added a comment -

Sorry mixed up compile with analyze. Of cource analyze-file doesn't compile, still it touches files and does the whole caching bits.

Still analyze-deps didn't use to toggle the warnings, but CLJS-948 will probably address all this anyways.

Side Note: I used the two pass approach in shadow-build since I wanted to discover closure-compliant JS files that aren't mentioned in deps.js. I have a few plain JS dependencies in my app which I converted to be closure-aware when I rewrote my project from JS->CLJS. cljs.closure (or lein-cljsbuild) still don't recognize these unless you configure it very explicitly. I think there is an open issue for this as well.

Show
Thomas Heller added a comment - Sorry mixed up compile with analyze. Of cource analyze-file doesn't compile, still it touches files and does the whole caching bits. Still analyze-deps didn't use to toggle the warnings, but CLJS-948 will probably address all this anyways. Side Note: I used the two pass approach in shadow-build since I wanted to discover closure-compliant JS files that aren't mentioned in deps.js. I have a few plain JS dependencies in my app which I converted to be closure-aware when I rewrote my project from JS->CLJS. cljs.closure (or lein-cljsbuild) still don't recognize these unless you configure it very explicitly. I think there is an open issue for this as well.
Hide
David Nolen added a comment - - edited

I've committed the following to master https://github.com/clojure/clojurescript/commit/7b683ed43c9316d8163c2f764cc6e9b2710c3f46

Hopefully this clears up how parse-ns is actually used by the compiler, which might have been
a source of confusion. I split apart the two flags, however cljs.analyzer/parse-ns has defaults which differ
from the root bindings of *analyze-deps* and *load-macros* which must be
explicitly over-ridden. It's intention was always as a helper for build tools not for running any actual
analysis or loading any macros.

If this is satisfactory then I'll close this ticket.

Show
David Nolen added a comment - - edited I've committed the following to master https://github.com/clojure/clojurescript/commit/7b683ed43c9316d8163c2f764cc6e9b2710c3f46 Hopefully this clears up how parse-ns is actually used by the compiler, which might have been a source of confusion. I split apart the two flags, however cljs.analyzer/parse-ns has defaults which differ from the root bindings of *analyze-deps* and *load-macros* which must be explicitly over-ridden. It's intention was always as a helper for build tools not for running any actual analysis or loading any macros. If this is satisfactory then I'll close this ticket.
Hide
David Nolen added a comment -

Fixed in master

Show
David Nolen added a comment - Fixed in master
Hide
Thomas Heller added a comment -

CLJS-948 sort of reintroduces the problem.

It is because we combine *analyze-deps* to mean analyze deps and warn about deps. I don't want shadow-build to analyze deps but I want the warnings. Would you be ok with introducing a *check-deps* or should I find another way to address this in shadow-build?

We should resolve CLJS-948 first, I'll come back to this once we have.

Show
Thomas Heller added a comment - CLJS-948 sort of reintroduces the problem. It is because we combine *analyze-deps* to mean analyze deps and warn about deps. I don't want shadow-build to analyze deps but I want the warnings. Would you be ok with introducing a *check-deps* or should I find another way to address this in shadow-build? We should resolve CLJS-948 first, I'll come back to this once we have.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: