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

Look for o!-syms in (flatten args) of defmacro! #1637

Merged
merged 2 commits into from
Jun 25, 2018
Merged

Look for o!-syms in (flatten args) of defmacro! #1637

merged 2 commits into from
Jun 25, 2018

Conversation

oskarkv
Copy link
Contributor

@oskarkv oskarkv commented Jun 11, 2018

Being able to have o!-symbols in lists nested in the arglist of defmacro! seems like a good idea to me. Otherwise one couldn't do things like

(defmacro! if-let [[sym o!expr] then else]
  `(if ~g!expr (let [~sym ~g!expr] ~then) ~else))

If this is not allowed, defmacro! is pretty useless for writing if-let in particular; at least I think so.

The tests didn't pass before I started. :P

@Kodiologist
Copy link
Member

I don't understand your example. I don't know what you expect the lambda list [[sym o!expr] then else] to mean, and when I try to run the example form with your change, I get HyTypeError: parse error for special form 'fn'.

@Kodiologist
Copy link
Member

The tests didn't pass before I started. :P

We're removing Python 3.7 on Travis (#1631), but all other Pythons should still pass.

@oskarkv
Copy link
Contributor Author

oskarkv commented Jun 12, 2018

@Kodiologist I was using a slightly old version of hy, hy-0.14.0+92.g4d98cde, and when I updated to hy-0.14.0+169.g79bd4b0 rply-0.7.6 today, I got the same error as you. Seems to me that something is broken, since now I can't do

=> (defn testf [[a b] c] [a b c])
  File "<input>", line 1, column 13

  (defn testf [[a b] c] [a b c])
              ^-------^
HyTypeError: parse error for special form 'fn*': should have reached end of form: HyList([
  HySymbol('a'),
  HySymbol('b')]): HyList([
  HyList([
    HySymbol('a'),
    HySymbol('b')]),
  HySymbol('c')])

which I could before, and I expected it to work (the [a b] list, of course, means destructuring).

About my example: I don't know which part you don't understand, so let me explain everything. No doubt I will explain something that you already know.

My if-let should work like Clojure's if-let, meaning (if-let [sym expr] then else) should evaluate expr, and if it is truthy, do the then expression, else the else expression. But when evaluating the then expression, sym should be bound to the result of expr. An example would be

(if-let [element (first some-seq)] (print element) (print "empty seq"))

In

(defmacro! if-let [[sym o!expr] then else]
  `(if ~g!expr (let [~sym ~g!expr] ~then) ~else))

[[sym o!expr] then else] intends to destructure the given list, so sym would be element and o!expr would be (first some-seq) in the example above. then and else would of course be (print element) and (print "empty seq").

I use defmacro! and o!expr because of course the expression should only be evaluated once. The implementation from Clojure, for example, is (after removing some assertions and irrelevant stuff):

