<< Back to previous view

[CLJS-615] Warning when required lib does not exist on disk Created: 08/Oct/13  Updated: 22/Feb/14  Resolved: 17/Feb/14

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Timothy Pratley
Resolution: Completed Votes: 0
Labels: patch

Attachments: Text File CLJS-615.patch    
Patch: Code and Test

 Description   

Currently when analyzing dependencies in analyzer.clj we do not warn when the library does not exist on disk. The error should be similar to the one emitted by Clojure so that the user is aware how the namespace maps to directories and filenames.

Problem
==

hello.cljs:

(ns hello
(:require [typomina :refer [x]]))

Expected: Should be reported when typomina is a namespace never provided.
Actual: when optimization is none or whitespace, no warnings are given.
Expected: when x does not exist in typomina, hello should not compile
Actual: x is not verified even if typomina exists

Background
==

Dependencies are not resolved at cljs compile time.
cljs.closure docstring:
"build = compile -> add-dependencies -> optimize -> output"

Dependencies are deferred to Closure
https://github.com/clojure/clojurescript/wiki/Differences-from-Clojure:
"designed to leverage Google's Closure library, and participates in its dependency/require/provide mechanism"

Closure compiler detects missing dependencies when optimizations simple or advanced are applied:
./bin/cljsc hello.cljs '{:optimizations :simple :output-to "hello.js"}'
Dec 31, 2013 4:10:03 PM com.google.javascript.jscomp.LoggerErrorManager println
SEVERE: hello:3: ERROR - required "typomina" namespace never provided
goog.require('typomina');
^
ERROR: JSC_MISSING_PROVIDE_ERROR. required "typomina" namespace never provided at hello line 3 : 0

But not for whitespace or none. For none, Closure is not even called by cljs "optimize".
./bin/cljsc hello.cljs '{:optimizations :whitespace :output-to "hello.js"}'

The compile phase of Clojurescript looks for things to compile by accumulating anything it finds in :import :require :use :require-macros :use-macros. However if it can't find those dependencies it is not considered an error and this is important to preserve because those dependencies can be provided by JavaScript. Clojurescript makes no distinction between import and require.

Solution
==

(1) If there are unprovided dependencies after get-dependencies, let the user know:
closure.clj:add-dependencies
provided (mapcat :provides (concat required-js required-cljs))
unprovided (clojure.set/difference (set requires) (set provided))] (when unprovided
(apply println "ERROR, required namespace never provided for" (sort unprovided)))

(2) Build the JavaScript index prior to cljs compilation, and use it to determine if a require is not provided by either JS or CLJS
analyzer.clj:analyze-deps

(3) A useful warning for :undeclared-ns-form was hidden due to typos in the 'type' checking. 'type' checking outside of (warning ) was redundant as the (warning ) handler checks if that 'type' is enabled. External checks removed.

(4) no-warn was used for subdeps in analyze-deps
It seems this will just hide real issues.

(5) There was an opts flag :warnings which would disable :undeclared-var :undeclared-ns :undeclared-ns-form
when undefined. Tools like leincljsbuild do not enable it, and I can't find the option documented or recommended. This patch removes the :warnings option in favor of reporting them. Obviously this means you see more warnings reported now. They are real warnings. I recommend a separate ticket/patch to expose and document warning controlling options (:warnings should control all warnings, not just undeclared - and there should be the ability to turn on/off individual or sets of warnings.) Also (repl) function takes a "warn-on-undeclared" optional key which if missing will disable error checking - I think I should take this out or default it to true when not present.

Some discussion here:
https://groups.google.com/forum/#!topic/clojurescript/7vaEETMW5xg



 Comments   
Comment by David Nolen [ 02/Jan/14 1:00 PM ]

This looks nice and comprehensive, but it will take some time for me to review. Will try to give it a look over the weekend.

Comment by Timothy Pratley [ 03/Jan/14 3:40 PM ]

O.K. great
Some things to consider:

a) Many more warnings are displayed now. To me that is a very good thing. It might be a shock to some users with existing codebases. Things like:
(JSON/stringify x) [should be (js/JSON.stringify x)]
canvas/offsetWidth [should be (.-offsetWidth canvas)]

b) Warnings are propagated from libraries e.g.:
WARNING: clone already refers to: /clone being replaced by: domina/clone at line 147
Again to me this is a very good thing, but it might shock users.

c) Behavior of (1) does not work well with cljsbuild. It is fine for a clean build, but with multiple dependencies and only touching one file, cljsbuild only recompiled that particular file and reports the other dependencies as missing. I plan to investigate further. We don't really need to post check dependencies provided (2) is acceptable and could just remove (1).

I think we will need to discuss further on turning on/off warnings, how that should be exposed to users, and what the default behavior should be.

Comment by Timothy Pratley [ 03/Jan/14 10:24 PM ]

building a directory results in spurious require warnings - investigating further

Comment by Timothy Pratley [ 05/Jan/14 2:42 AM ]

Additional changes to address directory build warnings

Comment by David Nolen [ 07/Jan/14 7:11 AM ]

Is CLJS-740 also addressed by this patch?

Comment by Timothy Pratley [ 07/Jan/14 10:27 AM ]

Yes, that is correct.

Comment by Timothy Pratley [ 15/Jan/14 10:58 AM ]

Hi David, have you had a chance to look at the patch? Let me know if it requires any adjustments.

Comment by David Nolen [ 15/Jan/14 11:47 AM ]

I will review but honestly what we need are some testers besides myself.

Comment by David Nolen [ 15/Jan/14 1:06 PM ]

Ok I reviewed the patch. It looks good however I cannot apply it with `git am`.

Comment by David Nolen [ 17/Jan/14 9:21 AM ]

The problem with the patch is that's not plain text. I copied to a file on a machine to fix that. I applied it and I now get a series of warnings when running the tests. These need to be addressed by the patch. In addition when I run the ClojureScript REPL I get many many errors.

Comment by David Nolen [ 17/Jan/14 9:26 AM ]

I've gone and applied CLJS-740 to master as it's a simpler fix for an immediate issue. Let's rebase this one and address the issues. Thanks!

Comment by Timothy Pratley [ 03/Feb/14 12:39 AM ]

Attaching rebased patch which resolves tests emitting warnings and repl not launching.

Comment by David Nolen [ 03/Feb/14 8:22 AM ]

With the patch applied running the tests still emit warnings for goog.string, the constants table, and line 1956 in the core_test.cljs. These need to be addressed.

Comment by Timothy Pratley [ 16/Feb/14 2:20 PM ]

./scripts/test warnings suppressed. Not sure why this patch doesn't have the previous patch included in it, presumably I need to do something to combine them.

Comment by David Nolen [ 16/Feb/14 2:43 PM ]

High level question - what is the logic for the suppression of the warnings? Also do I need to apply two patches?

Comment by Timothy Pratley [ 16/Feb/14 5:33 PM ]

Hi David, the warnings around goog.string were suppressed by requiring goog.string in test_core. This is necessary because the with-out-str macro expands to goog.string/stuff. In one sense this seems correct in that code relying on good.string should require it, but in another sense feels bad because using core should not require other stuff. Alternatively if there is a list of things we know we need like goog.string perhaps it should just be treated as known. In fact for the constants table dependency I have to treat it as known because it is compiler internal. For the line 1956 there was a self-referential def form, and so the warning seemed appropriate. To silence that one I added an empty def before the self-referential def.

I think the reason why there are two patches is I must have applied the original to my master to check it, I'll post a fresh one soon as I sort it out.

Comment by David Nolen [ 16/Feb/14 5:39 PM ]

Ok, I think the preferred solution is to make any Google Closure we import/require in core.cljs to be implicit so we don't have to do this in test_core.cljs as you've done.

Comment by Timothy Pratley [ 16/Feb/14 5:44 PM ]

Single file patch

Comment by Timothy Pratley [ 16/Feb/14 5:46 PM ]

Ok sure no problem will change that

Comment by Timothy Pratley [ 17/Feb/14 1:17 AM ]

Added goog.string to the special list in confirm-ns that do not need to be required, as with-out-str macro expands to that.

Comment by David Nolen [ 17/Feb/14 8:24 AM ]

Patch looks good but could we please get a squashed patch? Thanks.

Comment by Timothy Pratley [ 17/Feb/14 11:24 PM ]

Squashed patch attached

Comment by David Nolen [ 17/Feb/14 11:40 PM ]

fixed https://github.com/clojure/clojurescript/commit/e16a3dae7a63d1516f30cbeb065afeef6fbfb484

Comment by David Nolen [ 17/Feb/14 11:40 PM ]

Awesome patch many thanks!

Comment by Chas Emerick [ 18/Feb/14 7:44 AM ]

The addition of :js-dependency-index to the compiler environment raises a minor complication that I'd like to address; see CLJS-770.

Comment by Gary Trakhman [ 22/Feb/14 12:20 AM ]

I made a note on github, and I'll repeat it here.

The patch in question causes errors when the rhino repl is initialized:

I saw the behavior first by using piggieback, then verified it with the test that was disabled in this commit:
https://github.com/clojure/clojurescript/commit/1b3e1c6168cba66c12749f5bcb6747ef0b8a2950

Reverting the patch in this ticket fixed the problem.

Comment by Gary Trakhman [ 22/Feb/14 12:31 AM ]

I also verified that reverting my patch from #764 has no effect.

Comment by Chas Emerick [ 22/Feb/14 8:17 AM ]

Gary, what are the errors you see? I know Austin won't work because of what's described in CLJS-770, but perhaps there's more going on?

Comment by Gary Trakhman [ 22/Feb/14 4:00 PM ]

the workaround in CLJS-770 seems to fix the problem I'm seeing.

Made my test do this, and it passes:
(doto (env/default-compiler-env)
(swap! assoc :js-dependency-index (cljs.closure/js-dependency-index {})))

Generated at Fri Jul 25 11:16:55 CDT 2014 using JIRA 4.4#649-r158309.