Skip to content

[syntax-errors] Named expressions in decorators before Python 3.9 #16386

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

Merged
ntBre merged 10 commits into from
Mar 5, 2025
Merged

[syntax-errors] Named expressions in decorators before Python 3.9 #16386

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2b4b1a2
capture current tests, with err case failing
ntBre Feb 25, 2025
c7b36ef
pass inline tests
ntBre Feb 25, 2025
7ccbc74
allow normal dotted names
ntBre Feb 25, 2025
ca5b34f
clean up test names
ntBre Mar 3, 2025
e85248c
check decorator with a pyright-style mini visitor
ntBre Mar 5, 2025
0d56c4c
document variant, delete inline comment
ntBre Mar 5, 2025
ae2afcf
move tests into version check
ntBre Mar 5, 2025
4b5a706
add exotic test case
ntBre Mar 5, 2025
886db18
Merge branch 'main' into brent/syntax-decorators-39
ntBre Mar 5, 2025
0585337
update error message
ntBre Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/ruff_python_parser/resources/inline/err/decorator_expression_py38.py
View file
Open in desktop
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(): ...
3 changes: 3 additions & 0 deletions crates/ruff_python_parser/resources/inline/ok/decorator_expression_dotted_ident_py38.py
View file
Open in desktop
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(): ...
3 changes: 3 additions & 0 deletions crates/ruff_python_parser/resources/inline/ok/decorator_expression_eval_hack_py38.py
View file
Open in desktop
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(): ...
4 changes: 4 additions & 0 deletions crates/ruff_python_parser/resources/inline/ok/decorator_expression_identity_hack_py38.py
View file
Open in desktop
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(): ...
3 changes: 3 additions & 0 deletions crates/ruff_python_parser/resources/inline/ok/decorator_expression_py39.py
View file
Open in desktop
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(): ...
3 changes: 3 additions & 0 deletions crates/ruff_python_parser/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,7 @@ pub enum UnsupportedSyntaxErrorKind {
Match,
Walrus,
ExceptStar,
RelaxedDecorator,
}

impl Display for UnsupportedSyntaxError {
Expand All @@ -457,6 +458,7 @@ impl Display for UnsupportedSyntaxError {
UnsupportedSyntaxErrorKind::Match => "`match` statement",
UnsupportedSyntaxErrorKind::Walrus => "named assignment expression (`:=`)",
UnsupportedSyntaxErrorKind::ExceptStar => "`except*`",
UnsupportedSyntaxErrorKind::RelaxedDecorator => "named expression in decorator",
Copy link
Member

@AlexWaygood AlexWaygood Mar 3, 2025

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

Copy link
Contributor Author

@ntBre ntBre Mar 3, 2025

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.

Copy link
Member

@AlexWaygood AlexWaygood Mar 3, 2025

Choose a reason for hiding this comment

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

Maybe something like this?

impl Display for UnsupportedSyntaxError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        let kind = match self.kind {
            UnsupportedSyntaxErrorKind::Match => "`match` statement",
            UnsupportedSyntaxErrorKind::Walrus => "named assignment expression (`:=`)",
            UnsupportedSyntaxErrorKind::ExceptStar => "`except*`",
            UnsupportedSyntaxErrorKind::RelaxedDecorator => {
                let expression_kind = ...;
                return write!(
                    "Cannot use {expression_kind} expressions in decorators on Python {} (syntax was added in Python 3.9)",
                    self.target_version,
               );   
            }      
        };
        write!(
            f,
            "Cannot use {kind} on Python {} (syntax was added in Python {})",
            self.target_version,
            self.kind.minimum_version(),
        )
    }
}

Then if it was e.g. a subscript expression, the error message would be

Cannot use subscript expressions in decorators on Python 3.8 (syntax was added in Python 3.9)

Copy link
Contributor Author

@ntBre ntBre Mar 5, 2025

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":

Syntax Error: Unsupported expression in decorators on Python 3.8 (syntax was added in Python 3.9)

Copy link
Member

@AlexWaygood AlexWaygood Mar 5, 2025

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!

Copy link
Contributor Author

@ntBre ntBre Mar 5, 2025

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!

};
write!(
f,
Expand All @@ -474,6 +476,7 @@ impl UnsupportedSyntaxErrorKind {
UnsupportedSyntaxErrorKind::Match => PythonVersion::PY310,
UnsupportedSyntaxErrorKind::Walrus => PythonVersion::PY38,
UnsupportedSyntaxErrorKind::ExceptStar => PythonVersion::PY311,
UnsupportedSyntaxErrorKind::RelaxedDecorator => PythonVersion::PY39,
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_python_parser/src/parser/expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ impl<'src> Parser<'src> {
/// If the parser isn't position at a `(` token.
///
/// See: <https://docs.python.org/3/reference/expressions.html#calls>
fn parse_call_expression(&mut self, func: Expr, start: TextSize) -> ast::ExprCall {
pub(super) fn parse_call_expression(&mut self, func: Expr, start: TextSize) -> ast::ExprCall {
let arguments = self.parse_arguments();

ast::ExprCall {
Expand Down
96 changes: 94 additions & 2 deletions crates/ruff_python_parser/src/parser/statement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, Stmt, WithItem,
self as ast, ExceptHandler, Expr, ExprContext, IpyEscapeKind, Operator, PythonVersion, Stmt,
WithItem,
};
use ruff_text_size::{Ranged, TextSize};

Expand Down Expand Up @@ -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>
Expand All @@ -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(): ...
// @
Expand All @@ -2554,14 +2626,34 @@ impl<'src> Parser<'src> {
let decorator_start = self.node_start();
self.bump(TokenKind::At);

let checkpoint = self.checkpoint();
Copy link
Member

@MichaReiser MichaReiser Mar 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@dhruvmanila dhruvmanila Mar 4, 2025

Choose a reason for hiding this comment

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

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 @foo[0] (top-level expression is subscript) or @foo[0].bar (top-level expression is an attribute) as both are not allowed.

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.

Copy link
Contributor Author

@ntBre ntBre Mar 4, 2025

Choose a reason for hiding this comment

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

It looks like pyright's _isNameOrMemberAccessExpression function called there is recursively checking the decorator expression. I guess that is a minimal visitor. When Alex mentioned the greeter approach, I was picturing holding off on this error until I start working on the separate greeter for compile-time/semantic errors. Should I just have a mini visitor directly in the parser like pyright instead?

Copy link
Member

@MichaReiser MichaReiser Mar 4, 2025

Choose a reason for hiding this comment

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

I don't mind a mini visitor in the parser, as long as it isn't a full Visitor based visitor (and only runs for < 3.9)

Copy link
Contributor Author

@ntBre ntBre Mar 4, 2025

Choose a reason for hiding this comment

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

Of course, I think I can pretty much follow pyright directly here.

Copy link
Member

@dhruvmanila dhruvmanila Mar 4, 2025

Choose a reason for hiding this comment

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

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(): ...
let parsed_expr = self.parse_named_expression_or_higher(ExpressionContext::default());

decorators.push(ast::Decorator {
expression: parsed_expr.expr,
Expand Down
101 changes: 101 additions & 0 deletions crates/ruff_python_parser/tests/snapshots/invalid_syntax@decorator_expression_py38.py.snap
View file
Open in desktop
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(): ...
|
79 changes: 79 additions & 0 deletions ...python_parser/tests/snapshots/valid_syntax@decorator_expression_dotted_ident_py38.py.snap
View file
Open in desktop
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,
},
),
},
),
],
},
),
],
},
)
```
Loading
Loading