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

Replace apply with unpacking operators #1325

Merged
merged 6 commits into from
Jul 20, 2017
Merged

Conversation

Kodiologist
Copy link
Member

@Kodiologist Kodiologist commented Jul 16, 2017

It's possible to provoke a wide variety of cryptic error messages by misusing #* and #**, but I'm not sure it's worth trying to beautify all those possible errors in this pull request.

@Kodiologist
Copy link
Member Author

Oh great, Travis is showing that it segfaults on 3.3 and 3.4 for some reason. Probably because PEP 448 only made it into 3.5. I'll gate those tests appropriately and see what happens.

@Kodiologist
Copy link
Member Author

Kodiologist commented Jul 16, 2017

Nope. I guess this won't be so easy. There were probably changes to Python's expectations for the AST.

But please do take a look at this before I start hacking away at that.

@gilch
Copy link
Member

gilch commented Jul 16, 2017

Having written three of those four issues, I approve of the additional syntax. However, I didn't say anything about removing apply. But now that I think about it, if it's made redundant then it should be removed. It would certainly be easy to re-implement in terms of #* and #**, but when would we even need to?

To answer my own question, perhaps in macros and higher-order functions? Would we ever need the old apply in -> or doto? Is there a non-reader syntax for these special forms, like quote is for '? Sometimes it's easier if you don't need the reader syntax when writing macros, at least in the case of quote. Our current apply doesn't work as a HOF anyway, but maybe it should be shadowed. I might need to think about this some more.

