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

Always have a reasonable reader accessible #2494

Merged
merged 3 commits into from
Aug 16, 2023
Merged

Conversation

scauligi
Copy link
Member

@scauligi scauligi commented Aug 13, 2023

hy.read now attaches its reader to the form it returns, so patterns like

(hy.eval (hy.read "(defreader foo 3)"))
(hy.eval (hy.read "#foo"))

work as expected.

If a reader can't be found, then we'll create one temporarily so that everything goes through.

@Kodiologist
Copy link
Member

That was fast. Thanks so much.

I'm a bit puzzled by test-eval-read. Are you missing some assertions at the end of the first with form? You're putting a few reader macros into module and then not trying them out. It might also make more sense to put (defn eval1 …) inside the second with, since its value of module comes from the context manager there.

It looks like GitHub isn't associating your commits with your account, maybe because of an email-address change.

@scauligi
Copy link
Member Author

scauligi commented Aug 14, 2023

test-eval-read

Those tests are to ensure that hy.eval doesn't throw an exception, even though no explicit reader has been set.
Since it'll end up using a temporary reader, it's not actually possible to use the reader macros in a followup call directly,
although I can use the setup from #2292 (comment) to check that the macros can be copied out from the module. I guess I might as well do that?

moving eval1

👍

commit authoring

...weird, I'll fix up the author identity when I re-push.

@Kodiologist
Copy link
Member

Those tests are to ensure that hy.eval doesn't throw an exception

I see. You could add a comment saying that the point is just to check that these calls don't raise exceptions, or you could copy out the reader macros as you mentioned.

@scauligi
Copy link
Member Author

wut.

Need to catch a train soon but I'll dig in to this when I get the chance, sigh

@Kodiologist
Copy link
Member

I don't understand the test error, but it reminds me of a transient error I've seen on PyPy on GitHub Actions where some other test of a reader macro fails, then passes when I rerun the job. I reran this job and it still failed. Could there be some weird race condition involving PyPy's JIT compiler?

@scauligi
Copy link
Member Author

Nope it was a legit bug, HyReader.parse uses a context to set hy.&reader to self when it starts parsing, but it's a generator.
hy.read takes one element from that generator with next and then tosses the rest away. In CPython, the generator was getting garbage collected pretty much right away, so the parsing context exited and hy.&reader got properly unset.
In PyPy, the generator wasn't getting immediately garbage collected, so the partially-used reader stuck around as the global context and got used for the following calls.

What a wild ride.

Anyways, I pushed the context-setting into try_parse_one_form so the notion of "current reader" isn't conflated with reading a stream anymore, and I added a test to make sure that reading/eval'ing from two streams interleaved works properly.

@Kodiologist
Copy link
Member

That sounds like a nasty one. Good job.

valss is a funny variable name, but I get it.

Don't forget to remove your debugging prints in test-eval-read.

@scauligi
Copy link
Member Author

scauligi commented Aug 15, 2023

I pushed the fix for #2292 as an additional commit here, since it's pretty small and it's a lot easier to stick its test into the new tests here.

@Kodiologist
Copy link
Member

Noice. I would write "is true" rather than "is True", since you're doing a regular Boolean comparison, not an is True check. Otherwise, looks good.

@Kodiologist
Copy link
Member

Sorry, I forgot, the new option to HyReader could also use a NEWS line.

@Kodiologist Kodiologist merged commit 11467ea into hylang:master Aug 16, 2023
@scauligi scauligi deleted the reread branch August 22, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants