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

Add a sanity check for a missing derive for exception groups #3176

Closed

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Dec 30, 2024

Fixes #3175, probably.

Just adds a sanity check. I started thinking about doing our own .split() but then I decided that's a lot. I also looked at just using .derive at the end but the exception groups without .derive() can be anywhere in the exception group contents.

I made python-trio/flake8-async#334 for the linter rule side of this, but I think this is probably easy enough to forget especially if trying out this feature (and easy to overlook).

@TeamSpen210
Copy link
Contributor

Seems rather expensive to put in CancelScope.__exit__? Not sure if it really makes any sense to be here in particular either, since exception groups can be raised anywhere, and they're not Trio-specific either.

@A5rocks
Copy link
Contributor Author

A5rocks commented Dec 30, 2024

Seems rather expensive to put in CancelScope.__exit__?

Maybe the runtime cost is too high yeah.

Not sure if it really makes any sense to be here in particular either, since exception groups can be raised anywhere, and they're not Trio-specific either.

That's the only place in trio where we call exception_group.derive (indirectly) from collapse_exception_group or exception_group.split. In addition, it's probably a good idea to let users know, even if this might not be their first encounter.

@A5rocks A5rocks force-pushed the sanity-check-exception-group-derive branch from 2a14f10 to d8e853f Compare December 30, 2024 08:36
Copy link

codecov bot commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 96.22642% with 2 lines in your changes missing coverage. Please review.

Project coverage is 99.98920%. Comparing base (3de7996) to head (d8e853f).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/trio/_core/_run.py 81.81818% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##                 main       #3176         +/-   ##
====================================================
- Coverage   100.00000%   99.98920%   -0.01081%     
====================================================
  Files             124         124                 
  Lines           18458       18510         +52     
  Branches         1215        1222          +7     
====================================================
+ Hits            18458       18508         +50     
- Misses              0           1          +1     
- Partials            0           1          +1     
Files with missing lines Coverage Δ
src/trio/_core/_tests/test_run.py 100.00000% <100.00000%> (ø)
src/trio/_core/_run.py 99.80564% <81.81818%> (-0.19437%) ⬇️

@jakkdl
Copy link
Member

jakkdl commented Dec 30, 2024

I'm on the fence on whether we should check for this at runtime at all, esp when it gets this messy. And making people add derive to suppress the warning is quite a bit more expensive then adding a noqa.

On the other hand this would catch cases in libraries not running an appropriate linter, and an end user using that library+trio. But the error doesn't feel very trio-specific in the first case.

@A5rocks
Copy link
Contributor Author

A5rocks commented Dec 30, 2024

Actually this would false positive if someone mirrors the BaseExceptionGroup<->ExceptionGroup structure (where only BaseExceptionGroup has a derive)...

So maybe this isn't that useful of something to always warn about. I don't think everyone uses linters but I suppose if someone needs a custom ExceptionGroup odds are they use a linter?

@oremanj
Copy link
Member

oremanj commented Dec 30, 2024

I don't think this is Trio's job to detect, especially since the implementation is complicated and likely to be slow. I would much prefer to see us address the hazard with a lint, and/or a CPython change to warn in BaseExceptionGroup.__init_subclass__ if a subclass does not define derive.

@A5rocks
Copy link
Contributor Author

A5rocks commented Dec 31, 2024

I do think it's trio's place to try to prevent footguns (or even common mistakes) and I do think this qualifies. But I'll close this because:

  • runtime cost of doubling (maybe?) execution time for exiting cancel scopes upon errors for what should be a very rare occurrence
  • false positives are onerous and likely common, given how even ExceptionGroup doesn't implement derive

Mostly the second reason though. If someone comes up with a runtime thing that does better, I would love another attempt at this -- I totally could see myself ignoring documentation when playing around with a new thing (given I don't have linting in my REPL and I don't run lints until committing).

@A5rocks A5rocks closed this Dec 31, 2024
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.

nursery replaces custom ExceptionGroup subclasses with plain ExceptionGroups
4 participants