Make tc-ignore more do-like when used at the toplevel #4

Closed
wants to merge 1 commit into
from

2 participants

@S11001001

This lets you wrap tc-ignore around a big chunk of the buffer in more situations (e.g. in the included test code).

@frenchy64
Owner

So defprotocol doesn't generate a protocol if in a statement position of a do (inside a tc-ignore)?

@S11001001

I'm surprised it works at all, as you should get (tc-ignore-forms* (do (def x 42) x)), and I didn't think that would work, hence the use of defprotocol in the test at all. I think protocols themselves ended up declared, just not their methods.

@frenchy64
Owner

This smells like a bug in analyze. Or an oddity of the Clojure analyser.

@S11001001

It's a general property of do not appearing at the top-level after expansion. Here I replace tc-ignore-forms* with identity.

user=> (identity (do (defprotocol some-proto (some-proto-method [_]))
  #_=>               some-proto-method))
CompilerException java.lang.RuntimeException: Unable to resolve symbol: some-proto-method in this context, compiling:(NO_SOURCE_PATH:1) 
@frenchy64
Owner

Do you know what's special about the top-level wrt do?

@S11001001

In general, Compiler compiles and evaluates top-level forms one-by-one, so you can, say, use a function in a macro and an expansion of that macro all in the same file (which requires hoop-jumping in Common Lisp). But do is treated specially; its subforms are spliced in and treated individually as top-level forms, just as Common Lisp does with progn.

Compiler seems to have been improved so that defs are spotted anywhere, but that isn't good enough:

user> (do (defmacro xindo [] 42) (xindo))
42
user> (identity (do (defmacro xinid [] 42) (xinid)))
ArityException Wrong number of args (0) passed to: user$eval909$xinid  clojure.lang.AFn.throwArity (AFn.java:437)

The def xinid got recognized, but (. (var xinid) (setMacro)) never got a chance to happen before (xinid) was expanded. Meanwhile, xindo was fully defined and setMacroed by the time (xindo) was expanded.

@frenchy64
Owner

Thanks for the explanation. The patch looks good, will merge.

@frenchy64
Owner

Applied. 9498723

@frenchy64 frenchy64 closed this Jan 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment