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

Python3 changes for rpython #4807

Open
wants to merge 10 commits into
base: branch/default
Choose a base branch
from

Conversation

gitlab-importer
Copy link

In GitLab by @mattip on Oct 13, 2021, 20:40

I had been working on this branch to update some of the rpython syntax to be python3-compatible. This is the first set of changes.

@gitlab-importer
Copy link
Author

In GitLab by @rlamy on Oct 13, 2021, 22:56

Interesting! I can't properly review it now, but I'll first say that I'm not a fan of the invasive changes required to handle the pairtype + __extend__ hack.
Also, what was your objective with this branch? Running some well-chosen command without error on python3?

@gitlab-importer
Copy link
Author

In GitLab by @mattip on Oct 13, 2021, 23:29

The goal was to try to get "cd pypy/doc; make html" work when python==cpython3.8, since the day when read-the-docs does not support python2 are probably not far off.

@gitlab-importer
Copy link
Author

In GitLab by @mattip on Oct 14, 2021, 09:45

What is wrong with the changes? That they are so extensive and may contain an error or that there is an advantage to the previous syntax?

@gitlab-importer
Copy link
Author

In GitLab by @rlamy on Oct 14, 2021, 19:39

Right! With this branch, I think we're still pretty far off from this goal. OTOH, I have a much simpler patch that almost gets us there:

diff --git a/pypy/doc/pypyconfig.py b/pypy/doc/pypyconfig.py
--- a/pypy/doc/pypyconfig.py
+++ b/pypy/doc/pypyconfig.py
@@ -1,6 +1,7 @@
 
 
 def setup(app):
+    return
     import sys, os
     sys.path.insert(0, os.path.abspath("../../"))

Note that the pages we're losing that way depend both on the branch and the platform on which the docs are built, which is less than ideal.

@gitlab-importer
Copy link
Author

In GitLab by @olliemath on Oct 18, 2021, 01:00

I was recently looking into rpython (specifically, helping with the 2->3 thing) - anyhow, there's a possibility you could make all the pairtype diffs look like

-    def cmp((obj1, obj2)):
+    def cmp(obj1, obj2):

if you made a fancier metaclass to wrap the the methods:

def pairtype_method_wrapper(method):
    """
    Transform a method like add(a, b) to work on self
    (where self is a 2-tuple).
    """

    def wrapped(self, *args, **kwargs):
        return method(self[0], self[1], *args, **kwargs)

    return wrapped


class extendabletype(type):
    """A type with a syntax trick: 'class __extend__(t)' actually extends
    the definition of 't' instead of creating a new subclass."""
    def __new__(cls, name, bases, dict):
        if name == '__extend__':
            for cls in bases:
                for key, value in dict.items():
                    if key == '__module__':
                        continue
                    # XXX do we need to provide something more for pickling?
                    if callable(value):
                        value = pairtype_method_wrapper(value)

                    setattr(cls, key, value)
            return None
        else:
            return super(extendabletype, cls).__new__(cls, name, bases, dict)

@gitlab-importer
Copy link
Author

In GitLab by @arigo on Oct 18, 2021, 10:01

Hi Olivier,

I was recently looking into rpython - anyhow, there's a possibility you could make all your diffs look like

  • def cmp((obj1, obj2)):
  • def cmp(obj1, obj2):

I think this makes explicit calls to these methods very messy. For
example, rpython/rtyper.py contains the line:

return pair(r_arg1, r_arg2).rtype_contains(hop)

It's unclear how that line should be replaced. Something like

    return pairtype(type(r_arg1), type(r_arg2)).rtype_contains(r_arg1, r_arg2, hop)

may work but I think only on Python 3. On Python 2 it would complain
that the unbound method is called with a wrong 'self' argument type.
It would need even more hacking in the metaclass to suppress that
error. Or maybe...

return pair.rtype_contains(r_arg1, r_arg2, hop)

with "pair" being no longer a function but a singleton with a custom
__getattr__, something like this:

class _Pair(object):
    def __getattr__(self, name):
        def call_with_double_displatch(a, b, *args, **kwds):
            method = getattr(pairtype(a.__class__, b.__class__), name)
            method = method.im_func    # on python 2 only, I think
            return method(a, b, *args, **kwds)
       setattr(self, name, call_with_double_displatch)    # why not, probably speeds up accesses
       return call_with_double_displatch
pair = _Pair()

@gitlab-importer
Copy link
Author

In GitLab by @mattip on Oct 18, 2021, 10:04

@arigo your response was cut short

@gitlab-importer
Copy link
Author

In GitLab by @arigo on Oct 18, 2021, 10:06

Yes, just noticed and fixed. I hate this about this issue tracker, it fools me into thinking it's a normal e-mail from the mailing list, and then when I reply to it as if it were an e-mail, the reply is cut at the line "On Mon, 18 Oct 2021 at 00:00, xxx wrote".

@gitlab-importer
Copy link
Author

In GitLab by @olliemath on Oct 18, 2021, 11:36

I'm unsure what you mean here - with the metaclass wrapping everything in pairtype_method_wrapper you still get to call return pair(r_arg1, r_arg2).rtype_contains(hop). Unless there's more advanced usage going on that I haven't seen.

For example, with the metaclass I posted above, this works fine under py2.7 and py3:

class __extend__(pairtype(int, int)):

    def add(a, b):
        return a + b


print(pair(1, 2).add())

Is there reason this wouldn't work for rtype_contains?

(For reference, I'm running under py3 using a small snippet: pairtype.py) so might not have the full picture)

@gitlab-importer
Copy link
Author

In GitLab by @olliemath on Oct 18, 2021, 12:22

If the metaclass way is too magical, the equivalent is to explicitly decorate all of the methods:

@pairtype_method_wrapper
def add(a, b):
    return a + b

rather than autodecorating in the metaclass _ _ new _ _. Which is basically the changes @mattip made, but in decorator form.

@gitlab-importer
Copy link
Author

In GitLab by @arigo on Oct 18, 2021, 13:25

Sorry, my bad. You're right, I misread the metaclass that you wrote. As long as it continues to work on Python 2.7 I'm fine.

@TheShermanTanker
Copy link

Sorry to interject, but does this mean that rpython will be written in Python 3 once merged, so translation can be done with a Python 3 Interpreter instead of Python 2?

@olliemath
Copy link
Contributor

olliemath commented Jan 16, 2024

Sorry to interject, but does this mean that rpython will be written in Python 3 once merged, so translation can be done with a Python 3 Interpreter instead of Python 2?

Unfortunately no - there is a long way to go to support something like that: https://github.com/pypy/pypy/wiki/RPython-3-considerations

Any such changes to rpython would need to support translating under both py2 and py3 because pypy is written in python2

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.

None yet

4 participants