Page MenuHomePhabricator

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

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

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
305

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

@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
305

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

414

same here about missing ].

This revision now requires changes to proceed.Nov 19 2019, 11:39 AM
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#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
305

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

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?

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.