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

-> no longer works with dot expression #83

Open
asimjalis opened this issue Feb 23, 2024 · 12 comments
Open

-> no longer works with dot expression #83

asimjalis opened this issue Feb 23, 2024 · 12 comments

Comments

@asimjalis
Copy link

REPL session

=> (setv x 10)
x = 10
None
------------------------------
=> x.real
x.real
------------------------------
10
=> (. x real)
x.real
------------------------------
10
=> (-> x (. real))
real(x)
------------------------------
Traceback (most recent call last):
  File "stdin-32a4586a6638a8de7eec78e48c4e19dfcd34b12b", line 1, in <module>
    (-> x (. real))
              ^^^
NameError: name 'real' is not defined

Expected: (-> x (. real)) should produce 10 just like (. x real). It should not produce the error NameError: name 'real' is not defined.

hyrule version: 0.5.0
Hy version: 0.28

@asimjalis
Copy link
Author

Note this used to work in older versions of Hy. It is possible that the recent changes to -> had the effect of breaking this.

@Kodiologist
Copy link
Member

This looks like the correct behavior to me. (. real) compiles to just real, so (-> x (. real)) should be equivalent to (-> x real), which produces the same error.

@Kodiologist
Copy link
Member

@scauligi You were working on argmove macros relatively recently; what do you think?

@scauligi
Copy link
Member

scauligi commented Feb 24, 2024

Hmm, I think they have a point here:

(defmacro dot [#* syms] `(. ~@syms))
(-> x (dot real))

compiles to x.real, even though (-> x (. real)) compiles to real(x).

The problem is that the reader parses forms like str.upper directly into Expression([Symbol('.'), Symbol('str'), Symbol('upper')), so macros can't tell the difference between str.upper and (. str upper).

However, I think it's reasonable to expect that while (-> x foo.bar) compiles to (foo.bar x), the form (-> x (. foo bar)) should compile to (. x foo bar).

This would require a change to Hy's AST, to allow for a distinction somehow between 'foo.bar and '(. foo bar)


In previous versions of Hy, foo.bar parsed to Symbol('foo.bar') instead of Expression(...); however, this lead to problems where macros weren't treating (. foo bar) like foo.bar (iirc) which is why we changed it.

I'm not quite sure what the right solution here is... Potentially we could add a flag or other metadata to the Expression form when we parse foo.bar so that macros (like ->) that care can make a distinction?

@Kodiologist
Copy link
Member

Ah geez, I don't want to change Hy's built-in syntax to make -> slightly more convenient. If you're sure you want (-> x (. real)) to compile to x.real, then bite the bullet and make (-> x foo.bar) compile to (. x foo bar), too.

@Kodiologist
Copy link
Member

I guess there's always the ultimate flexibility obtainable by making -> a reader macro instead.

@asimjalis
Copy link
Author

asimjalis commented Feb 24, 2024

Here is an example of where the old simpler interpretation of -> is useful.

; Find parent directory path of code where current stack frame is running.
(-> (inspect.currentframe) (inspect.getframeinfo) (. filename) (Path) (.resolve) (. parent) (str) )

The operator -> has very simple semantics and is easy to reason about. It is fun to grow code organically as above exploring the methods and attributes of each successive result.

Here is the above line in a self-contained executable snippet. I’m overriding the latest -> with an older version so the snippet runs without errors.

(defmacro -> [head #* args]
  (setv ret head)
  (for [node args]
    (setv ret (if (isinstance node hy.models.Expression)
                  `(~(get node 0) ~ret ~@(cut node 1 None))
                  `(~node ~ret))))
  ret)

(import inspect)
(import pathlib [Path])

(-> (inspect.currentframe) (inspect.getframeinfo) (. filename) (print)) 
(-> (inspect.currentframe) (inspect.getframeinfo) (. filename) (Path) (.resolve) (. parent) (str) (print))

@msicilia
Copy link

Hi,
I had found the same problem referenced in this issue using the pandas library for which the threading macro is really convenient.

(import pandas :as pd)
(setv df (pd.DataFrame {"a" [1, 1, 2, 2, 3] "b" [4, 5, 6, 7, 8]}))
(print (get (. df shape) 0))
(print (-> 
           (. df shape)
           (get 0)
           ))
(print (-> df
           (. shape)
           (get 0)
           ))

The first two print calls work as expected printing 5 as the number of rows. But the last one fails:

Traceback (most recent call last):
  File "...", line 8, in <module>
    sys.exit(hy_main())
             ^^^^^^^^^
  File "<frozen runpy>", line 291, in run_path
  File "<frozen runpy>", line 98, in _run_module_code
  File "<frozen runpy>", line 88, in _run_code
  File "...", line 81, in <module>
    (. shape)
        ^^^^
NameError: name 'shape' is not defined

I struggle to understand why the second works but not the third, not sure if this is a different situation from the previous ones in this thread?
Hy version: 1.0.0
Hyrule version: 0.7.0

@Kodiologist
Copy link
Member

Yeah, looks the identical problem to me. (-> df (. shape) (get 0)) is better written (. df shape [0]) or (get df.shape 0), anyway.

@yiranlus
Copy link

yiranlus commented Oct 24, 2024

The problem seems to be the _dotted in argmove.hy. When .method is passed to _dotted, the list is converted to (.method); but when something like (. name) is passed, it returns ((. name)) which results in error.

I expanded the following expression as list, and found:

  • .method, which expanded to '(. None method)`
  • (.method), there is only one element .method in the list
  • (. name), there are two element '(. name)` as shown

Since _dotted is intended to convert only .method, it is good to suppose it only converts the first case.

The original condition for _dotted is

    (if (and (isinstance node hy.models.Expression)
             (= (get node 0) '.))

I added (is (get node 1) None) to make it only consider the first case.

#104

@yiranlus
Copy link

Sorry, I missed the case where the dotted expression is like 'str.upper, but for now have no idea how to distinguish it from '(. str upper). They seem to produce the same expression in Hy.

@Kodiologist
Copy link
Member

That's correct. a.b is syntactic sugar for (. a b). See Syntax § Dotted identifiers.

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.

5 participants