Skip to content

Commit

Permalink
Reject more syntactically invalid Python programs
Browse files Browse the repository at this point in the history
This commit adds some additional error checking to the parser such
that assignments that are invalid syntax are rejected. This covers the
obvious cases like `5 = 3` and some not so obvious cases like `x + y =
42`.

This does add an additional recursive call to the parser for the cases
handling assignments. I had initially been concerned about doing this,
but `set_context` is already doing recursion during assignments, so I
didn't feel as though this was changing any fundamental performance
characteristics of the parser. (Also, in practice, I would expect any
such recursion here to be quite shallow since the recursion is done
on the target of an assignment. Such things are rarely nested much in
practice.)

Fixes #6895
  • Loading branch information
BurntSushi committed Nov 6, 2023
1 parent 218f517 commit bd787b4
Show file tree
Hide file tree
Showing 16 changed files with 1,498 additions and 140 deletions.
808 changes: 808 additions & 0 deletions crates/ruff_python_parser/src/invalid.rs

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions crates/ruff_python_parser/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ use crate::lexer::LexResult;
mod function;
// Skip flattening lexer to distinguish from full ruff_python_parser
mod context;
mod invalid;
pub mod lexer;
mod parser;
mod soft_keywords;
Expand Down
27 changes: 15 additions & 12 deletions crates/ruff_python_parser/src/python.lalrpop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::{
context::set_context,
string::{StringType, concatenate_strings, parse_fstring_middle, parse_string_literal},
token::{self, StringKind},
invalid,
};
use lalrpop_util::ParseError;

Expand Down Expand Up @@ -108,12 +109,12 @@ DelStatement: ast::Stmt = {
};

ExpressionStatement: ast::Stmt = {
<location:@L> <expression:TestOrStarExprList> <suffix:AssignSuffix*> <end_location:@R> => {
<location:@L> <expression:TestOrStarExprList> <suffix:AssignSuffix*> <end_location:@R> =>? {
// Just an expression, no assignment:
if suffix.is_empty() {
ast::Stmt::Expr(
Ok(ast::Stmt::Expr(
ast::StmtExpr { value: Box::new(expression.into()), range: (location..end_location).into() }
)
))
} else {
let mut targets = vec![set_context(expression.into(), ast::ExprContext::Store)];
let mut values = suffix;
Expand All @@ -123,33 +124,35 @@ ExpressionStatement: ast::Stmt = {
for target in values {
targets.push(set_context(target.into(), ast::ExprContext::Store));
}

ast::Stmt::Assign(
invalid::assignment_targets(&targets)?;
Ok(ast::Stmt::Assign(
ast::StmtAssign { targets, value, range: (location..end_location).into() }
)
))
}
},
<location:@L> <target:TestOrStarExprList> <op:AugAssign> <rhs:TestListOrYieldExpr> <end_location:@R> => {
ast::Stmt::AugAssign(
<location:@L> <target:TestOrStarExprList> <op:AugAssign> <rhs:TestListOrYieldExpr> <end_location:@R> =>? {
invalid::assignment_target(&target.expr)?;
Ok(ast::Stmt::AugAssign(
ast::StmtAugAssign {
target: Box::new(set_context(target.into(), ast::ExprContext::Store)),
op,
value: Box::new(rhs.into()),
range: (location..end_location).into()
},
)
))
},
<location:@L> <target:Test<"all">> ":" <annotation:Test<"all">> <rhs:AssignSuffix?> <end_location:@R> => {
<location:@L> <target:Test<"all">> ":" <annotation:Test<"all">> <rhs:AssignSuffix?> <end_location:@R> =>? {
let simple = target.expr.is_name_expr();
ast::Stmt::AnnAssign(
invalid::assignment_target(&target.expr)?;
Ok(ast::Stmt::AnnAssign(
ast::StmtAnnAssign {
target: Box::new(set_context(target.into(), ast::ExprContext::Store)),
annotation: Box::new(annotation.into()),
value: rhs.map(ast::Expr::from).map(Box::new),
simple,
range: (location..end_location).into()
},
)
))
},
};

Expand Down
Loading

0 comments on commit bd787b4

Please sign in to comment.