Skip to content

Conversation

@solaluset
Copy link
Contributor

Hello. I develop a project that heavily depends on code modification and monkeypatching: https://github.com/solaluset/pwcp/tree/pytest-broken

In some cases when things get wrong (as replicated in pytest-broken branch), pytest terminates with a long traceback of INTERNALERROR> IndexError: list index out of range. Change in this PR ensures end will never be out of range. With this fix applied, pytest properly shows that the error is TypeError: _maybe_compile() takes 3 positional arguments but 4 were given.

This is probably a niche case, but extra out-of-bounds check won't hurt anyone, right?

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @solaluset!

Usually we request a test, specially to avoid possible regressions in the future. Can you think of a simple test case to add to the test suite with that intent?

@solaluset
Copy link
Contributor Author

Thank you for your reply. I thought about that, but not sure what would be a minimal reproducible example. I'll look into it.

@solaluset
Copy link
Contributor Author

Uh, pre commit messed it up

@solaluset
Copy link
Contributor Author

This is the reason why first line of function must start with non-space or be empty

# If we start with an indented line, put blockfinder to "started" mode.
block_finder.started = (
bool(source.lines[start]) and source.lines[start][0].isspace()
)

Otherwise end gets corrected and doesn't raise IndexError.
This is so tricky, I'm not sure if my solution is optimal.

@solaluset
Copy link
Contributor Author

@nicoddemus do I need to do anything else?

@Pierre-Sassoulas
Copy link
Member

Hey @solaluset, thank you for the PR and the automated test. Shouldn't getstatementrange_ast be fixed at the source instead of special casing a first empty line elsewhere ? Also could you add a changelog, please ?

@solaluset
Copy link
Contributor Author

Thanks for the response. I'm not sure I get your first point. Do you mean block_finder.started should always be false? As for the changelog, do I use the id of this PR?

@Pierre-Sassoulas
Copy link
Member

I'm sorry I think I was still half asleep when doing this review because I don't understand what I meant at the time now. 😕

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I think we still need a changelog though.

@solaluset
Copy link
Contributor Author

As there's no connected issue, do I use the id of this PR?

@Pierre-Sassoulas
Copy link
Member

Yes, please.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Nov 20, 2025
@Pierre-Sassoulas Pierre-Sassoulas enabled auto-merge (squash) November 20, 2025 21:34
@Pierre-Sassoulas Pierre-Sassoulas merged commit c4142af into pytest-dev:main Nov 21, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants