Use TypeVarTuple in our APIs #2881

TeamSpen210 · 2023-11-19T01:30:59Z

With Mypy 1.7 being released, variadic generics are now supported. This works real well for Nursery.start_soon(), to_thread.run_sync(), etc. However the complex types for Nursery.start() + TaskStatus are producing a bunch of spurious errors for code that just calls it and ignores the return type. That might need to be fixed first in Mypy...

codecov · 2023-11-19T01:32:20Z

Codecov Report

Merging #2881 (9f94f97) into master (31b87ad) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2881   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files         115      115           
  Lines       17663    17673   +10     
  Branches     3158     3159    +1     
=======================================
+ Hits        17583    17593   +10     
  Misses         52       52           
  Partials       28       28

Files	Coverage Δ
src/trio/_core/_entry_queue.py	`100.00% <100.00%> (ø)`
src/trio/_core/_run.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_guest_mode.py	`100.00% <ø> (ø)`
src/trio/_core/_tests/test_local.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_run.py	`100.00% <ø> (ø)`
src/trio/_dtls.py	`96.76% <100.00%> (ø)`
src/trio/_subprocess.py	`98.29% <100.00%> (+0.02%)`	⬆️
src/trio/_tests/test_channel.py	`100.00% <100.00%> (ø)`
src/trio/_tests/test_scheduler_determinism.py	`100.00% <100.00%> (ø)`
src/trio/_tests/test_subprocess.py	`100.00% <100.00%> (ø)`
... and 3 more

src/trio/_core/_run.py

A5rocks · 2023-11-19T01:51:45Z

Could you add some type tests?

A5rocks · 2023-11-19T07:24:15Z

src/trio/_core/_run.py

+        """Type of functions passed to `nursery.start() <trio.Nursery.start>`."""
+
+        def __call__(
+            self, *args: Unpack[PosArgT], task_status: TaskStatus[StatusT_co]


Suggested change

self, *args: Unpack[PosArgT], task_status: TaskStatus[StatusT_co]

self, *args: Unpack[PosArgT], *, task_status: TaskStatus[StatusT_co]

edit: I thought that would work (and it does if *args: Unpack[PosArgT] is gone) but it doesn't. I believe the issue is that the type var tuple can have a finite amount of arguments and then... kinda overflow, at least in some cases right? So like, calling with int, str, TaskStatus[str] when PosArgT is just [int, str] would mean that StatusT_co would be str right?

Errr, so we're promising that anything with this protocol can call it as such. But we're also passing functions that don't allow that? (actually, maybe not, and that's an actual bug. But as is our protocol shouldn't accept (a: int, *, task_status: TaskStatus[str]))

edit edit: ok my analysis above is totally incorrect, only keeping it to prevent someone from going down the same path i went down.

Unfortunately no, the *args already makes it keyword-only.

Yeah sorry about the repeated confusion with this thread, TypeVarTuple is getting to my head :(

Yeah it's real confusing. Since task_status is keyword-only it shouldn't get mixed up. As you can see with the type-tests, if it's being assigned to an annotated type, Mypy can do bidirectional inference and figure it out. But if it's not annotated or just a bare call, it gets confused and assumes it's Never. I think it's a bug.

I agree. I took a look at mypy's code for the last 45 minutes and spotted:

It looks like it isn't generating constraints for the typevar. (inference works by figuring out all constraints on a typevar and then constraining it -- if there are no constraints, then it returns UninhabitedType (str value: Never ... not literally Never) to us)

This is because the constraints only get generated for a callback protocol if the type being assigned is a subtype of the callback protocol w/ erased tvars (I assume this is to make less confusing error messages). Unfortunately, TypeVarTuple gets erased to tuple[Any, ...] and our callable doesn't take in infinite parameters.

Even if that problem were to be solved, it looks like mypy only generates constraints for either a bunch of type vars or a single unpack. This isn't a problem normally (Callable[[Unpack[Ts], T], None] has an implicit Unpack) but means that mypy focuses on the typevartuple, not making a constraint for our type variable.

I'm not really sure how to solve this myself, a bug report would probably work nicely.

@A5rocks probably worth cross-posting your analysis in the issue that teamspen opened.

jakkdl · 2023-11-22T13:54:49Z

for this PR: probably worth merging the signatures that work / type:ignoring the fails, so we don't have this lying around as a draft until the bug is resolved in mypy and get the incremental improvement. Especially as it pertains to deprecation of trio-typing

A5rocks · 2023-11-22T14:42:14Z

I think it depends on whether we want to release our next bug fix release sooner or later. Specifically due to the pyright issue w/ current release. Might not be good to have regressions in a common function!

TeamSpen210 · 2023-11-23T00:49:27Z

I'll split off the start() annotations to their own PR, since that's the only part that's not working right.

TeamSpen210 · 2023-11-28T03:58:04Z

I removed the start() signatures, making Mypy pass this fine. However, Pyright seems to be failing - if I do the full check offline, it seems to not be recognising typing_extensions.Unpack as something special?

A5rocks

I'm not sure why you're getting that from pyright, given it accepts this following program in VSCode on my machine:

from typing import Callable
from typing_extensions import TypeVarTuple, Unpack

Ts = TypeVarTuple("Ts")

def x(f: Callable[[Unpack[Ts]], None], *args: Unpack[Ts]) -> None:
    return f(*args)

x(lambda x: print(x + 42), 42)

---

re: pyright looks like it's not seeing the TypeVarTuple right?

pyproject.toml

A5rocks · 2023-11-28T05:00:24Z

Looks like pyright just hates the name PosArgT? If I ctrl+f and replace then all with PosArgTs then it all works ...???

EDIT: looks like it hates the import from _generated_run.py. If you remove that then it works.

A5rocks · 2023-11-30T06:42:47Z

Just a bump on this before I review!:

(Also I think you should add this to check.sh under the existing pyright run on type tests: pyright src/trio/_core/_tests/type_tests || EXIT_STATUS=$?)

edit: plus, now that I look at it, a newsfragment would be useful too!

…Tuple

TeamSpen210 · 2023-12-02T06:33:39Z

We may want to hold off on merging until the next Pyright version. It’ll have support for handling a callable with default arguments, making those optional parts of the TypeVarTuple. (microsoft/pyright#6623) We can then enable the test for that.

A5rocks · 2023-12-02T08:47:38Z

Pyright goes out in a weekly release and iirc pylance will pick it up in a month -- I think it's fine to merge and disregard this specific edge case that will resolve itself in <1 month.

(Hopefully I'm remembering my release schedules right)

TeamSpen210 · 2023-12-02T09:25:54Z

In bbd436d I added a pyright:ignore, since it was failing the test case I added for this. So we'd want to wait, so I can remove that ignore. Otherwise it'll start failing next time we update Pyright.

jakkdl · 2023-12-02T11:50:07Z

eh, we can just remove the ignore when updating it - that's probably less work than maintaining merge conflicts

A5rocks · 2023-12-06T05:56:48Z

Well pyright released a new version so... (I'll go run autodeps rn)

A5rocks

Looks good!

pyproject.toml

CoolCat467 · 2023-12-06T21:44:31Z

Oop, there's that segfault issue in pypy3.9 again. https://github.com/python-trio/trio/actions/runs/7120585043/job/19388115101

Co-authored-by: EXPLOSION <git@helvetica.moe>

A5rocks · 2023-12-07T02:13:05Z

Oop, there's that segfault issue in pypy3.9 again. https://github.com/python-trio/trio/actions/runs/7120585043/job/19388115101

This one's different from the other one you might be thinking about (iirc?) and it's one where I've provided more info in the issue so hopefully it'll resolve itself soon!

A5rocks · 2023-12-13T02:25:11Z

Mind checking my changes seem fine then merging? @TeamSpen210

Zac-HD · 2023-12-13T08:43:16Z

Woohoo! Can we release this soon, to get the lovely typing benefits without waiting to sort out the ExceptionGroup transitions?

TeamSpen210 added 7 commits November 19, 2023 09:46

Use TypeVarTuple in various functions

bd5a451

Use TypeVarTuple in _run

97d6e0e

Cast this partial() definition, we know it's safe

9bcb185

Fix some uncovered type errors

6f8153f

This now passes mypy checking

52c69c8

Fix return types here

13d5d63

Fix more errors

9ed1d08

TeamSpen210 added the typing Adding static types to trio's interface label Nov 19, 2023

Bump Mypy version

0933718

TeamSpen210 linked an issue Nov 19, 2023 that may be closed by this pull request

TypeVarTuple support #2859

Closed

A5rocks reviewed Nov 19, 2023

View reviewed changes

src/trio/_core/_run.py Show resolved Hide resolved

TeamSpen210 added 2 commits November 19, 2023 13:31

Fix docs failure

a0e9966

Add type tests for Nursery.start[_soon] methods

5899ad4

A5rocks reviewed Nov 19, 2023

View reviewed changes

TeamSpen210 mentioned this pull request Nov 19, 2023

Never inferred in complex situation with variadic callable protocol python/mypy#16522

Open

TeamSpen210 added 5 commits November 23, 2023 12:12

Revert using TypeVarTuple for Nursery.start()

e12273f

This seems to be a mypy bug?

5a39c8b

This type-error is resolved in Mypy master

5b0215c

Add missing import for PosArgT in _generated_run

342733c

The StatusT typevar can't be used here without TVT

749ef94

TeamSpen210 force-pushed the use-typevartuple branch from e7eb824 to 749ef94 Compare November 23, 2023 03:12

TeamSpen210 marked this pull request as ready for review November 28, 2023 03:58

A5rocks reviewed Nov 28, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

TeamSpen210 requested review from A5rocks and jakkdl November 30, 2023 06:41

TeamSpen210 added 3 commits November 30, 2023 17:09

Merge branch 'master' into use-typevartuple

ff01a82

Run _core/_tests/type_tests under Pyright also

b91b4b9

Add newsfragment

972961e

TeamSpen210 force-pushed the use-typevartuple branch from 9bf6a36 to 972961e Compare November 30, 2023 08:51

TeamSpen210 added 4 commits November 30, 2023 19:20

Default args are intentionally not considered by Pyright with TypeVar…

bbd436d

…Tuple

Type tests shouldn't be checked by coverage

bac6558

Handle Enum.__signature__ not being detected by Mypy

9783f8b

Mypy does see copy/deepcopy now, this is no longer required

e1edda0

jakkdl approved these changes Dec 1, 2023

View reviewed changes

A5rocks approved these changes Dec 6, 2023

View reviewed changes

pyproject.toml Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

CoolCat467 added 2 commits December 6, 2023 15:31

Merge branch 'master' into use-typevartuple

176092b

Fix type issue from merge

2903a59

Remove unnecessary ignore

d9d7a3d

Co-authored-by: EXPLOSION <git@helvetica.moe>

A5rocks added 2 commits December 12, 2023 21:18

Merge branch 'master' into use-typevartuple

580627d

Pyright-related changes

e012611

Fix strict-mode error

9f94f97

TeamSpen210 merged commit 597e345 into python-trio:master Dec 13, 2023
29 checks passed

TeamSpen210 deleted the use-typevartuple branch December 13, 2023 04:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use TypeVarTuple in our APIs #2881

Use TypeVarTuple in our APIs #2881

TeamSpen210 commented Nov 19, 2023

codecov bot commented Nov 19, 2023 •

edited

A5rocks commented Nov 19, 2023

A5rocks Nov 19, 2023 •

edited

TeamSpen210 Nov 19, 2023

A5rocks Nov 19, 2023

TeamSpen210 Nov 19, 2023

A5rocks Nov 19, 2023 •

edited

jakkdl Nov 22, 2023

jakkdl commented Nov 22, 2023

A5rocks commented Nov 22, 2023

TeamSpen210 commented Nov 23, 2023

TeamSpen210 commented Nov 28, 2023

A5rocks left a comment •

edited

A5rocks commented Nov 28, 2023 •

edited

A5rocks commented Nov 30, 2023 •

edited

TeamSpen210 commented Dec 2, 2023

A5rocks commented Dec 2, 2023 •

edited

TeamSpen210 commented Dec 2, 2023

jakkdl commented Dec 2, 2023

A5rocks commented Dec 6, 2023 •

edited

A5rocks left a comment

CoolCat467 commented Dec 6, 2023

A5rocks commented Dec 7, 2023

A5rocks commented Dec 13, 2023

Zac-HD commented Dec 13, 2023

	self, *args: Unpack[PosArgT], task_status: TaskStatus[StatusT_co]
	self, args: Unpack[PosArgT], , task_status: TaskStatus[StatusT_co]

Use TypeVarTuple in our APIs #2881

Use TypeVarTuple in our APIs #2881

Conversation

TeamSpen210 commented Nov 19, 2023

codecov bot commented Nov 19, 2023 • edited

Codecov Report

A5rocks commented Nov 19, 2023

A5rocks Nov 19, 2023 • edited

Choose a reason for hiding this comment

TeamSpen210 Nov 19, 2023

Choose a reason for hiding this comment

A5rocks Nov 19, 2023

Choose a reason for hiding this comment

TeamSpen210 Nov 19, 2023

Choose a reason for hiding this comment

A5rocks Nov 19, 2023 • edited

Choose a reason for hiding this comment

jakkdl Nov 22, 2023

Choose a reason for hiding this comment

jakkdl commented Nov 22, 2023

A5rocks commented Nov 22, 2023

TeamSpen210 commented Nov 23, 2023

TeamSpen210 commented Nov 28, 2023

A5rocks left a comment • edited

Choose a reason for hiding this comment

A5rocks commented Nov 28, 2023 • edited

A5rocks commented Nov 30, 2023 • edited

TeamSpen210 commented Dec 2, 2023

A5rocks commented Dec 2, 2023 • edited

TeamSpen210 commented Dec 2, 2023

jakkdl commented Dec 2, 2023

A5rocks commented Dec 6, 2023 • edited

A5rocks left a comment

Choose a reason for hiding this comment

CoolCat467 commented Dec 6, 2023

A5rocks commented Dec 7, 2023

A5rocks commented Dec 13, 2023

Zac-HD commented Dec 13, 2023

codecov bot commented Nov 19, 2023 •

edited

A5rocks Nov 19, 2023 •

edited

A5rocks Nov 19, 2023 •

edited

A5rocks left a comment •

edited

A5rocks commented Nov 28, 2023 •

edited

A5rocks commented Nov 30, 2023 •

edited

A5rocks commented Dec 2, 2023 •

edited

A5rocks commented Dec 6, 2023 •

edited