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.

Sign up for GitHub

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

Jump to bottom

Quoted annotations should be parsed as if parenthesized #9467

Open
charliermarsh opened this issue Jan 11, 2024 · 6 comments · May be fixed by #15387
Open

Quoted annotations should be parsed as if parenthesized #9467

charliermarsh opened this issue Jan 11, 2024 · 6 comments · May be fixed by #15387
Labels
bug Something isn't working help wanted Contributions especially welcome parser Related to the parser

Comments

@charliermarsh
Copy link
Member

It was recently agreed that quoted annotations should be parsed as if they were implicitly wrapped in parentheses. So, e.g., they can contain otherwise-invalid indentation, newlines within binary operators, etc.

See: microsoft/pyright#6940 (comment)
@charliermarsh charliermarsh added the parser Related to the parser label Jan 11, 2024
@charliermarsh charliermarsh added the bug Something isn't working label Jan 29, 2024
@Glyphack
Copy link
Contributor

Glyphack commented Feb 1, 2024 •
edited
Loading

Should this be implemented in the parser layer or here where we emit the warning?

@dhruvmanila
Copy link
Member

I think it should be implemented in the parse_type_annotation function which is used by the AST checker to report an error if found:

ruff/crates/ruff_python_parser/src/typing.rs

Lines 22 to 44 in a0263ab

/// Parse a type annotation from a string.
pub fn parse_type_annotation(
value: &str,
range: TextRange,
source: &str,
) -> Result<(Expr, AnnotationKind)> {
let expression = &source[range];
if str::raw_contents(expression).is_some_and(|body| body == value) {
// The annotation is considered "simple" if and only if the raw representation (e.g.,
// `List[int]` within "List[int]") exactly matches the parsed representation. This
// isn't the case, e.g., for implicit concatenations, or for annotations that contain
// escaped quotes.
let leading_quote = str::leading_quote(expression).unwrap();
let expr = parse_expression_starts_at(value, range.start() + leading_quote.text_len())?;
Ok((expr, AnnotationKind::Simple))
} else {
// Otherwise, consider this a "complex" annotation.
let mut expr = parse_expression(value)?;
relocate_expr(&mut expr, range);
Ok((expr, AnnotationKind::Complex))
}
}

I would also suggest to look at other references of parse_type_annotation function to make sure they aren't affected in any way with this change. They shouldn't be but just to make sure :)

@Glyphack
Copy link
Contributor

Nice, I can work on this.

@Glyphack Glyphack mentioned this issue Apr 2, 2024
Closed
@MichaReiser
Copy link
Member

An easy fix here would probably be adding new parse_parenthesized_expression methods or to extend parse_expression with an argument whether it is parenthesized and then funneling that information to the lexer.

@carljm
Copy link
Contributor

carljm commented Jan 8, 2025

At this point, we'll want the fix here to apply to both ruff and red-knot.

@MichaReiser MichaReiser added the help wanted Contributions especially welcome label Jan 8, 2025
@dhruvmanila
Copy link
Member

At this point, we'll want the fix here to apply to both ruff and red-knot.

Agreed. Both ruff and red knot utilizes the parse_expression_range function from the parser:

ruff/crates/ruff_python_parser/src/lib.rs

Lines 157 to 167 in 21aa12a

pub fn parse_expression_range(
source: &str,
range: TextRange,
) -> Result<Parsed<ModExpression>, ParseError> {
let source = &source[..range.end().to_usize()];
Parser::new_starts_at(source, Mode::Expression, range.start())
.parse()
.try_into_expression()
.unwrap()
.into_result()
}

So, as Micha suggested, we could add a parse_parenthesized_expression_range function. I'd suggest adding a new Mode::ParenthesizedExpression and initiating the Lexer with a nesting value of 1 which I think should solve this. It might require some testing.

@Glyphack Glyphack linked a pull request Jan 9, 2025 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working help wanted Contributions especially welcome parser Related to the parser
Projects
None yet
Milestone
No milestone
5 participants
@carljm @MichaReiser @charliermarsh @Glyphack @dhruvmanila