This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7296

pycompat: kludge around pytype being confused by __new__
Changes PlannedPublic

Authored by durin42 on Nov 6 2019, 6:24 PM.

Details

Reviewers
indygreg
dlax
Group Reviewers
hg-reviewers
Summary

Credit to Yuya for the idea of annotating the class as though it was a
callable. I honestly didn't expect this to work, but it does!

We annotate this awkwardly on a name alias of the type because putting
the annotation directly on the class ...: line as a comment breaks
black and I don't want to go down that rabbit hole.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Nov 6 2019, 6:24 PM
indygreg added a subscriber: indygreg.Nov 7 2019, 3:38 AM

This one makes me sad because of potential performance regressions due to call overhead. I'd almost rather rip out Python 2 support rather than land this. But I could be convinced if pytypes is as cool as you say...

Is there no way to annotate the types here without the call overhead? Could we do something like an if False trick to define a def bytestr() with the proper annotations? Surely there's got to be a way...

durin42 added a comment.Nov 7 2019, 11:07 AM

I've filed a bug with the pytype team, I'll report back.

indygreg requested changes to this revision.Nov 8 2019, 11:48 AM

I'm going to mark for revisions until we know more so this doesn't show up in the reviewable list.

This revision now requires changes to proceed.Nov 8 2019, 11:48 AM
dlax added a subscriber: dlax.Nov 12 2019, 10:45 AM
dlax added a comment.Nov 14 2019, 6:05 AM
class bytestr(bytes):  # type: (Union[bytes,str]) -> bytestr
  [...]

Does this work?

dlax requested changes to this revision.Nov 15 2019, 6:03 AM

Rather class bytestr(bytes): # type: Callable[[Union[bytes, str], bytestr] as @yuya suggested in D7380.

This revision now requires changes to proceed.Nov 15 2019, 6:03 AM
durin42 edited the summary of this revision. (Show Details)Nov 15 2019, 12:31 PM
durin42 updated this revision to Diff 18161.
durin42 updated this revision to Diff 18163.
dlax added a comment.Nov 15 2019, 12:47 PM

black complains because inline comments have only one space before (esp. the first one produces a parse error).
LGTM otherwise.

durin42 updated this revision to Diff 18170.Nov 15 2019, 4:55 PM
dlax requested changes to this revision.Nov 16 2019, 4:28 AM
dlax added inline comments.
mercurial/pycompat.py
294

A ] is missing before , bytestr. Then, it's also missing typing imports.
But, even with this, I get:

Invalid type comment: Callable[[Union[bytes, str]], bytestr] [invalid-type-comment]
  Name 'bytestr' is not defined

I'm able to make this pass with:

bytestr = _bytestr
bytestr = bytestr  # type: Callable[[Union[bytes, str]], bytestr]
This revision now requires changes to proceed.Nov 16 2019, 4:28 AM
indygreg added a comment.Nov 18 2019, 10:47 PM

@durin42 I just queued most of the remaining patches in this series. This one still needs your attention, it appears.

durin42 edited the summary of this revision. (Show Details)Nov 19 2019, 11:14 AM
durin42 updated this revision to Diff 18240.
dlax requested changes to this revision.Nov 19 2019, 11:39 AM

Sorry, still not ok afaict :/

mercurial/pycompat.py
294

] is still at the wrong place, I think. Should be Callable[[Union[bytes, str]], bytestr].
Then I still get the "Name 'bytestr' is not defined" error mentioned above.

401

same here about missing ].

This revision now requires changes to proceed.Nov 19 2019, 11:39 AM
durin42 added a comment.Nov 19 2019, 1:20 PM
In D7296#109672, @dlax wrote:

Sorry, still not ok afaict :/

So, I tried fixing this and it actually made things worse? dagparser.py no longer typechecks if I correct the syntax? Try the pytype invocation from the test at the end of the series and you'll see what I mean.

dlax added a subscriber: yuja.Nov 19 2019, 3:10 PM
In D7296#109683, @durin42 wrote:
In D7296#109672, @dlax wrote:

Sorry, still not ok afaict :/

So, I tried fixing this and it actually made things worse? dagparser.py no longer typechecks if I correct the syntax? Try the pytype invocation from the test at the end of the series and you'll see what I mean.

Ok, trying pytype mercurial/dagparser.py I get a couple of those errors:

File ".../mercurial/dagparser.py", line 172, in parsedag: Function bytestr.__init__ was called with the wrong arguments [wrong-arg-types]
  Expected: (self, ints: Iterable[int])
  Actually passed: (self, ints: str)