(defmacro if-let
  "bindings => binding-form test
  If test is true, evaluates then with binding-form bound to the value of 
  test, if not, yields else"
  ([bindings then else]
   (let [form (bindings 0) tst (bindings 1)]
     `(let [temp# ~tst]
        (if temp#
          (let [~form temp#]
            ~then)
          ~else)))))

which is quite a bit longer that my version with defmacro!.

Additionally, without my change, I noticed that not only can one not have o!-syms in destructuring lists in the macro arguments, one can't have destructuring lists at all, since defmacro! tries (.startswith s "o!") on all arguments.

@gilch
Copy link
Member

gilch commented Jun 12, 2018

We've already removed tuple unpacking in lambda lists. #1590.

We might still see nested lists in the defmacro lambda list when using &optional, but this proposal wouldn't distinguish key from value. You should only check for the os in the left side of those pairs.

The new model patterns might be a better option for defmacro anyway. I'm not sure if we still want &optional. &key and &kwonly are already disallowed since they don't make sense.

@Kodiologist
Copy link
Member

Kodiologist commented Jun 12, 2018

Ah, so you meant to do unpacking. That makes sense. But as Matthew points out, unpacking in lambda lists was recently removed, and if you want to look for o-parameters in &optional lists, you should be checking only the first element of each &optional list.

In any case, be sure to add a test for your new feature.

@oskarkv
Copy link
Contributor Author

oskarkv commented Jun 12, 2018

I tried to look at all the issues related to removing tuple unpacking in lambda lists, but I didn't really get what the alternative is supposed to be. Is there currently an alternative or is this a work in progress?

@Kodiologist
Copy link
Member

There's been some work on fancier unpacking (#1328), but nothing has landed yet.

@gilch
Copy link
Member

gilch commented Jun 12, 2018

Is there currently an alternative or is this a work in progress?

Python 2 had this and it was removed in Python 3. We've followed suit, since the implementation would be more complicated in Python 3, we decided this feature belongs in macros build on defn or fn and not in the special forms themselves.

So besides my destructure macros, you can work around this the same way as is recommended in Python: with an assignment at the top of the function. Note that in Hy, fn also has an implicit do. I showed an example of doing this in #1589 after it was closed.

@oskarkv
Copy link
Contributor Author

oskarkv commented Jun 12, 2018

I guess my dream of having o!-parameters in unpacking lists will have to wait.

But since defmacro! currently can't even use regular optional arguments (because it tries to call .startswith on all arguments), maybe adding support for o!-parameters in optional arguments is still good. Admittedly it's hard to come up with an example using optional arguments for macros. Of course, one could fix the regular optional arguments without adding o!-parameter support, but why not add it?

So anyway, what do you think of this?

(defmacro defmacro! [name args &rest body]
  "Like `defmacro/g!`, with automatic once-only evaluation for 'o!' params.

Such 'o!' params are available within `body` as the equivalent 'g!' symbol."
  (defn extract-o [arg]
    (cond [(and (symbol? arg) (.startswith arg "o!"))
           arg]
          [(and (instance? list arg) (.startswith (first arg) "o!"))
           (first arg)]))
  (setv os (list (filter identity (map extract-o args)))
        gs (list-comp (HySymbol (+ "g!" (cut s 2))) [s os]))
  `(defmacro/g! ~name ~args
     `(do (setv ~@(interleave ~gs ~os))
          ~@~body)))

@Kodiologist
Copy link
Member

Try adding a test.

@oskarkv
Copy link
Contributor Author

oskarkv commented Jun 12, 2018

I might have discovered a problem with the tests.

I was trying to add a test in test-defmacro! in tests/native_tests/native_macros.hy. I was confused because some tests that should be failing was passing. But if I added a bad macro it would complain when trying to expand it, so something was working. At last, I just added (assert (= 1 0)) in test-defmacro! without anything else new, and the tests still passed. At this point I was really confused. I thought that maybe the function didn't run, and the bad macros were failing at compile time. Then I counted the number of test functions (19), but pytest said "17 passed". Two function names had ! at the end. I renamed test-defmacro! to test-defmacrobang, and then pytest started saying 1 failed, 17 passed.

So, it is just for me that the functions ending with ! don't run? I don't think so, because one of the old asserts fail, namely (assert (in "_;res|" s1)).

@Kodiologist
Copy link
Member

Kodiologist commented Jun 12, 2018

It's not just you. That's a regression I introduced that #1625 fixes. Sorry about that.

@oskarkv
Copy link
Contributor Author

oskarkv commented Jun 12, 2018

Ah, OK! I added some tests.

@oskarkv
Copy link
Contributor Author

oskarkv commented Jun 12, 2018

@gilch Oh, missed your comment there.

When you say I can work around it, do you mean privately, or do you suggest I submit an updated version of defmacro! that supports unpacking? I figured I could wait until you guys finish the more general destructuring macros. It seems a little strange that defmacro! should support unpacking but not defn or defmacro. But I guess it makes more sense in defmacro!, since not having unpacking requires more extra lines than in defn, for example I think this is the shortest way to write if-let without it, which is two extra lines compared to having unpacking:

(defmacro! if-let [bindings then else]
  (setv [sym expr] bindings)
  `(do (setv ~g!t ~expr)
       (if ~g!t (let [~sym ~g!t] ~then) ~else)))

@Kodiologist Kodiologist merged commit 86deff6 into hylang:master Jun 25, 2018
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.

3 participants