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

Make assoc into a function instead of a special form #1346

Closed
wants to merge 2 commits into from

Conversation

Kodiologist
Copy link
Member

@Kodiologist Kodiologist commented Jul 28, 2017

Fixes #1068.

I was putting the finishing touches on a PR making assoc into a macro and only then realized it could be a plain old function. Derp!

@gilch
Copy link
Member

gilch commented Jul 28, 2017

assoc should also work with slices. That should be tested.

I noticed some awkward zip/cuts in the implementation. partition is defined as a global in that module, so it's available. Or was this performance motivated? Perhaps we should modify core's list manipulation functions to use direct indexing instead of iterators where possible. (It's a separate issue.)

But why was assoc a special form in the first place? It's not the only one that could be implemented as a function instead. Therefore, I think assoc should be a macro for performance, and avoid the overhead of an extra function call. There's no need for assoc* as I originally supposed. It can expand to repeated gets in a setv instead. The function version you just wrote can then be moved to shadow so it's still available when a macro wouldn't work.

@refi64
Copy link
Contributor

refi64 commented Jul 28, 2017

Therefore, I think assoc should be a macro for performance

+1 to this. Using a plain function will likely be a performance killer.

@Kodiologist
Copy link
Member Author

assoc should also work with slices.

What exactly do you mean? Can you give an example?

Good catch regarding partition; I forgot it existed.

But why was assoc a special form in the first place?

I don't know. Possibly by mistake, possibly because (setv (get x k) …) hadn't yet been implemented. Arguably it doesn't provide much of an advantage over setv and get in the first place and is hence superfluous.

It's not the only one that could be implemented as a function instead.

I would generally advocate changing special forms to macros or functions when this is possible, so let me know if you find any others.

Do we really need the micro-optimization of assoc being a macro? Most, maybe all, of Hy's core functions could conceivably be implemented as macros; just as macros are better than special forms, I think functions are better than macros.

@gilch
Copy link
Member

gilch commented Jul 28, 2017

give an example?

Look what Python can do.

>>> spam = [0]*5
>>> spam
[0, 0, 0, 0, 0]
>>> spam[::2] = [1]*3
>>> spam
[1, 0, 1, 0, 1]
>>> spam[1::2] = [-1, -2]
>>> spam
[1, -1, 1, -2, 1]

Actually, I think this will work fine even in the function version with explicit (slice ...) objects. (I think Python uses those internally anyway.) So (assoc spam (slice 1 None 2) [-1 -2]) should work the same as (setv (cut spam 1 None 2) [-1 -2]). But a test wouldn't hurt.

Arguably it doesn't provide much of an advantage

The advantage is more obvious when setting multiple things at once.

(setv (get foo :a) 1
      (get foo :b) 2
      (get foo :c) 3)

See that repetition of foo and get?

(assoc foo
  :a 1
  :b 2
  :c 3)

Much more concise, but it compiles exactly the same way.

Do we really need the micro-optimization of assoc being a macro? Most, maybe all, of Hy's core functions could conceivably be implemented as macros; just as macros are better than special forms, I think functions are better than macros.

Pretty much all of our operators are both special forms and functions in shadow. Why not eliminate all the operator special forms and just use the functions? Performance. Shadowed forms give us the best of both worlds, and it's the same with assoc. It is considered an operator in Python, just like + is. It's a variation of operator.setitem, written with [] and =.

I would generally advocate changing special forms to macros or functions when this is possible, so let me know if you find any others.

I would be in favor of redefining compiler special forms as macros in terms of more basic special forms, as is the case with assoc in terms of setv and get. Macros are a lot easier to work with, and it's good to keep the compiler simpler, all else equal. Maybe I should just check all of those. But when there's a direct correspondence to a basic Python statement or operator, I'd like to keep the form in the compiler.

@Kodiologist
Copy link
Member Author

So (assoc spam (slice 1 None 2) [-1 -2]) should work the same as (setv (cut spam 1 None 2) [-1 -2]). But a test wouldn't hurt.

Sounds good to me.

Why not eliminate all the operator special forms and just use the functions? Performance.

While I agree with the performance consideration in the case of math operators, I think that being able to produce real ast.BinOp nodes is the more important reason we have special forms for them. I'd like it to be possible to produce the AST corresponding to any Python program with pure Hy (although, admittedly, we're not there yet).

It is considered an operator in Python, just like + is. It's a variation of operator.setitem, written with [] and =.

I see what you mean, but then how do we decide which core functions are a variation on an operator and which aren't? Is last a variation on getitem? Is inc a variation on +? Is zero a variation on =?

@gilch
Copy link
Member

gilch commented Jul 29, 2017

I'd like it to be possible to produce the AST corresponding to any Python program with pure Hy

I also want this to be possible, at least for the most current version of Python.

I see what you mean, but then how do we decide which core functions are a variation on an operator and which aren't? Is last a variation on getitem? Is inc a variation on +? Is zero a variation on =?

Everything with a direct analogue in operator probably qualifies, even if Hy's version takes more arguments. And assoc is a special form presently.

I don't think those three examples qualify by that standard, but making, say, zero? a shadowed macro is maybe not such a bad idea. It's one operation. It's the kind of thing a good optimizing compiler would inline. I wonder if PyPy would actually do this. I'd only consider that for trivially short functions like that though, or it would never end.

@Kodiologist
Copy link
Member Author

Yeah, if we want to go down that route, it seems that we should be generating the code rather than maintaining two versions (shadow and macro) of every function. One can imagine a macro or special form inline that takes a function definition and creates a corresponding macro with the same name.

@gilch gilch mentioned this pull request Nov 8, 2022
@gilch
Copy link
Member

gilch commented Jul 29, 2017

A basic defshadowed macro wasn't hard. There are certain basic limitations, some of them fundamental to this approach.

@gilch
Copy link
Member

gilch commented Aug 3, 2017

So, an assoc macro in terms of setv and get could be implemented like this.

(defmacro! assoc [o!coll &rest kvs]
  "doc here"
  (assert (even? (len kvs)))  ; or equivalant macro-error
  `(setv
     ~@(chain.from-iterable
         (genexpr [`(get ~g!coll ~k) v]
                  [[k v]
                   (partition kvs)]))))

@Kodiologist
Copy link
Member Author

Can we leave the macro version and shadowing and so on for a later PR for defshadowed or whatever, and let assoc be just a function for now?

@Kodiologist
Copy link
Member Author

Or let it be just a macro for now. One or the other.

@gilch
Copy link
Member

gilch commented Aug 10, 2017

Making it a macro for now is the more conservative change. I'd approve that alone, but I'd approve both that and shadowing it with a function too.

@Kodiologist
Copy link
Member Author

Okay, I'll check on my first, macro version and PR that instead.

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