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

Move hy-repr to core and rename eval to hy-eval #1550

Closed
wants to merge 6 commits into from

Conversation

Kodiologist
Copy link
Member

hy-repr is about a year old and it seems to work pretty well. With this PR, hy-repr is used to print REPL output by default, and Hy gets a pair of functions hy-repr and hy-eval analogous to Python's repr and eval, the latter of which is no longer shadowed by Hy.

The change of default repl-output-fun will require a lot of documentation changes, of course, which I haven't yet done, but I will before this PR is merged.

@gilch
Copy link
Member

gilch commented Mar 26, 2018

Veto. I prefer the Python-compatible reprs as default, so you can see what's actually there instead of having to guess. I think changing the default repl output function to hy-reprs would only cause confusion.

I don't mind changing eval back to Python's, so we don't have to use builtins.eval. But I'd prefer we keep the old name. So we'd invoke it as hy.eval instead of hy-eval, or hy.hy_eval on the Python side.

@vodik
Copy link
Contributor

vodik commented Mar 28, 2018

Shame, because I was going to suggest doing it too.

I don't mind changing eval back to Python's, so we don't have to use builtins.eval. But I'd prefer we keep the old name.

I need to jig it around a bit more, and maybe as my time frees up this week I'll pull it out and do it a separate PR, but the importer rework has this change included. But its not just hy.eval - it tries to expose Hy equivalents to Python's parse, compile and exec as well.

@Kodiologist
Copy link
Member Author

