Skip to content

Commit

Permalink
Fix WithItem ranges for parenthesized, non-as items (#6782)
Browse files Browse the repository at this point in the history
## Summary

This PR attempts to address a problem in the parser related to the
range's of `WithItem` nodes in certain contexts -- specifically,
`WithItem` nodes in parentheses that do not have an `as` token after
them.

For example,
[here](https://play.ruff.rs/71be2d0b-2a04-4c7e-9082-e72bff152679):

```python
with (a, b):
    pass
```

The range of the `WithItem` `a` is set to the range of `(a, b)`, as is
the range of the `WithItem` `b`. In other words, when we have this kind
of sequence, we use the range of the entire parenthesized context,
rather than the ranges of the items themselves.

Note that this also applies to cases
[like](https://play.ruff.rs/c551e8e9-c3db-4b74-8cc6-7c4e3bf3713a):

```python
with (a, b, c as d):
    pass
```

You can see the issue in the parser here:

```rust
#[inline]
WithItemsNoAs: Vec<ast::WithItem> = {
    <location:@l> <all:OneOrMore<Test<"all">>> <end_location:@r> => {
        all.into_iter().map(|context_expr| ast::WithItem { context_expr, optional_vars: None, range: (location..end_location).into() }).collect()
    },
}
```

Fixing this issue is... very tricky. The naive approach is to use the
range of the `context_expr` as the range for the `WithItem`, but that
range will be incorrect when the `context_expr` is itself parenthesized.
For example, _that_ solution would fail here, since the range of the
first `WithItem` would be that of `a`, rather than `(a)`:

```python
with ((a), b):
    pass
```

The `with` parsing in general is highly precarious due to ambiguities in
the grammar. Changing it in _any_ way seems to lead to an ambiguous
grammar that LALRPOP fails to translate. Consensus seems to be that we
don't really understand _why_ the current grammar works (i.e., _how_ it
avoids these ambiguities as-is).

The solution implemented here is to avoid changing the grammar itself,
and instead change the shape of the nodes returned by various rules in
the grammar. Specifically, everywhere that we return `Expr`, we instead
return `ParenthesizedExpr`, which includes a parenthesized range and the
underlying `Expr` itself. (If an `Expr` isn't parenthesized, the ranges
will be equivalent.) In `WithItemsNoAs`, we can then use the
parenthesized range as the range for the `WithItem`.
  • Loading branch information
charliermarsh authored Aug 31, 2023
1 parent 96a9717 commit 68f605e
Show file tree
Hide file tree
Showing 6 changed files with 7,721 additions and 6,744 deletions.
173 changes: 169 additions & 4 deletions crates/ruff_python_ast/src/nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2716,7 +2716,7 @@ impl Ranged for crate::nodes::StmtContinue {
self.range
}
}
impl Ranged for StmtIpyEscapeCommand {
impl Ranged for crate::nodes::StmtIpyEscapeCommand {
fn range(&self) -> TextRange {
self.range
}
Expand Down Expand Up @@ -2888,7 +2888,7 @@ impl Ranged for crate::nodes::ExprSlice {
self.range
}
}
impl Ranged for ExprIpyEscapeCommand {
impl Ranged for crate::nodes::ExprIpyEscapeCommand {
fn range(&self) -> TextRange {
self.range
}
Expand Down Expand Up @@ -2927,7 +2927,6 @@ impl Ranged for crate::Expr {
}
}
}

impl Ranged for crate::nodes::Comprehension {
fn range(&self) -> TextRange {
self.range
Expand All @@ -2945,7 +2944,6 @@ impl Ranged for crate::ExceptHandler {
}
}
}

impl Ranged for crate::nodes::Parameter {
fn range(&self) -> TextRange {
self.range
Expand Down Expand Up @@ -3086,6 +3084,173 @@ impl Ranged for crate::nodes::ParameterWithDefault {
}
}

/// An expression that may be parenthesized.
#[derive(Clone, Debug)]
pub struct ParenthesizedExpr {
/// The range of the expression, including any parentheses.
pub range: TextRange,
/// The underlying expression.
pub expr: Expr,
}
impl Ranged for ParenthesizedExpr {
fn range(&self) -> TextRange {
self.range
}
}
impl From<Expr> for ParenthesizedExpr {
fn from(expr: Expr) -> Self {
ParenthesizedExpr {
range: expr.range(),
expr,
}
}
}
impl From<ParenthesizedExpr> for Expr {
fn from(parenthesized_expr: ParenthesizedExpr) -> Self {
parenthesized_expr.expr
}
}
impl From<ExprIpyEscapeCommand> for ParenthesizedExpr {
fn from(payload: ExprIpyEscapeCommand) -> Self {
Expr::IpyEscapeCommand(payload).into()
}
}
impl From<ExprBoolOp> for ParenthesizedExpr {
fn from(payload: ExprBoolOp) -> Self {
Expr::BoolOp(payload).into()
}
}
impl From<ExprNamedExpr> for ParenthesizedExpr {
fn from(payload: ExprNamedExpr) -> Self {
Expr::NamedExpr(payload).into()
}
}
impl From<ExprBinOp> for ParenthesizedExpr {
fn from(payload: ExprBinOp) -> Self {
Expr::BinOp(payload).into()
}
}
impl From<ExprUnaryOp> for ParenthesizedExpr {
fn from(payload: ExprUnaryOp) -> Self {
Expr::UnaryOp(payload).into()
}
}
impl From<ExprLambda> for ParenthesizedExpr {
fn from(payload: ExprLambda) -> Self {
Expr::Lambda(payload).into()
}
}
impl From<ExprIfExp> for ParenthesizedExpr {
fn from(payload: ExprIfExp) -> Self {
Expr::IfExp(payload).into()
}
}
impl From<ExprDict> for ParenthesizedExpr {
fn from(payload: ExprDict) -> Self {
Expr::Dict(payload).into()
}
}
impl From<ExprSet> for ParenthesizedExpr {
fn from(payload: ExprSet) -> Self {
Expr::Set(payload).into()
}
}
impl From<ExprListComp> for ParenthesizedExpr {
fn from(payload: ExprListComp) -> Self {
Expr::ListComp(payload).into()
}
}
impl From<ExprSetComp> for ParenthesizedExpr {
fn from(payload: ExprSetComp) -> Self {
Expr::SetComp(payload).into()
}
}
impl From<ExprDictComp> for ParenthesizedExpr {
fn from(payload: ExprDictComp) -> Self {
Expr::DictComp(payload).into()
}
}
impl From<ExprGeneratorExp> for ParenthesizedExpr {
fn from(payload: ExprGeneratorExp) -> Self {
Expr::GeneratorExp(payload).into()
}
}
impl From<ExprAwait> for ParenthesizedExpr {
fn from(payload: ExprAwait) -> Self {
Expr::Await(payload).into()
}
}
impl From<ExprYield> for ParenthesizedExpr {
fn from(payload: ExprYield) -> Self {
Expr::Yield(payload).into()
}
}
impl From<ExprYieldFrom> for ParenthesizedExpr {
fn from(payload: ExprYieldFrom) -> Self {
Expr::YieldFrom(payload).into()
}
}
impl From<ExprCompare> for ParenthesizedExpr {
fn from(payload: ExprCompare) -> Self {
Expr::Compare(payload).into()
}
}
impl From<ExprCall> for ParenthesizedExpr {
fn from(payload: ExprCall) -> Self {
Expr::Call(payload).into()
}
}
impl From<ExprFormattedValue> for ParenthesizedExpr {
fn from(payload: ExprFormattedValue) -> Self {
Expr::FormattedValue(payload).into()
}
}
impl From<ExprFString> for ParenthesizedExpr {
fn from(payload: ExprFString) -> Self {
Expr::FString(payload).into()
}
}
impl From<ExprConstant> for ParenthesizedExpr {
fn from(payload: ExprConstant) -> Self {
Expr::Constant(payload).into()
}
}
impl From<ExprAttribute> for ParenthesizedExpr {
fn from(payload: ExprAttribute) -> Self {
Expr::Attribute(payload).into()
}
}
impl From<ExprSubscript> for ParenthesizedExpr {
fn from(payload: ExprSubscript) -> Self {
Expr::Subscript(payload).into()
}
}
impl From<ExprStarred> for ParenthesizedExpr {
fn from(payload: ExprStarred) -> Self {
Expr::Starred(payload).into()
}
}
impl From<ExprName> for ParenthesizedExpr {
fn from(payload: ExprName) -> Self {
Expr::Name(payload).into()
}
}
impl From<ExprList> for ParenthesizedExpr {
fn from(payload: ExprList) -> Self {
Expr::List(payload).into()
}
}
impl From<ExprTuple> for ParenthesizedExpr {
fn from(payload: ExprTuple) -> Self {
Expr::Tuple(payload).into()
}
}
impl From<ExprSlice> for ParenthesizedExpr {
fn from(payload: ExprSlice) -> Self {
Expr::Slice(payload).into()
}
}

#[cfg(target_pointer_width = "64")]
mod size_assertions {
use static_assertions::assert_eq_size;
Expand Down
24 changes: 24 additions & 0 deletions crates/ruff_python_parser/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,30 @@ with (0 as a, 1 as b,): pass
insta::assert_debug_snapshot!(parse_suite(source, "<test>").unwrap());
}

#[test]
fn test_parenthesized_with_statement() {
let source = "\
with ((a), (b)): pass
with ((a), (b), c as d, (e)): pass
with (a, b): pass
with (a, b) as c: pass
with ((a, b) as c): pass
with (a as b): pass
with (a): pass
with (a := 0): pass
with (a := 0) as x: pass
with ((a)): pass
with ((a := 0)): pass
with (a as b, (a := 0)): pass
with (a, (a := 0)): pass
with (yield): pass
with (yield from a): pass
with ((yield)): pass
with ((yield from a)): pass
";
insta::assert_debug_snapshot!(parse_suite(source, "<test>").unwrap());
}

#[test]
fn test_with_statement_invalid() {
for source in [
Expand Down
Loading

0 comments on commit 68f605e

Please sign in to comment.