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

Assoc returning the dict #238

Closed
laarmen opened this issue Jul 9, 2013 · 22 comments
Closed

Assoc returning the dict #238

laarmen opened this issue Jul 9, 2013 · 22 comments

Comments

@laarmen
Copy link

laarmen commented Jul 9, 2013

Would it be possible to have assoc return the modified dict instead of nil?

It would make dealing with dictionaries in Hy a bit easier, IMHO

@algernon
Copy link
Member

algernon commented Jul 9, 2013

+1

@algernon
Copy link
Member

algernon commented Jul 9, 2013

This is a bit tricky implementation-wise, though, as (assoc foo bar baz) translates to foo[bar]=baz right now. If we want it to return foo, then we'll have to wrap it in a function, which is a bit costy. Or we need to make the compiler smart enough to wrap it only when neccessary... Either way, not easy.

@laarmen
Copy link
Author

laarmen commented Jul 9, 2013

Yes, if it was easy I probably would have attached a patch :-)

@Foxboron
Copy link
Member

Wouldn't it be possible to override it in the core lib? name the compilers assoc too assoc*(or something) and reimplement the actual assoc function in the std lib and make it return the modified map
?

@paultag
Copy link
Member

paultag commented Jul 14, 2013

@Foxboron: since we can express it in AST, we should avoid making a function for it :)

@rcarmo
Copy link
Contributor

rcarmo commented Dec 6, 2014

...but can we fix it? I keep getting bit by this when context-switching from another LISP :)

@emidln
Copy link

emidln commented May 29, 2015

I'd prefer to see a set! that uses __setitem__ under the hood and make assoc return the modified dict.

@chr15m
Copy link

chr15m commented Jun 10, 2015

+1

@chr15m
Copy link

chr15m commented Jun 10, 2015

Maybe I am missing something but (assoc foo bar baz) could instead translate to:

foo = dict(foo, **{bar: baz})

http://stackoverflow.com/a/1453013/2131094

Hm, I should PR shouldn't I.

@refi64
Copy link
Contributor

refi64 commented Jun 10, 2015

@chr15m Wouldn't that be slower, though? You're copying the entire dictionary to update one key.

Hy has the ability to implement this the correct way, similarly to setv and the try expression.

@chr15m
Copy link

chr15m commented Jun 10, 2015

Just tested - yes it is nearly 1000 times slower on my machine.

@cwebber
Copy link
Member

cwebber commented Jun 11, 2015

So, assoc is useful in a lisp using linked lists because it usually works with alists, which, while not the most efficient structures in the world, are (assuming nobody does any nasty stuff and mutate the cons cells), mostly immutable mapping structures. Of course, sometimes people want more efficient variants... in Guile there are vlists, and there's a whole set of purely functional data structures and fectors available. Which is great. Often these reuse the name "assoc" in some way, but are purely functional!

So my suggestion is, instead of abusing assoc to try to fake immutability through having nice new structures returned on totally mutable structures by copying the dicts (yikes!), Hy should add nice sugar around existing immutable datastructures for Python... maybe pyrsistent?

@rcarmo
Copy link
Contributor

rcarmo commented Jun 11, 2015 via email

@chr15m
Copy link

chr15m commented Jul 10, 2015

If I introduced an assoc! which changes the dict and also returns it would that tiny PR be accepted?

@zackmdavis
Copy link
Contributor

@chr15m I'd be in favor of such a function, but I can imagine the name assoc! being confusing (because the presence of both assoc and assoc! in the language might lead many to expect that assoc returns a new dict and doesn't modify the existing one, almost the opposite of what we actually want to convey). I'm not sure what alternative name to suggest at the moment.

@chr15m
Copy link

chr15m commented Jul 10, 2015

Good point.

The immediate situation is that the current assoc doesn't do what anybody expects. In my opinion the name of the current assoc should be something else (maybe fast-assoc or set-key or something better), and a new assoc that is slow but does what people expect should exist and have documentation that says why it is slower than the faster alternative. This gives the user a version of assoc that works as everyone expects it to, and also an alternative when they need a performant way to set dict key-vals inside e.g. a tight loop or whatever.

tl;dr: rename existing assoc and create a slower assoc that does what everybody expects.

@gilch
Copy link
Member

gilch commented Jul 10, 2015

How about compiling assoc to something more like this?

# current assoc
spam[i] = y

# proposed assoc
(spam.__setitem__(i, y) or spam)

# or even like this?
(spam.__setitem__(i, y), spam)[1]

Which of the two we use depends on if we want to allow custom __setitem__ implementations on user classes to return something.

If we want the old behavior, for performance reasons, we can rename the current assoc to something more intuitive, like setitem or set-item.

@zackmdavis
Copy link
Contributor

@gilch, I like that idea!

If assoc were to be renamed, I suggest assoc* (with an asterisk) as the new name, in analogy to how Hy and Clojure define the more-commonly-used for in terms of the more-fundamental for*. (Edit: ah, I see @Foxboron already suggested this upthread the other year.)

@gilch
Copy link
Member

gilch commented Jul 10, 2015

A possible issue with my approach:

>>> spam = [1,2,3]
[1, 2, 3]
>>> def getspam():
...   print("side effect!")
...   return spam
...
>>> getspam().__setitem__(0,"zero") or getspam()
side effect!
side effect!
['zero', 2, 3]

I'm still not familiar enough with Hy's AST manipulation approach to know if this is easily avoidable.
The obvious way around this problem in handwritten pure Python has the overhead of a function call:

# (assoc (getspam) 0 "zero"))  ; compiles to:
(lambda x: x.__setitem__(0, "zero") or x)(getspam())

But this doesn't appear to work automatically, since then the x is not gensym'd and could shadow something important:

# (def x "something important")
# (assoc (getspam) 1 x)  ; compiles to:
x = "something important"
(lambda x: x.__setitem__(0, x) or x)(getspam())  # probably not what you meant.

I would also like to point out that Python already has an operator.setitem function. We don't need a new one, do we? Maybe we should import it by default though.

@gilch
Copy link
Member

gilch commented Jul 10, 2015

On second thought this should work:

x = "important"
(lambda x,i,y:x.__setitem__(i, y) or x)(getspam(), 0, x)

I noticed assoc is not considered a function now:

=> assoc
assoc
Traceback (most recent call last):
  File "<input>", line 1, in <module>
NameError: name 'assoc' is not defined

So there's a possible performance reason to duplicate the operator.setitem behavior as a macro. I don't know if Python can do any kind of in-lining optimizations.

@refi64
Copy link
Contributor

refi64 commented Aug 1, 2015

I think I have a really simple solution. Basically, this:

(print (assoc d 1 2))

turns into:

d[1] = 2
print(d)

and this:

(print (assoc (f) 1 2))

turns into:

_hy_anon_var_x = f()
_hy_anon_var_x[1] = 2
print(_hy_anon_var_x)

@refi64
Copy link
Contributor

refi64 commented Apr 16, 2017

After the discussion in #1246 and #1257, I'm going to close this. I don't see it happening now that setv also returns None.

@refi64 refi64 closed this as completed Apr 16, 2017
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.