Clojure

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

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.5
  • Fix Version/s: Release 1.8
  • Component/s: None
  • Environment:
    OS X
  • Patch:
    Code and Test
  • Approval:
    Incomplete

Description

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

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Can't refer to qualified var that doesn't exist, compiling:(NO_SOURCE_PATH:1:1)

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

  1. clj-1400-2.diff
    31/Aug/14 3:53 PM
    2 kB
    Scott Bale
  2. clj-1400-3.diff
    09/Sep/14 9:29 AM
    2 kB
    Alex Miller
  3. clj-1400-4.diff
    11/Sep/14 11:22 PM
    2 kB
    Scott Bale

Activity

Hide
Scott Bale added a comment -

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

Show
Scott Bale added a comment - This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.
Hide
Scott Bale added a comment -

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)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Show
Scott Bale added a comment - 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)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not. [footnote]
public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Hide
Scott Bale added a comment -

patch: code and test

Show
Scott Bale added a comment - patch: code and test
Hide
Scott Bale added a comment -

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)
Show
Scott Bale added a comment - 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)
Hide
Andy Fingerhut added a comment -

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

Show
Andy Fingerhut added a comment - 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
Hide
Scott Bale added a comment - - edited

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

Show
Scott Bale added a comment - - edited Attached is an updated patch: "clj-1400-2.diff". I removed the stale patch.
Hide
Alex Miller added a comment -

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.
Show
Alex Miller added a comment - 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.
Hide
Scott Bale added a comment - - edited

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)
Show
Scott Bale added a comment - - edited 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)
Hide
Scott Bale added a comment -

(properly named patch)

Show
Scott Bale added a comment - (properly named patch)
Hide
Alex Miller added a comment -

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

Show
Alex Miller added a comment - You could throw a CompilerException with the location of the problem instead (as the ticket description suggests).
Hide
Scott Bale added a comment -

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.

Show
Scott Bale added a comment - 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.
Hide
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated: