Patchwork D7296: pycompat: kludge around pytype being confused by __new__

login
register
mail settings
Submitter phabricator
Date Nov. 6, 2019, 11:24 p.m.
Message ID <differential-rev-PHID-DREV-jc2oxczrbnmeywjmxqca-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/42854/
State New
Headers show

Comments

phabricator - Nov. 6, 2019, 11:24 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We add a function's worth of indirection to construction, but in so
  doing can properly annotate the factory function's types, obviating
  some annoying messes.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

AFFECTED FILES
  mercurial/pycompat.py
  mercurial/urllibcompat.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 7, 2019, 8:38 a.m.
indygreg added a comment.


  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...

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - Nov. 7, 2019, 4:07 p.m.
durin42 added a comment.


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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - Nov. 8, 2019, 4:48 p.m.
This revision now requires changes to proceed.
indygreg added a comment.
indygreg requested changes to this revision.


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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Nov. 14, 2019, 11:05 a.m.
dlax added a comment.


    class bytestr(bytes):  # type: (Union[bytes,str]) -> bytestr
      [...]
  
  Does this work?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg
Cc: mjpieters, dlax, indygreg, mercurial-devel
phabricator - Nov. 15, 2019, 11:03 a.m.
This revision now requires changes to proceed.
dlax added a comment.
dlax requested changes to this revision.


  Rather `class bytestr(bytes):  # type: Callable[[Union[bytes, str], bytestr]` as @yuya suggested in D7380 <https://phab.mercurial-scm.org/D7380>.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: mjpieters, dlax, indygreg, mercurial-devel
phabricator - Nov. 15, 2019, 5:47 p.m.
dlax added a comment.


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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: mjpieters, dlax, indygreg, mercurial-devel
phabricator - Nov. 16, 2019, 9:28 a.m.
This revision now requires changes to proceed.
dlax added inline comments.
dlax requested changes to this revision.

INLINE COMMENTS

> pycompat.py:294
>  
> +    bytestr = _bytestr  # type: Callable[[Union[bytes, str], bytestr]
> +

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]

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: mjpieters, dlax, indygreg, mercurial-devel
phabricator - Nov. 19, 2019, 3:47 a.m.
indygreg added a comment.


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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: mjpieters, dlax, indygreg, mercurial-devel
phabricator - Nov. 19, 2019, 4:39 p.m.
This revision now requires changes to proceed.
dlax added a comment.
dlax requested changes to this revision.


  Sorry, still not ok afaict :/

INLINE COMMENTS

> pycompat.py:305
>  
> +    bytestr = _bytestr  # type: Callable[[Union[bytes, str], bytestr]]
> +

`]` 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.

> pycompat.py:414
>      byterepr = repr
> -    bytestr = str
> +    bytestr = str  # type: Callable[[Union[bytes, str], bytestr]
>      iterbytestr = iter

same here about missing `]`.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: mjpieters, dlax, indygreg, mercurial-devel
phabricator - Nov. 19, 2019, 6:21 p.m.
durin42 added a comment.


  In D7296#109672 <https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: mjpieters, dlax, indygreg, mercurial-devel
phabricator - Nov. 19, 2019, 8:10 p.m.
dlax added a comment.
dlax added a subscriber: yuja.


  In D7296#109683 <https://phab.mercurial-scm.org/D7296#109683>, @durin42 wrote:
  
  > In D7296#109672 <https://phab.mercurial-scm.org/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 <https://github.com/python/typeshed/blob/34d68ab0a2117a08fa221d3a10884f35cacf2cdc/stdlib/2and3/builtins.pyi#L559>). So I suspect the "Callable" trick is not enough and we'd need a workaround similar to da925257 <https://phab.mercurial-scm.org/rHGda925257a39e5797e5b2e35ce1d68e923ea8ddf2> by @yuja .

INLINE COMMENTS

> pycompat.py:305
>  
> +    bytestr = _bytestr  # Callable[[Union[bytes, str]], bytestr]
> +

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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: yuja, mjpieters, dlax, indygreg, mercurial-devel
phabricator - Nov. 19, 2019, 8:14 p.m.
dlax added a comment.


  In D7296#109684 <https://phab.mercurial-scm.org/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/blob/34d68ab0a2117a08fa221d3a10884f35cacf2cdc/stdlib/2and3/builtins.pyi#L559>).
  
  https://github.com/python/typeshed/issues/2630

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: yuja, mjpieters, dlax, indygreg, mercurial-devel
phabricator - Jan. 30, 2020, 6:30 p.m.
marmoute added a comment.


  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?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: marmoute, yuja, mjpieters, dlax, indygreg, mercurial-devel
phabricator - March 19, 2020, 11:14 p.m.
marmoute added a comment.


  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 ?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: marmoute, yuja, mjpieters, dlax, indygreg, mercurial-devel
phabricator - March 20, 2020, 3:59 p.m.
durin42 added a comment.
durin42 planned changes to this revision.


  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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7296/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7296

To: durin42, #hg-reviewers, indygreg, dlax
Cc: marmoute, yuja, mjpieters, dlax, indygreg, mercurial-devel

Patch

diff --git a/mercurial/urllibcompat.py b/mercurial/urllibcompat.py
--- a/mercurial/urllibcompat.py
+++ b/mercurial/urllibcompat.py
@@ -105,7 +105,7 @@ 
     def quote(s, safe=r'/'):
         # bytestr has an __iter__ that emits characters. quote_from_bytes()
         # 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 = urllib.parse.quote_from_bytes(s, safe=safe)
         return s.encode('ascii', 'strict')
diff --git a/mercurial/pycompat.py b/mercurial/pycompat.py
--- a/mercurial/pycompat.py
+++ b/mercurial/pycompat.py
@@ -154,7 +154,7 @@ 
     bytechr = struct.Struct(r'>B').pack
     byterepr = b'%r'.__mod__
 
-    class bytestr(bytes):
+    class _bytestr(bytes):
         """A bytes which mostly acts as a Python 2 str
 
         >>> bytestr(), bytestr(bytearray(b'foo')), bytestr(u'ascii'), bytestr(1)
@@ -208,7 +208,7 @@ 
         """
 
         def __new__(cls, s=b''):
-            if isinstance(s, bytestr):
+            if isinstance(s, _bytestr):
                 return s
             if not isinstance(
                 s, (bytes, bytearray)
@@ -398,7 +398,7 @@ 
     unicode = unicode
     bytechr = chr
     byterepr = repr
-    bytestr = str
+    _bytestr = str
     iterbytestr = iter
     maybebytestr = identity
     sysbytes = identity
@@ -504,3 +504,8 @@ 
     return tempfile.NamedTemporaryFile(
         mode, bufsize, suffix=suffix, prefix=prefix, dir=dir, delete=delete
     )
+
+
+def bytestr(s=b''):
+    # type: (Union[bytes,str]) -> _bytestr
+    return _bytestr(s)