Skip to content

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented May 1, 2025

fixes #263

I came up with another false alarm that requires a bigger fix, so leaving that one as a tomorrow problem. It appears that 2.5 years is past my limit of fully remembering complicated details of the innards of this code base 😅

@Zac-HD
Copy link
Member

Zac-HD commented May 1, 2025

I wonder if it'd be easier to scan the body of the nursery block searching for yields, rather than saving potential errors to emit later? Small performance hit, but much easier to implement and I'd be less worried about subtle bugs.

Separately, I'd love to test on a nursery where we yield and then start_soon a task.

@jakkdl
Copy link
Member Author

jakkdl commented May 2, 2025

I wonder if it'd be easier to scan the body of the nursery block searching for yields, rather than saving potential errors to emit later? Small performance hit, but much easier to implement and I'd be less worried about subtle bugs.

eyy, this cracked it. And with setting no_visit if there's no yield I think this is a performance gain. and it fixed another false alarm, though one that relies on pretty egregious code.
...
and it let me remove the other code that led to other missed alarms since manually visiting doesn't let the TypeTrackerVisitor interject. bishbashbosh perfect fix

Separately, I'd love to test on a nursery where we yield and then start_soon a task.

👍

@jakkdl jakkdl marked this pull request as ready for review May 2, 2025 13:36
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

needs changelog etc, then merge 🚀

@jakkdl jakkdl enabled auto-merge (squash) May 5, 2025 08:56
@jakkdl jakkdl merged commit 9935ce6 into python-trio:main May 5, 2025
10 checks passed
@jakkdl jakkdl deleted the async133_false_alarm branch May 5, 2025 09:02
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.

reduce false alarms for ASYNC113

2 participants