My point was that we need to keep "mercurial/pycompat.py" passing pytype before considering modules it depends on. Once the missing type: is added and bytestr <-> _bytestr trick applied, it's okay but the error in dagparser.py persists...
Looking closer at the error above, it mentions bytestr.__init__, not __new__ (and there is in fact no type annotation for __new__ in typeshed). So I suspect the "Callable" trick is not enough and we'd need a workaround similar to da925257 by @yuja .

mercurial/pycompat.py
294

Now the type: is missing, so the comment is ignored.

Adding it, pytype mercurial/pycompat.py gives:

mercurial/pycompat.py", line 305, in <module>: Invalid type comment: Callable[[Union[bytes, str]], bytestr] [invalid-type-comment]
  Name 'bytestr' is not defined

hence the kind of trick I suggested in my first comment on this line.

dlax added a comment.Nov 19 2019, 3:14 PM
In D7296#109684, @dlax wrote:

Looking closer at the error above, it mentions bytestr.__init__, not __new__ (and there is in fact no type annotation for __new__ in typeshed).

https://github.com/python/typeshed/issues/2630

marmoute added a subscriber: marmoute.Jan 30 2020, 1:30 PM

The status of this is unclear ? Are we waiting on:

  1. a the right way to do it in our codebase?
  2. a fix in pytype?
  3. a fix in python?
  4. something else?
marmoute added a comment.Mar 19 2020, 7:14 PM

This is the last series stuck at the bottom of yadda.

It feels like it is still a work in progress, @durin42 can you clarify the status of this and possibly move it out of need-review if it is still a work in progress ?

durin42 planned changes to this revision.Mar 20 2020, 11:59 AM

I'd like to get back to pytyping all the things, but I can't justify spending more time on it. If someone else wants to push things forward I'll gladly provide some mentorship.

Revision Contents
Changeset List

PathPackages
Mmercurial/pycompat.py (11 lines)
Mmercurial/urllibcompat.py (2 lines)

Diff 17672

mercurial/pycompat.py

# On Windows, the native argv is unicode and is converted to MBCS bytes # On Windows, the native argv is unicode and is converted to MBCS bytes
# since we do enable the legacy filesystem encoding. # since we do enable the legacy filesystem encoding.
if getattr(sys, 'argv', None) is not None: if getattr(sys, 'argv', None) is not None:
sysargv = list(map(os.fsencode, sys.argv)) sysargv = list(map(os.fsencode, sys.argv))
bytechr = struct.Struct(r'>B').pack bytechr = struct.Struct(r'>B').pack
byterepr = b'%r'.__mod__ byterepr = b'%r'.__mod__
class bytestr(bytes): class _bytestr(bytes):
"""A bytes which mostly acts as a Python 2 str """A bytes which mostly acts as a Python 2 str
>>> bytestr(), bytestr(bytearray(b'foo')), bytestr(u'ascii'), bytestr(1) >>> bytestr(), bytestr(bytearray(b'foo')), bytestr(u'ascii'), bytestr(1)
('', 'foo', 'ascii', '1') ('', 'foo', 'ascii', '1')
>>> s = bytestr(b'foo') >>> s = bytestr(b'foo')
>>> assert s is bytestr(s) >>> assert s is bytestr(s)
__bytes__() should be called if provided: __bytes__() should be called if provided:
Be careful to not pass a bytestr object to a function which expects Be careful to not pass a bytestr object to a function which expects
bytearray-like behavior. bytearray-like behavior.
>>> t = bytes(t) # cast to bytes >>> t = bytes(t) # cast to bytes
>>> assert type(t) is bytes >>> assert type(t) is bytes
""" """
def __new__(cls, s=b''): def __new__(cls, s=b''):
if isinstance(s, bytestr): if isinstance(s, _bytestr):
return s return s
if not isinstance( if not isinstance(
s, (bytes, bytearray) s, (bytes, bytearray)
) and not hasattr( # hasattr-py3-only ) and not hasattr( # hasattr-py3-only
s, u'__bytes__' s, u'__bytes__'
): ):
s = str(s).encode('ascii') s = str(s).encode('ascii')
return bytes.__new__(cls, s) return bytes.__new__(cls, s)
def _wrapattrfunc(f): def _wrapattrfunc(f):
@functools.wraps(f) @functools.wraps(f)
def w(object, name, *args): def w(object, name, *args):
return f(object, sysstr(name), *args) return f(object, sysstr(name), *args)
return w return w
# these wrappers are automagically imported by hgloader # these wrappers are automagically imported by hgloader
dlaxUnsubmitted
Not Done

A ] is missing before , bytestr. Then, it's also missing typing imports.
But, even with this, I get:

Invalid type comment: Callable[[Union[bytes, str]], bytestr] [invalid-type-comment]
  Name 'bytestr' is not defined

I'm able to make this pass with:

bytestr = _bytestr
bytestr = bytestr  # type: Callable[[Union[bytes, str]], bytestr]
dlax: A `]` is missing before `, bytestr`. Then, it's also missing `typing` imports. But, even with…
dlaxUnsubmitted
Not Done

] is still at the wrong place, I think. Should be Callable[[Union[bytes, str]], bytestr].
Then I still get the "Name 'bytestr' is not defined" error mentioned above.

dlax: `]` is still at the wrong place, I think. Should be `Callable[[Union[bytes, str]], bytestr]`.
dlaxUnsubmitted
Not Done

Now the type: is missing, so the comment is ignored.

Adding it, pytype mercurial/pycompat.py gives:

mercurial/pycompat.py", line 305, in <module>: Invalid type comment: Callable[[Union[bytes, str]], bytestr] [invalid-type-comment]
  Name 'bytestr' is not defined

hence the kind of trick I suggested in my first comment on this line.

dlax: Now the `type: ` is missing, so the comment is ignored. Adding it, `pytype mercurial/pycompat.
delattr = _wrapattrfunc(builtins.delattr) delattr = _wrapattrfunc(builtins.delattr)
getattr = _wrapattrfunc(builtins.getattr) getattr = _wrapattrfunc(builtins.getattr)
hasattr = _wrapattrfunc(builtins.hasattr) hasattr = _wrapattrfunc(builtins.hasattr)
setattr = _wrapattrfunc(builtins.setattr) setattr = _wrapattrfunc(builtins.setattr)
xrange = builtins.range xrange = builtins.range
unicode = str unicode = str
def open(name, mode=b'r', buffering=-1, encoding=None): def open(name, mode=b'r', buffering=-1, encoding=None):
else: else:
import cStringIO import cStringIO
xrange = xrange xrange = xrange
unicode = unicode unicode = unicode
bytechr = chr bytechr = chr
byterepr = repr byterepr = repr
bytestr = str _bytestr = str
dlaxUnsubmitted
Not Done

same here about missing ].

dlax: same here about missing `]`.
iterbytestr = iter iterbytestr = iter
maybebytestr = identity maybebytestr = identity
sysbytes = identity sysbytes = identity
sysstr = identity sysstr = identity
strurl = identity strurl = identity
bytesurl = identity bytesurl = identity
open = open open = open
delattr = delattr delattr = delattr
def namedtempfile( def namedtempfile(
mode=b'w+b', bufsize=-1, suffix=b'', prefix=b'tmp', dir=None, delete=True mode=b'w+b', bufsize=-1, suffix=b'', prefix=b'tmp', dir=None, delete=True
): ):
mode = sysstr(mode) mode = sysstr(mode)
assert r'b' in mode assert r'b' in mode
return tempfile.NamedTemporaryFile( return tempfile.NamedTemporaryFile(
mode, bufsize, suffix=suffix, prefix=prefix, dir=dir, delete=delete mode, bufsize, suffix=suffix, prefix=prefix, dir=dir, delete=delete
) )
def bytestr(s=b''):
# type: (Union[bytes,str]) -> _bytestr
return _bytestr(s)

mercurial/urllibcompat.py

) )
# urllib.parse.quote() accepts both str and bytes, decodes bytes # urllib.parse.quote() accepts both str and bytes, decodes bytes
# (if necessary), and returns str. This is wonky. We provide a custom # (if necessary), and returns str. This is wonky. We provide a custom
# implementation that only accepts bytes and emits bytes. # implementation that only accepts bytes and emits bytes.
def quote(s, safe=r'/'): def quote(s, safe=r'/'):
# bytestr has an __iter__ that emits characters. quote_from_bytes() # bytestr has an __iter__ that emits characters. quote_from_bytes()
# does an iteration and expects ints. We coerce to bytes to appease it. # does an iteration and expects ints. We coerce to bytes to appease it.
if isinstance(s, pycompat.bytestr): if isinstance(s, pycompat._bytestr):
s = bytes(s) s = bytes(s)
s = urllib.parse.quote_from_bytes(s, safe=safe) s = urllib.parse.quote_from_bytes(s, safe=safe)
return s.encode('ascii', 'strict') return s.encode('ascii', 'strict')
# urllib.parse.urlencode() returns str. We use this function to make # urllib.parse.urlencode() returns str. We use this function to make
# sure we return bytes. # sure we return bytes.
def urlencode(query, doseq=False): def urlencode(query, doseq=False):
s = urllib.parse.urlencode(query, doseq=doseq) s = urllib.parse.urlencode(query, doseq=doseq)