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

[flake8-annotations] Fix parsing of "complex" forward reference type annotations in any-type (ANN401) #14700

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sbrugman
Copy link
Contributor

@sbrugman sbrugman commented Dec 1, 2024

Summary

Fixes #14695.

The root cause was tricky to find.
"Complex" forward reference type annotations are handled differently. Forward references are "complex" in this case when the raw contents (without quotes) of the expression with the parsed contents is not contained in the string literal.

if raw_contents(expr_text)

Annotations are recursively parsed, for instance:
"'int'" (complex) => "int" (simple)

The raw contents of the expression "in\x74" is not contained in the string literal int and hence requires multiple complex parsing steps: "'in\x74'" (complex) -> "in\x74" (complex). This is rare, and probably that's why the bug went unnoticed.

Parsed type annotations are cached by their start offset:

.entry(annotation.start())

As it turned out, for these complex annotations the starting offset was not properly updated (removing the quotes), as well already do for the simple annotations. This caused an infinite loop, and eventually a stack overflow.

Test Plan

Extended the test suite with regression tests.

Copy link
Contributor

github-actions bot commented Dec 2, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@dhruvmanila
Copy link
Member

dhruvmanila commented Dec 2, 2024

Do you think this is an issue that should be solved holistically (#10586)? Because the relocate_expr is actually incorrect in the first place.

@dhruvmanila
Copy link
Member

As it turned out, for these complex annotations the starting offset was not properly updated (removing the quotes), as well already do for the simple annotations.

Sorry, I'm a bit confused still. Why are we using the range without quotes for the parsed expression in case of complex annotation? Because for simple annotations, we just use the range without quotes for the original string expression.

@dylwil3 dylwil3 added the parser Related to the parser label Dec 2, 2024
@dhruvmanila
Copy link
Member

I think an ideal solution here would be to fix the relocate_expr to transform the entire parsed tree accordingly and then move the range_exluding_quotes in parse_type_annotation which then passes to both parse_simple_type_annotation and parse_complex_type_annotation.

Comment on lines -96 to +106
relocate_expr(parsed.expr_mut(), string_expr.range());
// Remove quotes from nested forward reference type annotations
let range_excluding_quotes =
if let Expr::StringLiteral(ExprStringLiteral { ref value, .. }) = *parsed.syntax.body {
let string_parts = value.as_slice();
let start = string_parts[0].flags.opener_len();
let end = string_parts[string_parts.len() - 1].flags.closer_len();
string_expr.range().add_start(start).sub_end(end)
} else {
string_expr.range()
};
relocate_expr(parsed.expr_mut(), range_excluding_quotes);
Copy link
Member

Choose a reason for hiding this comment

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

The way it would work with simple type annotations is that it first parses the outer string and then the inner string on the next call:

parse_simple_type_annotation: "'int'" (10..15)
parse_simple_type_annotation: "int" (11..14)

In the case of complex annotations, it seems like the range is always going to be the one without the inner quotes:

parse_complex_type_annotation: "'int'" (10..18)
parse_complex_type_annotation: "int" (10..18)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Related to the parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ANN401 induces a stack overflow on quoted quoted escape sequences
3 participants