One other issue I take with the proposed implementation is how you lex the tokens. A tag macro can be any symbol, yet the tests demonstrate (setv [a #*b c] "ghijklmno") would apply the unpacking operator to b. But what happens if you have a tag macro with symbol *b? Would it apply to c or just be unusable?

I think whitespace should be required between a #* and a symbol. But it should be fine to put it directly before a list with no space #*[..., likewise, a #** would work on a dict display with no space #**{..., but not before a symbol.

@Kodiologist
Copy link
Member Author

Is there a non-reader syntax for these special forms, like quote is for '?

Yes, #* foo is just shorthand for (unpack-iterable foo), and #** foo for (unpack-mapping foo).

A tag macro can be any symbol

Not if you want to be able to call it. Here are two examples:

=> (deftag "{b" [x] "hello")
<function <lambda> at 0x7f579c0d2d08>
=> (setv b "foo")
=> (print #{b 1 2})
{1, 2, 'foo'}
=> (deftag "[b" [x] "hello")
<function <lambda> at 0x7f913d8e0048>
=> (print #[b 1 2])
[…] NameError: name '#' is not defined

So the effect of this change is that a tag macro's name can't begin with an asterisk, the same way it can't begin with { or [. Because such a macro would look like unpacking, it's a bad idea anyway.

@Kodiologist
Copy link
Member Author

But it should be fine to put it directly before a list with no space #*[..., likewise, a #** would work on a dict display #**{..., but not before a symbol.

Beyond testing Hy itself, why would you construct a literal list or dictionary only to unpack it?

@gilch
Copy link
Member

gilch commented Jul 16, 2017

Beyond testing Hy itself, why would you construct a literal list or dictionary only to unpack it?

I'm saying it should be valid syntactically, not that I had a use case in mind. But think about how this could interact with the threading macros -> and ->>. Could you thread in multiple arguments at once? Macros can do interesting things.

In the case of a dictionary, even in Python, you can use dictionary unpacking to put the kwarg name in a variable.

>>> def bar(*, a=None, b=None):
	print(a,b)

	
>>> foo = 'a'
>>> bar(**{foo: 1})
1 None
>>> foo = 'b'
>>> bar(**{foo: 1})
None 1

@Kodiologist
Copy link
Member Author

Kodiologist commented Jul 16, 2017

In the case of a dictionary, even in Python, you can use dictionary unpacking to put the kwarg name in a variable.

Oh, that's a good point.

@gilch
Copy link
Member

gilch commented Jul 16, 2017

Not if you want to be able to call it.

I said any symbol, not any string. I meant the kind of symbol you can type in directly, without going through a HySymbol call to convert a string. Those can't contain things like [ or {. See also #1117.

We do have a *map symbol in core. We also have the *earmuffs* convention. It's not that weird for a symbol to start with an asterisk in Lisp.

@Kodiologist
Copy link
Member Author

Not if you want to be able to call it.

I said any symbol, not any string. I meant the kind of symbol you can type in directly, without going through a HySymbol call to convert a string.

Okay, here's another counterexample:

=> (deftag "!a" [x] "hello")
<function <lambda> at 0x7f83b0b8ef28>
=> #!a 3
[Returns None]
=> (repr #!a 3)
  File "<stdin>", line 1, column 7

  (repr #!a 3)
        ^
LexException: Ran into a HASHBANG where it wasn't expected.

@gilch
Copy link
Member

gilch commented Jul 16, 2017

HASHBANG

Yikes! I'd call that one that a bug in the lexer. I think we were only using that for the shebang on the first line? We should be able to use #! as a tag macro elsewhere.

@Kodiologist
Copy link
Member Author

So, to be clear, you want #*b to be parsed as calling a tag macro named b instead of the same way as #* b?

@gilch
Copy link
Member

gilch commented Jul 17, 2017

It seems much more consistent to me to implement #* and #** as tag macros that happen to expand into the new special forms unpack-iterable, and unpack-mapping, respectively, without messing with the current lexer at all. (I mean, besides fixing that HASHBANG thing in a separate issue.)

@gilch
Copy link
Member

gilch commented Jul 17, 2017

#* and #** could be implemented like this:

(deftag * [form] `(unpack-iterable ~form))
(deftag ** [form] `(unpack-mapping ~form))

Then you don't need the special case in the compiler, and don't have to change the lexer. You still need the new special forms.

So, to be clear, you want #*b to be parsed as calling a tag macro named b instead of the same way as #* b?

No, #*b should be a tag macro named *b. So #*b c calls the *b tag macro with argument c. But #* b c calls the * tag macro with argument b.

@Kodiologist
Copy link
Member Author

All right, that makes sense.

@Kodiologist
Copy link
Member Author

Kodiologist commented Jul 17, 2017

Okay, Pythons 3.3 and 3.4 should work now. However, I learned the hard way that the #* and #** syntax can't be enabled with tag macros alone. The implementation of unpacking requires peeking into the not-yet-compiled HyModel for (unpack_iterable …) and (unpack_mapping …) forms, and #* x appears as (dispatch_tag_macro "*" x) at that stage. So we'd need to treat these particular dispatch_tag_macro forms as magical, which seems unwise.

@gilch
Copy link
Member

gilch commented Jul 17, 2017

can't be enabled with tag macros alone.

It had better, or I don't think we can do it. This setup is causing errors in macros.

=> (defmacro star [form] `(unpack-iterable ~form))
  File "<input>", line 1, column 41

  (defmacro star [form] `(unpack-iterable ~form))
                                          ^----^
HyTypeError: b"`unquote' can't be used at the top-level"

That is not supposed to happen. Quasiquote is broken. Let's try that manually.

=> (defmacro star [form] ('unpack-iterable form))
import hy
from hy import HySymbol
hy.macros.macro('star')((lambda form: HySymbol('unpack_iterable')(form)))
<function <lambda> at 0x000001F921BE81E0>
=> (setv [a (star b) c] [1 2 3 4])
  File "<input>", line 1, column 10

  (setv [a (star b) c] [1 2 3 4])
           ^------^
HyMacroExpansionError: b'expanding `star\': TypeError("\'HySymbol\' object is not callable",)'

Macros are supposed to recursively expand before special forms are applied. What's going on here?

@Kodiologist
Copy link
Member Author

b"`unquote' can't be used at the top-level"

You get this error because quasiquote is getting interpreted as a function. See hy.compiler line 1960.

Note that it would do you no good for me to add quote or quasiquote as an exception to the rule that unpack_iterable and unpack_mapping cause their parent form to be interpreted as a function, because (star x) still wouldn't get expanded early enough.

Macros are supposed to recursively expand before special forms are applied.

That isn't quite true. A macro is only expanded once compile_expression gets to it (the first statement of compile_expression is expression = macroexpand(expression, self)). So when a form is being compiled that contains a macro call as one of its children (direct or indirect), the macro hasn't been expanded yet.

@gilch
Copy link
Member

gilch commented Jul 17, 2017

I did the manual build wrong.

Let's try an explicit Hy model.

=> (defmacro star [form] (HyExpression ['unpack-iterable form]))
import hy
from hy import HySymbol
hy.macros.macro('star')((lambda form: HyExpression([HySymbol('unpack_iterable'), form])))
<function <lambda> at 0x000001F921BE81E0>
=> (setv [a (star b) c] [1 2 3 4])
[a, *b, c] = [1, 2, 3, 4]
None

That worked.

Breaking quasiquote is not okay though. I know some macros in Common Lisp will recursively macroexpand their body before acting on it. We might need that feature to implement this properly, unless we can come up with a better idea.

@gilch
Copy link
Member

gilch commented Jul 17, 2017

Hey, this works too.

=> (deftag star [form] (HyExpression ['unpack-iterable form]))
import hy
from hy import HySymbol
hy.macros.tag('star')((lambda form: HyExpression([HySymbol('unpack_iterable'), form])))
<function <lambda> at 0x000001F921BE8510>
=> (setv [a #star b c] [1 2 3 4])
[a, *b, c] = [1, 2, 3, 4]
None
=> [1 2 #star[3 4] 5 6]
[1, 2, *[3, 4], 5, 6]

Are you sure we can't implement it as a tag macro? Which part is failing?

@Kodiologist
Copy link
Member Author

Kodiologist commented Jul 17, 2017

That worked.

Not in general. The case you tried doesn't require the compiler to see the unpack_iterable in advance. Here's an example with Python 2:

hy 0.13.0+39.g38ef9c2 using CPython(default) 2.7.12+ on Linux
=> (defmacro star [form] (import hy) (hy.HyExpression ['unpack-iterable form]))
<function _hy_anon_fn_1 at 0x7f30b058b5f0>
=> (f (star [1 2 3]))
  File "<input>", line 1, column 4

  (f (star [1 2 3]))
     ^------------^
HyTypeError: `unpack-iterable` isn't allowed here

The compiler sees star instead of unpack_iterable, so it doesn't set oldpy_starargs in _compile_collect.

Breaking quasiquote is not okay though.

It's not broken. Quasiquote can't do what you want it to do here because of Hy's basic strategy for compilation. The most that can be done is to produce a more informative error message.

I know some macros in Common Lisp will recursively macroexpand their body before acting on it

That's out of scope for this PR.

Are you sure we can't implement it as a tag macro?

Yes, nor with a regular macro.

Which part is failing?

Any part that requires looking at child forms. Check where I've used the function is_unpack in compiler.hy.

@Kodiologist
Copy link
Member Author

Also, I pushed https://github.com/Kodiologist/hy/tree/unpacking-tag-macro if you want to experiment with my attempt.

@gilch
Copy link
Member

gilch commented Jul 17, 2017

More weirdness.

=> {1 2 #**{3 4}}
{1: 2, None: {3: 4, }, }
{1: 2, 3: 4}

Why does the Python translation have a None key? I would have expected {1: 2, **{3: 4, }, }.

@Kodiologist
Copy link
Member Author

I got that far by treating the dispatch_tag_macro forms as magic by making is_unpack detect them. You could make this code work by also having the compiler bits that use is_unpack extract item 2 instead of item 1 when we've detected dispatch_tag_macro rather than is_unpack. The point is, you can't do it without magic in compiler.hy.

@Kodiologist
Copy link
Member Author

Kodiologist commented Jul 17, 2017

Why does the Python translation have a None key?

I noticed that, too. I think it's a bug (or rather, a missing feature) in astor's code generation. The way the Python 3.5 and 3.6 AST represents dictionary unpacking is indeed with a None key, although it doesn't work if you write {1: 2, None: {3: 4, }} as Python code, because the None has to be an unboxed None value instead of an identifier named None. It's pretty confusing.

@gilch
Copy link
Member

gilch commented Jul 17, 2017

But not 2.7? @berkerpeksag are we doing something wrong with astor to get these None keys? I thought we weren't supporting 2.6 anymore?

@Kodiologist
Copy link
Member Author

Sorry, that was a typo for 3.5 and 3.6. Fixed.

@Kodiologist
Copy link
Member Author

To answer the obvious question, in earlier Pythons, dictionary unpacking is represented by passing the kwargs argument to ast.Call.

@gilch
Copy link
Member

gilch commented Jul 17, 2017

Python code, because the None has to be an unboxed None value instead of an identifier named None. It's pretty confusing.

Examples work better.

=> (ast.dump (compile "{None:1,**{1:2},**{3:4}}" "<str>" "eval" :flags ast.PyCF-ONLY-AST))
ast.dump(compile('{None:1,**{1:2},**{3:4}}', '<str>', 'eval', flags=ast.PyCF_ONLY_AST))
'Expression(body=Dict(keys=[NameConstant(value=None), None, None], values=[Num(n=1), Dict(keys=[Num(n=1)], values=[Num(n=2)]), Dict(keys=[Num(n=3)], values=[Num(n=4)])]))'

I think I get it now. The NameConstant(value=None) is for an actual None key, but a plain None means "unpack it". It's unambiguous, if confusing. This seems like a bug in astor.

@gilch
Copy link
Member

gilch commented Jul 18, 2017

Not in general. The case you tried doesn't require the compiler to see the unpack_iterable in advance.

One of the problem cases was unpacking a mapping into a dict display. I've been playing around with this a bit. It's possible to set an arbitrary attr flag in the ast itself. This seems to work in Python 3.6.

    @builds(HyDict)
    def compile_dict(self, m):
        keyvalues, ret, _ = self._compile_collect(m)

        temp = []
        for kv in keyvalues:
            if hasattr(kv, "unpack_mapping"):
                temp.extend((None, kv))
            else:
                temp.append(kv)
        keyvalues = temp

        ret += ast.Dict(lineno=m.start_line,
                        col_offset=m.start_column,
                        keys=keyvalues[::2],
                        values=keyvalues[1::2])
        return ret

    @builds("unpack_mapping")
    @checkargs(exact=1)
    def compile_unpack_mapping(self, expr):
        ret = self.compile(expr[1])
        ret.expr.unpack_mapping = True
        return ret

Older ast versions will have to build it differently, but it seems like it should work.
There's no error checking--an unpack-mapping form where it doesn't belong is simply ignored.
But error checking should be possible too. I still think it's possible to implement the operators as tag macros.

@Kodiologist
Copy link
Member Author

Kodiologist commented Jul 18, 2017

Well done. But, making it work for all uses of the unpacking operator will require rejiggering the order in which child forms are compiled in several places in the compiler. Where the compiler currently inspects the HyModels of child forms and compiles them after, you'll have to compile them first and then inspect the Python AST. As an analogy, the following doesn't work

=> (defmacro m [] :hello)
<function <lambda> at 0x7ff1ed3838c8>
=> (dict (m) 3)
TypeError: dict expected at most 1 arguments, got 2

because the compiler looks for a literal HyKeyword when it's compiling (dict …) and before it's gotten to compiling (m).

In all, it seems like a lot of work for the mere bragging rights of being able to say that we implemented #* and #** as tag macros. You realize that the easiest way to re-enable tag macros whose names start with * is just to make my HASHSTARS regex require whitespace (or other non-identifier character) at the end, or to make HASHOTHER specifically exclude #* and #**, right?

@gilch
Copy link
Member

gilch commented Jul 18, 2017

As an analogy, the following doesn't work

I feel like that would work in other Lisps. Hy makes an unusual distinction between quoted and unquoted keywords, but only in function calls. So that's like calling (dict ':hello 3), but in other Lisps (and elsewhere in Hy), keywords always count as quoted.

This is a wart in Hy, but I don't know that we can fix it syntactically to make it always quoted, given how Python works. (Given (foo a :b c) should that keyword be the next-to-last positional argument, or a keyword argument? But with the distinction, (foo a ':b c), is unambiguously the former.)

The fact that that macro doesn't work bothers me, but with unpacking, I could mostly work around it. This might be something to fix in the compiler later.

rejiggering the order in which child forms are compiled in several places

I'm not clear on which parts aren't working. Where are the operators used in Python?

Unpack for mappings is used in

  • function definitions. Hy function definitions already use the &-word syntax.
  • dict displays. I think I got that working above.
  • function calls. I think it could be done the same way?

Was that all of them? That's one we can implement as a tag macro, at least.

Unpack for iterables is used in

  • function definitions. Hy has &rest.
  • tuple and list displays.
  • function calls.
  • assignments. Doesn't this case already work?

Was that all of them?

In all, it seems like a lot of work for the mere bragging rights

I don't see it as mere bragging rights.

First, it would keep those special cases out of the lexer, which should be kept as simple as is reasonable. A DSL could use #* and #** tags for something else. We might want to upgrade tag macros to true reader macros or Clojure-like tagged literals some time.

Second, and more importantly, I want them to work properly as special forms even without the tags. The new special forms are not well-behaved now. They break quasiquote, and probably break in macros in ways a user would find hard to predict. If they worked consistently, they could be (tag) macros.

@Kodiologist
Copy link
Member Author

Was that all of them?

Yes, that's everything I implemented.

First, it would keep those special cases out of the lexer, which should be kept as simple as is reasonable. A DSL could use #* and #** tags for something else.

Redefining a core macro seems like a really bad idea. Python lets you assign something to list, but that's a mistake, in my mind.

We might want to upgrade tag macros to true reader macros or Clojure-like tagged literals some time.

So we'll cross that bridge when we come to it.

The new special forms are not well-behaved now. They break quasiquote, and probably break in macros in ways a user would find hard to predict

But they are well-behaved and work consistently with the way the rest of the compiler works. For example, look at all the instances in compiler.hy of isinstance(something, HySomething).

If you want to change the compiler in deep ways, I'm certainly open to such a PR, but again, it's out of scope for this PR.

@gilch
Copy link
Member

gilch commented Jul 19, 2017

Redefining a core macro seems like a really bad idea.

Ha. Now which of us is illiberal? (1246). Yes, this could absolutely be abused and one should be careful. But see the defn redefinition in contrib. That's a core macro name, but calling it that was my idea, and I think it was a good one. #1050. I do think redefinition of those tags should be allowed, if frowned upon without a compelling need and explicit documentation.

@Kodiologist
Copy link
Member Author

All right, this is ready for review now.

@refi64 refi64 merged commit 35f7dd3 into hylang:master Jul 20, 2017
@refi64
Copy link
Contributor

refi64 commented Jul 20, 2017

(FWIW I wonder if we can use the same syntax ideas to implement tuple unpacking such...)

@refi64
Copy link
Contributor

refi64 commented Jul 20, 2017

and such

@Kodiologist
Copy link
Member Author

But tuple unpacking is among what's implemented here. Or did you mean something other than Python's one-star unpacking?

@Kodiologist Kodiologist deleted the unpacking branch August 27, 2017 21:37
zackmdavis added a commit to zackmdavis/HyTimer that referenced this pull request Mar 15, 2018
I guesss I forgot to `pip install --upgrade` last night; `apply` went away in
hylang/hy#1325.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants