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

avoid using return t.cast which can prevent attribute access during process teardown #913

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

minrk
Copy link
Member

@minrk minrk commented Nov 28, 2024

return t.cast(..., X) is equivalent to return X # type:ignore on the result of the expression, but (negligibly) more expensive and can cause errors e.g. preventing access to attributes during __del__ in process teardown, as seen in jupyterhub/repo2docker#1379. It's a good idea to try to avoid anything that may prevent access to attribtues. This means trying our best to keep any module.method calls out of the __get__ path (note: importing from typing import cast is another way to avoid this specific issue and works for methods we really do want to use in this code, but the ignores just seem better than the casts to me, anyway).

Note this is only applied to return statements, since the type narrowing of t.cast necessarily has no effect there other than explicitly ignoring a single type check. Other uses of t.cast in the middle of some code for type narrowing are still totally sensible.

@minrk minrk added the bug label Nov 28, 2024
@minrk minrk force-pushed the no-return-cast branch 2 times, most recently from c437575 to c080d5b Compare November 28, 2024 10:52
return cast is equivalent to `type: ignore` on the result of the expression,
but more expensive and can cause errors e.g. preventing access to traits during __del__ in process teardown
Copy link
Member

@Carreau Carreau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to resolve conflicts, let me know if you agree.

I was just the warning in def isidentifier.

+1 to merge once test passes if you want to self merge.

Copy link
Member

@Carreau Carreau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good catch with the return.

@minrk minrk merged commit 3c1676d into ipython:main Feb 4, 2025
30 checks passed
@minrk minrk deleted the no-return-cast branch February 4, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants