Skip to content

assertion/rewrite: fix internal error on collection error due to decorated function#7749

Merged
nicoddemus merged 2 commits into
pytest-dev:masterfrom
bluetech:fix-get_source-crash
Sep 19, 2020
Merged

assertion/rewrite: fix internal error on collection error due to decorated function#7749
nicoddemus merged 2 commits into
pytest-dev:masterfrom
bluetech:fix-get_source-crash

Conversation

@bluetech

Copy link
Copy Markdown
Member

Fixes #4984.

Note: interesting change in the 2nd commit, first one is just preparation.

For decorated functions, the lineno of the FunctionDef AST node points to the def line, not to the first decorator line. On the other hand, in code objects, the co_firstlineno points to the first decorator line.

Assertion rewriting inserts some imports to code it rewrites. The imports are inserted at the lineno of the first statement in the AST. In turn, the code object compiled from the rewritten AST uses the lineno of the first statement (which is the first inserted import).

This means that given a module like this,

@foo
@bar
def baz(): pass

the lineno of the code object without assertion rewriting (--assertion=plain) is 1, but with assertion rewriting it is 3.

And this causes some issues for the exception repr when e.g. the decorator line is invalid and raises during collection. The code becomes confused and crashes with

INTERNALERROR>   File "_pytest/_code/code.py", line 638, in get_source
INTERNALERROR>     lines.append(space_prefix + source.lines[line_index].strip())
INTERNALERROR> IndexError: list index out of range

Fix it by special casing decorators. Maybe there are other cases like this but off hand I can't think of another Python construct where the lineno of the item would be after its first line, and this is the only such issue we have had reported.

…rated function

For decorated functions, the lineno of the FunctionDef AST node points
to the `def` line, not to the first decorator line. On the other hand,
in code objects, the `co_firstlineno` points to the first decorator
line.

Assertion rewriting inserts some imports to code it rewrites. The
imports are inserted at the lineno of the first statement in the AST. In
turn, the code object compiled from the rewritten AST uses the lineno of
the first statement (which is the first inserted import).

This means that given a module like this,

```py
@foo
@bar
def baz(): pass
```

the lineno of the code object without assertion rewriting
(`--assertion=plain`) is 1, but with assertion rewriting it is 3.

And *this* causes some issues for the exception repr when e.g. the
decorator line is invalid and raises during collection. The code becomes
confused and crashes with

INTERNALERROR>   File "_pytest/_code/code.py", line 638, in get_source
INTERNALERROR>     lines.append(space_prefix + source.lines[line_index].strip())
INTERNALERROR> IndexError: list index out of range

Fix it by special casing decorators. Maybe there are other cases like
this but off hand I can't think of another Python construct where the
lineno of the item would be after its first line, and this is the only
such issue we have had reported.

@DahlitzFlorian DahlitzFlorian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me. However, someone else should have a look at it, too.

@test14573 test14573 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

rthte

@pytest-dev pytest-dev deleted a comment from test14573 Sep 13, 2020

@nicoddemus nicoddemus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @bluetech!

@nicoddemus nicoddemus added the needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch label Sep 19, 2020
@nicoddemus nicoddemus merged commit 9bfd14a into pytest-dev:master Sep 19, 2020
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Sep 19, 2020
@nicoddemus

Copy link
Copy Markdown
Member

Backport: #7771

nicoddemus added a commit that referenced this pull request Sep 19, 2020
[6.0.x] Merge pull request #7749 from bluetech/fix-get_source-crash
@bluetech bluetech deleted the fix-get_source-crash branch October 1, 2020 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash with NameError through decorator during collection

4 participants