Make code.FormattedExcinfo.get_source more defensive #8227
Conversation
When line_index was a large negative number, get_source failed on `source.lines[line_index]`. Use the same dummy Source as with a large positive line_index.
| @@ -721,11 +721,11 @@ def get_source( | |||
| ) -> List[str]: | |||
| """Return formatted and marked up source lines.""" | |||
| lines = [] | |||
| if source is None or line_index >= len(source.lines): | |||
| if source is not None and line_index < 0: | |||
| line_index += len(source.lines) | |||
There was a problem hiding this comment.
I think this is insufficient still -- for example if len(source.lines) is 4 and line_index is -900 -- you could use % but it would probably be a good idea to figure out why these are negative at all (or refuse to produce source when the number is negative)
There was a problem hiding this comment.
That's why there's line_index < 0 in the next condition :)
I'll try to investigate why this happens in the setuptools tests.
|
I have encountered the same error on Python 3.10, with a test using the I can induce the crash with: import ast
import pytest
def test_literal_eval():
with pytest.raises(ValueError, match="^$"):
ast.literal_eval("pytest")On Python 3.9 and below the above test fails but pytest doesn't crash. Digging into the code with PDB the traceback passed to |
|
We see more packages affected by this: e.g. parso It makes testing on 3.10 very hard because you cannot tell why a test failed. Can we get this merged please? What is missing? |
|
is it a regression in python 3.10? if so we should raise that before the beta period and probably get it fixed there instead of here |
|
I am not sure. Would it help if I bisected the cpython commit that changed this? |
|
i do wonder if there is any way to unittest this |
|
we should be able to use the example from the comment here, no? |
|
I'll try to submit a test here. |
|
probably, btw - @hroncok i think its possible to temporary work around this using --tb=native |
|
Yes, Test added here. I had a hard time naming it and describing it. Suggestions welcome. It fails without @encukou's patch and succeeds with it. |
hroncok
force-pushed
the
defensive-get_source
branch
from
f5844a6 to
84c2e20
Compare
hroncok
force-pushed
the
defensive-get_source
branch
from
84c2e20 to
0a75c8e
Compare
|
imho this is goo for now - i'd like @asottile to lean in and i think a upstream report is necessary (and to link it in here and the code) |
|
haven't had a chance to dig into the actual bug, but here's the bisection failure: python/cpython@3bd6035 python/cpython#24202 |
imo if we merge it I think we should plan to revert it when upstream fixes the issue |
Sure. 👍 |
|
here's the bpo issue, this is definitely a regression in 3.10 and I don't think pytest should make a code change for it: https://bugs.python.org/issue43933 here's a minimal reproduction for example without pytest: class Boom:
def __enter__(self):
return self
def __exit__(self, *_):
raise AssertionError('boom!')
def main() -> int:
with Boom():
raise AssertionError('hi')
if __name__ == '__main__':
exit(main())$ python3.10 t.py
Traceback (most recent call last):
File "/home/asottile/workspace/cpython/t.py", line 10, in main
raise AssertionError('hi')
AssertionError: hi
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/asottile/workspace/cpython/t.py", line 14, in <module>
exit(main())
File "/home/asottile/workspace/cpython/t.py", line -1, in main
File "/home/asottile/workspace/cpython/t.py", line 5, in __exit__
raise AssertionError('boom!')
AssertionError: boom!notice we get a frame with a |
|
imho we should, as broken pytest by python breaks other testing on that python |
it's marked as a release blocker so this will never appear in actual released versions. note also that it doesn't break all testing of python, just for a very particular subset of exceptional cases |
I agree with that sentiment. To make all points clear:
I think we should help downstream packagers here, it doesn't really cost us much. |
|
alright, I'll handle reverting once this is fixed 👍 |
|
Thank you all. |
Great, thanks! |
Make code.FormattedExcinfo.get_source more defensive (cherry picked from commit 67af623)
Make code.FormattedExcinfo.get_source more defensive (cherry picked from commit 67af623) (Michał Górny: removed type hints for 6.2.x)
|
this has been fixed upstream so here's the revert (I kept the test): #8729 |
Revert "Merge pull request #8227 from encukou/defensive-get_source"
|
I'm seeing this again with Python 3.10.0b3+ (heads/3.10:1b4addf3cb, Jun 19 2021, 01:52:02) [Clang 12.0.0 (clang-1200.0.32.29)] pytest log |
|
@brechtm if you could share the code that would be helpful |
|
assuming this is a recent regression -- I've pinged this issue on this bpo: https://bugs.python.org/issue44297 |
Not sure whether you mean this: |
|
here's the underlying traceback -- looks like |
|
To be sure, I checked with Python 3.10.0b2. No issues there. |
|
I think you're actually hitting the cross section of one (annoying, intentional?) importlib.metadata breaking change as well as a regression in the traceback lines -- it appears importlib.metadata in 3.10b3 is unhappy with the custom |
|
@asottile Wow, thanks for the analysis! I guess I'll have to set a name... |
I am trying to investigate a bug in Setuptools, and I ran into this internal error in pytest:
Full traceback...
When line_index was a large negative number, get_source failed on
source.lines[line_index].I don't (yet) understand the underlying error (Setuptools does some creative backwards-compatibility stunts with the import machinery), but the pytest code looks like it wants to be defensive here. Possibly, using the same dummy Source as with a large positive line_index is correct.
Sorry for not including tests; I don't know how to test it properly. This is more of a bug report with attached code than a proper pull request :)
It seems related to #752 (but the pylib link there is dead).