Error "Can't refer to qualified var that doesn't exist" should name the bad symbol

Question

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1) Syntax error compiling def at (REPL:1:1). Can't create defs outside of current ns

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch: clj-1400-4.diff

Screened by: Alex Miller

Answer 1

Comment made by: scottbale

This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.

Answer 2

Comment made by: scottbale

Patch {{clj-1400-1.diff}} to {{Compiler.java}}.

With this patch the example would now look like:

user> (def foo/bar 1) CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

I'm not sure the {{if(namesStaticMember(sym))}} (link: see below), and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

(link: footnote)

`
public static boolean namesStaticMember(Symbol sym){

return sym.ns != null && namespaceFor(sym) == null;

}
`

Answer 3

Comment made by: scottbale

patch: code and test

Answer 4

Comment made by: scottbale

I tested on an actual source file, and the exception message included the file/line/col info as desired:

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)

Answer 5

Comment made by: jafingerhut

Patch clj-1400-1.diff dated Jun 26 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing Patches

Answer 6

Comment made by: scottbale

Attached is an updated patch: "clj-1400-2.diff". I removed the stale patch.

Answer 7

Comment made by: alexmiller

Few comments to address:
- Compiler diff was using spaces, not tabs, which makes it harder to diff. I attached a -3.diff that fixes this.
- the call to namesStaticMember seems weird. The name of that method is confusing for this use. Beyond that, I think it's doing more than you need. That method is going to attempt resolve the qualified name in terms of the current ns, but I think you don't even want to do that. Rather you just want to know if the sym has a ns (sym.ns != null) - isn't that enough?
- In what case will the other error "Var doesn't exist" occur? In other words, in what case will lookupVar not succeed in creating a new var here? If there is no such case, then remove this case. If there is such a case, then add a test.

Answer 8

Comment made by: scottbale

Agree with all three of your bullets. Attached is an updated patch, clj-1400-4.diff.
I used tabs in Compiler.java
After close inspection of call to lookupVar(...), I believe null is returned only in the case of exactly this ticket (the symbol having a non-null namespace which has not been loaded yet). So I've taken out the conditional and the 2nd branch.
* (Test is unchanged)

Answer 9

Comment made by: scottbale

(properly named patch)

Answer 10

Comment made by: alexmiller

You could throw a CompilerException with the location of the problem instead (as the ticket description suggests).

Answer 11

Comment made by: scottbale

Sorry, I should've mentioned because this wasn't obvious to me either (and in fact I forgot until just now): the RuntimeException is already caught and wrapped in a CompilerException.

I'm not sure which try-catch block within Compiler.java this is happening in, there are multiple. But you can see in the output that the exception is a CompilerException and the file|line|col info is there:

In the Repl...

user> (def foo/bar 1) CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

...or in a source file

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)

Also, at the point at which the RuntimeException of this patch is being thrown, the {{source}} {{line}} and {{col}} params to CompilerException are not available, or at least not afaict.

Answer 12

Comment made by: alexmiller

I'll follow up on this patch later - Rich thought it was making too many assumptions. I think we validated many of those but need to double-check those.

Answer 13

Comment made by: alexmiller

Updated error result as of Clojure 1.10. It is now throwing a :compile-syntax error phase exception with file/line/col too.

Answer 14

Reference: https://clojure.atlassian.net/browse/CLJ-1400 (reported by hlewisship)