[syntax-errors] Named expressions in decorators before Python 3.9 #16386
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # parse_options: { "target-version": "3.8" } | ||
| @buttons[0].clicked.connect | ||
| def spam(): ... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # parse_options: { "target-version": "3.8" } | ||
| @buttons.clicked.connect | ||
| def spam(): ... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # parse_options: { "target-version": "3.8" } | ||
| @eval("buttons[0].clicked.connect") | ||
| def spam(): ... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| # parse_options: { "target-version": "3.8" } | ||
| def _(x): return x | ||
| @_(buttons[0].clicked.connect) | ||
| def spam(): ... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # parse_options: { "target-version": "3.9" } | ||
| @buttons[0].clicked.connect | ||
| def spam(): ... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,8 @@ use rustc_hash::{FxBuildHasher, FxHashSet}; | |
|
|
||
| use ruff_python_ast::name::Name; | ||
| use ruff_python_ast::{ | ||
| self as ast, ExceptHandler, Expr, ExprContext, IpyEscapeKind, Operator, PythonVersion, Stmt, | ||
| WithItem, | ||
| }; | ||
| use ruff_text_size::{Ranged, TextSize}; | ||
|
|
||
|
|
@@ -2533,6 +2534,45 @@ impl<'src> Parser<'src> { | |
| } | ||
| } | ||
|
|
||
| /// Try to parse a decorator list from before Python 3.9. | ||
| /// | ||
| /// See: <https://docs.python.org/3.8/reference/compound_stmts.html#grammar-token-decorators> | ||
| fn try_parse_old_decorators(&mut self) -> Option<ParsedExpr> { | ||
| let errors = self.errors.len(); | ||
| let start = self.node_start(); | ||
| // initial identifier | ||
| let ident = self.parse_identifier(); | ||
| if !ident.is_valid() { | ||
| return None; | ||
| } | ||
| let mut name = Expr::from(ast::ExprName { | ||
| range: self.node_range(start), | ||
| id: ident.id, | ||
| ctx: ExprContext::Load, | ||
| }); | ||
| // ("." identifier)* | ||
| while self.at(TokenKind::Dot) { | ||
| let attr = self.parse_attribute_expression(name, start); | ||
| if !attr.attr.is_valid() { | ||
| return None; | ||
| } | ||
| name = Expr::from(attr); | ||
| } | ||
| // ["(" [argument_list [","]] ")"] NEWLINE | ||
| let parsed = match self.current_token_kind() { | ||
| TokenKind::Lpar => Some(Expr::Call(self.parse_call_expression(name, start)).into()), | ||
| TokenKind::Newline => Some(name.into()), | ||
| _ => None, | ||
| }; | ||
|
|
||
| // ignore version-related errors if there are more serious errors when parsing this way | ||
| if self.errors.len() > errors { | ||
| return None; | ||
| } | ||
|
|
||
| parsed | ||
| } | ||
|
|
||
| /// Parses a decorator list followed by a class, function or async function definition. | ||
| /// | ||
| /// See: <https://docs.python.org/3/reference/compound_stmts.html#grammar-token-python-grammar-decorators> | ||
|
|
@@ -2542,6 +2582,38 @@ impl<'src> Parser<'src> { | |
| let mut decorators = vec![]; | ||
| let mut progress = ParserProgress::default(); | ||
|
|
||
| // these examples are adapted from [PEP 614](https://peps.python.org/pep-0614/), which | ||
| // relaxed the restriction that "decorators consist of a dotted name, optionally followed by | ||
| // a single call" in Python 3.9. we want to catch decorators that don't meet these criteria | ||
| // before 3.9 but avoid false positives on examples like the `@_` "identity function hack" | ||
| // or the "eval hack" called out in the PEP. | ||
|
|
||
| // test_ok decorator_expression_dotted_ident_py38 | ||
| // # parse_options: { "target-version": "3.8" } | ||
| // @buttons.clicked.connect | ||
| // def spam(): ... | ||
|
|
||
| // test_ok decorator_expression_identity_hack_py38 | ||
| // # parse_options: { "target-version": "3.8" } | ||
| // def _(x): return x | ||
| // @_(buttons[0].clicked.connect) | ||
| // def spam(): ... | ||
|
|
||
| // test_ok decorator_expression_eval_hack_py38 | ||
| // # parse_options: { "target-version": "3.8" } | ||
| // @eval("buttons[0].clicked.connect") | ||
| // def spam(): ... | ||
|
|
||
| // test_ok decorator_expression_py39 | ||
| // # parse_options: { "target-version": "3.9" } | ||
| // @buttons[0].clicked.connect | ||
| // def spam(): ... | ||
|
|
||
| // test_err decorator_expression_py38 | ||
| // # parse_options: { "target-version": "3.8" } | ||
| // @buttons[0].clicked.connect | ||
| // def spam(): ... | ||
|
|
||
| // test_err decorator_missing_expression | ||
| // @def foo(): ... | ||
| // @ | ||
|
|
@@ -2554,14 +2626,34 @@ impl<'src> Parser<'src> { | |
| let decorator_start = self.node_start(); | ||
| self.bump(TokenKind::At); | ||
|
|
||
| let checkpoint = self.checkpoint(); | ||
|
There was a problem hiding this comment. I'd prefer not to use checkpoints here. Checkpoints are expensive. I think I'd prefer to visit the parsed AST instead and check if any name isn't a named expression. One thing that might be challenging is that the outer-most expression can't be parenthesized, but that's something that we should be able to detect at the parse decorator function There was a problem hiding this comment. I don't think it's just named expression. As per the grammar and the tests show this as well, one other example is a subscript expression like I agree that it might be simplest to do this using a visitor instead of using speculative parsing. I quickly looked at what Pyright does and it seems to be doing a basic check on the parsed expression if the target version is < 3.9: https://github.com/microsoft/pyright/blob/1d1b43c17a83927d422c9ff52f98fb595419e329/packages/pyright-internal/src/parser/parser.ts#L2401-L2417. I'm not sure if that covers the entire extent of the grammar but it might be worth exploring. There was a problem hiding this comment. It looks like pyright's There was a problem hiding this comment. I don't mind a mini visitor in the parser, as long as it isn't a full There was a problem hiding this comment. Of course, I think I can pretty much follow pyright directly here. There was a problem hiding this comment. Thanks for looking into it. I also think that's fine. |
||
| let parsed_expr = if self.options.target_version < PythonVersion::PY39 { | ||
| match self.try_parse_old_decorators() { | ||
| Some(parsed) => parsed, | ||
| None => { | ||
| self.rewind(checkpoint); | ||
| let parsed = | ||
| self.parse_named_expression_or_higher(ExpressionContext::default()); | ||
|
|
||
| self.add_unsupported_syntax_error( | ||
| UnsupportedSyntaxErrorKind::RelaxedDecorator, | ||
| self.node_range(decorator_start), | ||
| ); | ||
|
|
||
| parsed | ||
| } | ||
| } | ||
| } else { | ||
| self.parse_named_expression_or_higher(ExpressionContext::default()) | ||
| }; | ||
|
|
||
| // test_err decorator_invalid_expression | ||
| // @*x | ||
| // @(*x) | ||
| // @((*x)) | ||
| // @yield x | ||
| // @yield from x | ||
| // def foo(): ... | ||
|
|
||
| decorators.push(ast::Decorator { | ||
| expression: parsed_expr.expr, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| --- | ||
| source: crates/ruff_python_parser/tests/fixtures.rs | ||
| input_file: crates/ruff_python_parser/resources/inline/err/decorator_expression_py38.py | ||
| --- | ||
| ## AST | ||
|
|
||
| ``` | ||
| Module( | ||
| ModModule { | ||
| range: 0..89, | ||
| body: [ | ||
| FunctionDef( | ||
| StmtFunctionDef { | ||
| range: 45..88, | ||
| is_async: false, | ||
| decorator_list: [ | ||
| Decorator { | ||
| range: 45..72, | ||
| expression: Attribute( | ||
| ExprAttribute { | ||
| range: 46..72, | ||
| value: Attribute( | ||
| ExprAttribute { | ||
| range: 46..64, | ||
| value: Subscript( | ||
| ExprSubscript { | ||
| range: 46..56, | ||
| value: Name( | ||
| ExprName { | ||
| range: 46..53, | ||
| id: Name("buttons"), | ||
| ctx: Load, | ||
| }, | ||
| ), | ||
| slice: NumberLiteral( | ||
| ExprNumberLiteral { | ||
| range: 54..55, | ||
| value: Int( | ||
| 0, | ||
| ), | ||
| }, | ||
| ), | ||
| ctx: Load, | ||
| }, | ||
| ), | ||
| attr: Identifier { | ||
| id: Name("clicked"), | ||
| range: 57..64, | ||
| }, | ||
| ctx: Load, | ||
| }, | ||
| ), | ||
| attr: Identifier { | ||
| id: Name("connect"), | ||
| range: 65..72, | ||
| }, | ||
| ctx: Load, | ||
| }, | ||
| ), | ||
| }, | ||
| ], | ||
| name: Identifier { | ||
| id: Name("spam"), | ||
| range: 77..81, | ||
| }, | ||
| type_params: None, | ||
| parameters: Parameters { | ||
| range: 81..83, | ||
| posonlyargs: [], | ||
| args: [], | ||
| vararg: None, | ||
| kwonlyargs: [], | ||
| kwarg: None, | ||
| }, | ||
| returns: None, | ||
| body: [ | ||
| Expr( | ||
| StmtExpr { | ||
| range: 85..88, | ||
| value: EllipsisLiteral( | ||
| ExprEllipsisLiteral { | ||
| range: 85..88, | ||
| }, | ||
| ), | ||
| }, | ||
| ), | ||
| ], | ||
| }, | ||
| ), | ||
| ], | ||
| }, | ||
| ) | ||
| ``` | ||
| ## Unsupported Syntax Errors | ||
|
|
||
| | | ||
| 1 | # parse_options: { "target-version": "3.8" } | ||
| 2 | @buttons[0].clicked.connect | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Syntax Error: Cannot use named expression in decorator on Python 3.8 (syntax was added in Python 3.9) | ||
| 3 | def spam(): ... | ||
| | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| --- | ||
| source: crates/ruff_python_parser/tests/fixtures.rs | ||
| input_file: crates/ruff_python_parser/resources/inline/ok/decorator_expression_dotted_ident_py38.py | ||
| --- | ||
| ## AST | ||
|
|
||
| ``` | ||
| Module( | ||
| ModModule { | ||
| range: 0..86, | ||
| body: [ | ||
| FunctionDef( | ||
| StmtFunctionDef { | ||
| range: 45..85, | ||
| is_async: false, | ||
| decorator_list: [ | ||
| Decorator { | ||
| range: 45..69, | ||
| expression: Attribute( | ||
| ExprAttribute { | ||
| range: 46..69, | ||
| value: Attribute( | ||
| ExprAttribute { | ||
| range: 46..61, | ||
| value: Name( | ||
| ExprName { | ||
| range: 46..53, | ||
| id: Name("buttons"), | ||
| ctx: Load, | ||
| }, | ||
| ), | ||
| attr: Identifier { | ||
| id: Name("clicked"), | ||
| range: 54..61, | ||
| }, | ||
| ctx: Load, | ||
| }, | ||
| ), | ||
| attr: Identifier { | ||
| id: Name("connect"), | ||
| range: 62..69, | ||
| }, | ||
| ctx: Load, | ||
| }, | ||
| ), | ||
| }, | ||
| ], | ||
| name: Identifier { | ||
| id: Name("spam"), | ||
| range: 74..78, | ||
| }, | ||
| type_params: None, | ||
| parameters: Parameters { | ||
| range: 78..80, | ||
| posonlyargs: [], | ||
| args: [], | ||
| vararg: None, | ||
| kwonlyargs: [], | ||
| kwarg: None, | ||
| }, | ||
| returns: None, | ||
| body: [ | ||
| Expr( | ||
| StmtExpr { | ||
| range: 82..85, | ||
| value: EllipsisLiteral( | ||
| ExprEllipsisLiteral { | ||
| range: 82..85, | ||
| }, | ||
| ), | ||
| }, | ||
| ), | ||
| ], | ||
| }, | ||
| ), | ||
| ], | ||
| }, | ||
| ) | ||
| ``` |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think "named expression" generally refers specifically to walrus expressions (https://peps.python.org/pep-0572 uses the term several times), so I'm not sure it's the best term to use here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense, I was just following PEP 614 and the AST reference (which I see now actually calls them
assignment_expressions, but I think that's another synonym for the walrus operator?).Do you have any ideas for a replacement? I didn't think "relaxed decorator" was much more helpful, but that's closer to the PEP name and the "What's new" entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this?
Then if it was e.g. a subscript expression, the error message would be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll leave this nicer error reporting for a possible follow-up. It would complicate the code a fair amount to track the expression kind through the whole process just for the error message. Micha also pointed out on Discord that 3.9 is the oldest version receiving security updates, so this message will probably have a fairly limited audience.
I did remove the confusing "named expression" part and went for something closer to pyright's "Expression form not supported for decorator prior to Python 3.9":
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks. I still think there's room for improvement but this is definitely good enough for now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, I meant to say I think your suggestion would make for the ideal error message. Thanks!