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

Docs not fully updated for Keyword.__call__ #2157

Closed
fmqa opened this issue Aug 29, 2021 · 4 comments · Fixed by #2178
Closed

Docs not fully updated for Keyword.__call__ #2157

fmqa opened this issue Aug 29, 2021 · 4 comments · Fixed by #2178

Comments

@fmqa
Copy link

fmqa commented Aug 29, 2021

Trying to access a dict keyed by a keyword (as presented in some more complex examples from the documentation e.g. https://docs.hylang.org/en/alpha/api.html?highlight=person#fn) does not seem to work.

Hy 1.0a3+116.g0abaaaa7 using CPython(default) 3.8.10 on Linux
=> (:x {:x 1})
Traceback (most recent call last):
  File "stdin-5b93f6598dc2625adddf6d6eef831e622528a8f2", line 1, in <module>
    (:x {:x 1})
KeyError: 'x'
=> 

Using the (get obj key) form seems to work however

Hy 1.0a3+116.g0abaaaa7 using CPython(default) 3.8.10 on Linux
=> (get {:x 1} :x)
1
=> 

This seems to be a consequence of

hy/hy/models.py

Line 254 in 949e07b

return data[mangle(self.name)]
where (:kw d) is effectively d["kw"] after mangling+stringification.

Perhaps it would be reasonable to introduce a two-step lookup for keywords i.e. where keyword lookup is attempted first, and "mangled string lookup" is attempted if that does not work? Perhaps something like this would be a reasonable compromise:

    def __call__(self, data, default=_sentinel):
        from hy.lex import mangle
        try:
            return data[self]
        except KeyError:
            try:
                return data[mangle(self.name)]
            except KeyError:
                if default is Keyword._sentinel:
                    raise
                return default

{:x 1 "x" 2} would then become ambiguous but in this case the user can utilize (get {:x 1 "x" 2} :x) or (get {:x 1 "x" 2} "x") to disambiguate.

@Kodiologist
Copy link
Member

This is a failure to update the documentation for #1977. #1019 is the larger issue that leads to these things.

@Kodiologist Kodiologist changed the title callable keyword syntax for dict accesses does not work as expected Docs not fully updated for Keyword.__call__ Aug 29, 2021
@Kodiologist
Copy link
Member

Kodiologist commented Aug 29, 2021

To be clear: in recent versions of Hy, you rarely want to use actual keyword objects as the keys of a dictionary. Use strings instead. Note that constructs like (dict :a 1 :b 2), unlike {:a 1 :b 2}, create a dictionary with string keys.

@fmqa
Copy link
Author

fmqa commented Aug 29, 2021

Thanks, that makes sense. The documentation wasn't very clear on the idiomatic notation for dicts, or at least I missed that part. I'm used to clojure, so I might be biased here :) - I assume any examples in the documentation should be rewritten to use string keys or the (dict :a 1 :b 2 ...)-notation then?

Note that constructs like (dict :a 1 :b 2), unlike {:a 1 :b 2}, create a dictionary with string keys.

However, doesn't (dict :a 1 :b 2) behave a bit differently? AFAIK that calls the dict builtin, which is not the same as the "literal construction" used by Python bytecode-wise:

Python 3.8.10 (default, Jun  2 2021, 10:49:15) 
[GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import dis
>>> 
>>> 
>>> def f(): return {"x": 1}
... 
>>> def g(): return dict(x=1)
... 
>>> dis.dis(f)
  1           0 LOAD_CONST               1 ('x')
              2 LOAD_CONST               2 (1)
              4 BUILD_MAP                1
              6 RETURN_VALUE
>>> dis.dis(g)
  1           0 LOAD_GLOBAL              0 (dict)
              2 LOAD_CONST               1 (1)
              4 LOAD_CONST               2 (('x',))
              6 CALL_FUNCTION_KW         1
              8 RETURN_VALUE
>>> 

Unfortunately, there is a significant performance difference between the two - or at least that is still the case on python 3.8

$ python -m timeit "d = dict(a=1, b=2, c=3, d=4, e=5)"
500000 loops, best of 5: 258 nsec per loop
$ python -m timeit "d = {'a':1, 'b':2, 'c':3, 'd':4, 'e':5}"
2000000 loops, best of 5: 153 nsec per loop

I thought this is was worth mentioning (but in general I don't this is much of a big problem) -- I agree that the real problem seems to be that the documentation is not clear enough on the fact that keywords are not meant to be used as general purpose objects, but rather exist only to facilitate keyword arguments.

@Kodiologist
Copy link
Member

I assume any examples in the documentation should be rewritten to use string keys or the (dict :a 1 :b 2 ...)-notation then?

Right.

AFAIK that calls the dict builtin, which is not the same as the "literal construction" used by Python bytecode-wise

Right, the Hy equivalent of {"a": "b"} is {"a" "b"} rather than {:a "b"}. Regarding speed, you can also probably use the old Python trick of rebinding a global to a local name to make the lookup faster.

the documentation is not clear enough on the fact that keywords are not meant to be used as general purpose objects, but rather exist only to facilitate keyword arguments.

That's a fair point. Although you can use Hy keywords much as you can keywords in other Lisps, it requires a little care because e.g. (foo :bar 3) is interpreted as foo(bar = 3) rather than foo(hy.models.Keyword("bar"), 3) (you must write (foo ':bar 3) instead), and most Python libraries will get confused if you give them keywords when they expect strings. Generally it's best to use strings or enum.Enumss.

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.

2 participants