You can still review and approve this PR if you like it, and the PR can get merged with two approvals. What the veto means is just that the PR can't get merged under the two-week rule. For what it's worth, I fully expected Matthew to veto this because he disapproved the same thing a year ago, when I first wrote hy-repr (#1246).

@tuturto
Copy link
Contributor

tuturto commented Jun 9, 2018

I like idea of renaming eval to hy-eval. I most of the time use hy-repr when using REPL, but I recognize that not everyone is doing that. Maybe we'll just vote on this and let the majority decide if hy-repr is enabled by default?

@Kodiologist
Copy link
Member Author

Kodiologist commented Jun 9, 2018

That sounds reasonable. Tuukka and I are for and Matthew is against. @vodik, are you for? Does anybody else (developers or users) have an opinion one way or the other?

@gilch
Copy link
Member

gilch commented Jun 9, 2018

I'll reproduce examples from my previous objection below. This is never going to work right except for a narrow subset of data types. This is a niche use that shouldn't be the default. Going our own way makes us less compatible with the Python ecosystem. We cannot afford to do that. Hy's only chance to thrive is to embrace what Python offers. We can find other ways to fill these needs without breaking compatibility.


Suppose you have some Python objects (foos) from some Python library. They have reprs like
foo(1, duck()), but no hy-repr. Now you make some Hy objects (bars) that take some foos as arguments. They have reprs like bar(foo(1, 2), foo(3, bar(foo(4, 5)))). So far, so good, the bar repr defers to the repr of its foo arguments, which defer to the int and bar reprs and the net result is still valid Python, and it will generate an equivalent bar object.

But wait, now we have to write a __hy-repr__. Now it prints (bar foo(1, 2) foo(3, bar(foo(4, 5)))), and that's one of those weird hybrids that's neither Hy nor Python. It's not clear at all to the future user how to parse this. Note that bar appears both in the form (bar ...) and bar(...). Why? Because foo is written in Python and knows nothing about __hy-repr__, so of course it has to use __repr__ from bar instead.

So what if we use generic placeholders instead? Now you've got
(bar <foo instance at 0xXXX1> <foo instance at 0xXXX2>). The user can totally parse this. But, you've hidden important information. Not good. It's not really valid Hy, but at least you know exactly where the holes are.

Okay, maybe we can auto-wrap the repr with the placeholders. Now you get
(bar <Py foo(1, 2)> <Py foo(3, bar(foo(4, 5)))>). It's still not valid Hy, but now we know both where the holes are and what goes in those holes. But look, the Hy repr version only applies to the first layer! We're still getting the form bar(...). What's the point of a Hy repr if we're printing it all off in Python anyway?

Even though this is a contrived example, it is very representative of the common case. It's very typical in Python for the repr to be a constructor call that defers to the reprs of its arguments. There are more Python libraries written in Python than in Hy, so almost all Python objects will have no hy-repr.


@vodik consider my objections before deciding.

@kirbyfan64 @olasd @algernon, you seem to have objected before to similar proposals.

@hylang/core anyone else who wants to stop this, speak up.

Do we know any other users? @ekaschalk? If we don't have a strong consensus, maybe we should leave this open for a while to hear from them?

@paultag care to weigh in? Your opinion counts for a lot around here. We might also need an arbiter.

@paultag
Copy link
Member

paultag commented Jun 10, 2018

This is a tough one. Let me think about it a bit. I can see both sides of this; on the one hand, having eval eval hy code when you're in Hy seems sensible (after all, eval'ing Python from Hy seems.... weird. eval'ing Hy forms seems more likely), but I also understand that overloading internals causes confusion, and one would expect accessing a Python internal from Hy would give you that internal.

I half-heartedly reach for the bashism / out of sheer instinct, but I'm also sure that's not the right solution either (e.g.:

$ alias ls=echo
$ ls /tmp
/tmp
$ \ls /tmp
pulse-PKdhtXMmr18n

)

Let me work this over a bit

@Kodiologist
Copy link
Member Author

To be clear, the point of most controversy here is using hy-repr, rather than Python's repr, as the default method for printing results in the Hy REPL.

@gilch
Copy link
Member

gilch commented Jun 10, 2018

Right, I'm OK with having eval pointing to Python's version. The proposal here was hy-eval. I made a counter proposal of keeping it in the hy namespace and accessing it as hy.eval by convention. We just have to call the two versions different names so we don't lose access to either. We could keep Hy's eval and rename Python's to py-eval. We could even call Hy's evaluate and Python's eval.

Anyway, the part I'm objecting to most is the Hy repl not printing valid Python reprs by default. If it's valid Python we could at least paste them back in the repl and eval them (using Python's eval) most of the time. With Hy reprs, it's much more fragile, since they can contain Python objects that don't have a Hy repr. Thus, it only works on a very restricted set of data types, or we force the user to write a hy repr for any Python type they might ever use. I don't think this can be automated reliably, not that we've even tried.

@Kodiologist
Copy link
Member Author

@tuturto, it doesn't look like anybody else is weighing in, so the majority vote as it stands is aye.

@gilch
Copy link
Member

gilch commented Jun 25, 2018

We should wait for @paultag. After considering this at length, I've decided that I am not fundamentally opposed to using s-expression output in the REPL. I think it would be nice. But I am vehemently opposed to making this half-baked implementation of it the default. This is nowhere near ready for core.

  1. Adding a __hy_repr__ magic method will reduce the compatibility of our entire ecosystem with Python. If that's the REPL default, it will be easier to just write the __hy_repr__ method and forget the __repr__. Except now we're stuck with the default object repr from the Python side.

  2. This implementation easily creates confusing hybrids reprs that are neither fully Python nor fully Hy. We should output one or the other. Currently, that means we output Python.

I think we could do better. But if we put this in core now, it may never happen.

I would suggest that (if we want Hy reprs at all) we automatically convert Python reprs to Hy syntax.

  1. Wrap the standard <...> placeholders with strings, like '''<...>''', (escaping any internal as required).
  2. Run the repr through (an updated) py2hy, or implement an equivalent.
  3. Find and unwrap the strings. Or don't bother.
  4. If any of the above steps fail, fall back to the fully Python repr, and prepend a Python comment indicating that the autotranslation failed.
  5. Pretty print with proper indentation.

This means that there's no __hy_repr__ magic, only __repr__, which addresses point 1. And the output is only fully Hy when this is possible to do automatically, and fully Python otherwise, which addresses point 2.

Even if that's the default we should, of course, have an option to always output the Python reprs. There may be special formatting or comments and such that would get clobbered by the auto-translation.

And to complete the loop, we ought to have a macro or something that allows us to write valid Python __repr__s using Hy code. We already have hy2py, so this should not be as difficult.

@hylang/core somebody stop this before it's too late.

@Kodiologist
Copy link
Member Author

To be clear, hy-repr uses registered functions, rather than methods, as of #1516.

I would suggest that (if we want Hy reprs at all) we automatically convert Python reprs to Hy syntax.

I don't think this is a good idea. It seems error-prone. But if I did it, would you approve this pull request?

@gilch
Copy link
Member

gilch commented Jun 25, 2018

I would suggest that (if we want Hy reprs at all) we automatically convert Python reprs to Hy syntax.

I don't think this is a good idea. It seems error-prone.

Oh it seems very error-prone, but that still seems a lot better than what you're proposing currently. Especially since we could probably detect most of them automatically and fall back to Python. In cases that don't, they can open an issue and we can try to improve the converter. We could maybe also give the user a hook to the repl output function to manually override the conversion if some case proves especially problematic.

But if I did it, would you approve this pull request?

Yes.

To be clear, I would approve that pull request, not this one currently. (Pending other details, of course.)

@Kodiologist
Copy link
Member Author

@brandonwillard I'm thinking of picking this back up again, but I recall that you wrote in a blog post about a major overhaul of hy-repr you were working on. Do you think you'll submit that soon, and that it will make the changes here irrelevant?

@Kodiologist
Copy link
Member Author

This should be made obsolete by the bigger core changes I'd like to make in the future.

@Kodiologist Kodiologist closed this Apr 7, 2020
@Kodiologist
Copy link
Member Author

The PyPy bug fix in the first commit has since gotten in by other means.

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.

5 participants