Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Macros do not like unpack-mapping #1730

Closed
Quelklef opened this issue Jan 15, 2019 · 24 comments · Fixed by #2283
Closed

Macros do not like unpack-mapping #1730

Quelklef opened this issue Jan 15, 2019 · 24 comments · Fixed by #2283

Comments

@Quelklef
Copy link
Contributor

Quelklef commented Jan 15, 2019

(defmacro id [x] x)
(print #** {})  ; no problem
(print (id #** {}))  ; error

;   File "<input>", line 1, column 12
; 
;   (print (id #** {}))
;              ^----^
; HyTypeError: The special form 'unpack-mapping' is not allowed here

I tried to test this on devel, but devel wouldn't compile 😐. Perhaps I'm doing something wrong; not sure.

@Kodiologist
Copy link
Member

How would you expect this to work, if it did work? Imagine a less trivial case, like (defmacro m [a b c] (+ a b c)) (m #* l).

@Quelklef
Copy link
Contributor Author

(m #* 1) would be (m (unpack-mapping 1)) which only satisfies the a argument. That's how I expected it to work, anyway, so (print (id #** {})) would be the same as (print #** {}).

@Quelklef
Copy link
Contributor Author

I expected this because AFAICT #** x always desugars to the special form (unpack-mapping x). All other special forms I've seen seem to act this way with macros--which is to say not at all--so I expected this one to as well.

@Kodiologist
Copy link
Member

That's even weirder than I expected, but to be clear, I was using a lowercase letter L there. (Consider switching monospace fonts.) In other words, what would (m #* l) do?

@Quelklef
Copy link
Contributor Author

(m #* l) would do nothing because #* l satisfies the a argument but b and c are left unspecified.

@Kodiologist
Copy link
Member

I presume you meant that (m #* l) would be an error, then, since the macro call would be missing two required arguments. In any case, it sounds like you want the unpacking forms passed in as single arguments, not for any actual unpacking to occur at macro-expansion time. But this is exactly what already happens. (id #** {}) expands to #** {}, which is illegal at the top level, as the error message indicates. Look at (macroexpand '(id #** {})).

@Quelklef
Copy link
Contributor Author

which is illegal at the top level

Excuse me if this is dumb, but isn't it not at top-level because it's nested in the print call?

@Kodiologist
Copy link
Member

Hmm, that's a good point. By contrast, (defmacro foop [x] `(print ~x)) (foop #** {}) does what you probably expected. I think it has to do with how support for #** is implemented (and, before Python 3.5, #*). In order for (print #** x) to work, the compiler checks the arguments for unpacking forms while compiling the print form (in _compile_collect). By the time id has expanded, it's too late for _compile_collect to see the unpacking form. I don't think there's a way around this short of changing Hy's macro-expansion semantics to be inner-first instead of outer-first.

@Kodiologist
Copy link
Member

Either that, or some kind of multi-pass compilation.

@Quelklef
Copy link
Contributor Author

Would it not be possible to have arguments be compiled not in sequential order but macro-first?

@Kodiologist
Copy link
Member

You mean, to have the compiler expand all macros before compiling special forms? Perhaps, although you'd need to make an exception for special forms that are needed to collect macros.

@Quelklef
Copy link
Contributor Author

I think I meant something that doesn't make sense.

After squinting at the code for a bit, it seems that it would be sufficient to expand only one level extra--just arguments to functions, if those arguments are macro calls--rather than all the way.

Related, am I correct in saying that Hy does "no" evaluation? Hy expressions are compiled to the Python AST which is then evaluated? So it wouldn't make sense to have the checks for unpack- at invocation time.

@Kodiologist
Copy link
Member

Related, am I correct in saying that Hy does "no" evaluation? Hy expressions are compiled to the Python AST which is then evaluated?

Mostly, yeah, but eval-when-compile and eval-and-compile are important exceptions, and they're used to implement macros.

@Kodiologist
Copy link
Member

Maybe changing this is as simple as making sure the loop in _compile_collect tries to compile each expression before checking whether it's an unpacking form (is_unpack("mapping", expr)). But an adjustment to compile_expression might be necessary, too, to allow the unpacking form.

@Kodiologist
Copy link
Member

It's still not totally clear to me that this is a bug.

@allison-casey
Copy link
Contributor

can macros even use keywords to begin with? You can define a macro with keyword args in the lambda list, but as far as i can see there's no way to actually take advantage of that because the keywords are taken literally as positional arguments

=> (defmacro something [[x 1]] x)
=> (something :x 2)
hy.errors.HyMacroExpansionError:
  File "<string>", line 1
    (something :x 2)
    ^--------------^
expanding macro something
  TypeError: something() takes from 1 to 2 positional arguments but 3 were given

@Kodiologist
Copy link
Member

Arguments in macros have names, like any function arguments, but you can't use / or #** because, as you say, "the keywords are taken literally as positional arguments". (The fact that a lambda list like [#* a [b 1]] is allowed is a bug.) The idea is that the user should be free to decide how a macro treats keywords, so if you want function-style keyword arguments, you have to do that as an extra step.

@allison-casey
Copy link
Contributor

So it sounds like we need to fix the lambda list parser for macros to make this explicit. In which case this wouldn't be a bug like you said, more a failure by us to telegraph that macros can't use keyword args and by extension #**

@Kodiologist
Copy link
Member

I think the only fix needed is to forbid additional arguments after #* args. #** and * are already forbidden.

@allison-casey
Copy link
Contributor

allison-casey commented Apr 21, 2022

the fact that macros can define default arguments, but there's no way to call them in any way other than positionally definitely seems like a doc me issue. So in terms of action items it's:

  1. fix defmacro lambda list parsing to dissallow keyword only arguments after #* args
  2. document that macros can define arguments with default values, but those default args can only be called positionally. i.e.
=> (defmacro something [[a 1] [b 2] #* body]
...  `[~a ~b ~body])
=> (something 2 3 :hello)
[2 3 #(:hello)]

;; trying to use keyword arguments to pass arguments out of order leads
;; to unexpected results
=> (something :b 1 :a 2 :world)
[:b 1 #(:a 2 :world)]
;; or trying to skip a default argument does not do what you might expect
=> (something :b 1 :world)
[:b 1 #(:world)]

@allison-casey
Copy link
Contributor

actually, given that default argument behavior in macros are so different than regular functions and prone to unexpected behavior, does it make sense that macros can define default args in the lambda list at all? It seems like if you want to have default args in a macro you should handle that manually in the body

@Kodiologist
Copy link
Member

I don't really agree, in the sense that default arguments in macros work just like default arguments in ordinary functions; what's different is how keywords are handled. (In the old days, before autopromotion, you had to say e.g. (defmacro m [[a 'None]] …) instead of [a None], but that's no longer the case.)

@allison-casey
Copy link
Contributor

it should be documented then that all arguments in macros are position only then. adding defaults to positional arguments acts like an using an implicit / in a regular function def since there's no way to pass keyword arguments when calling a macro.

@Kodiologist
Copy link
Member

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants