Skip to content

Commit

Permalink
Fix nested validation of expressions
Browse files Browse the repository at this point in the history
Summary:
We have a validation pass that looks for things like `lambda a,a:a` and turns it into an error. Previously this looked for the top-level expression directly inside a statement. That means it missed things like `lambda: lambda a,a:a`. Easily fixed by just recursing in the visit_expr call. Our validation is assumed to have happened by the compiler, so that code would lead to:

```
assertion failed: old_local.is_none()
```

Caught by the OSS Fuzzer that we have set up.

Reviewed By: IanChilds

Differential Revision: D64663606

fbshipit-source-id: 13c71abb4609be75f8879dfdf9eb2706c53d6dd1
  • Loading branch information
ndmitchell authored and facebook-github-bot committed Oct 21, 2024
1 parent ea40b70 commit 81cfbe7
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 4 deletions.
7 changes: 7 additions & 0 deletions starlark/src/tests/def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,13 @@ fn test_lambda_errors() {
assert::fail("lambda a,a:a", "duplicated parameter name");
}

#[test]
fn test_lambda_errors_nested() {
// Test from https://issues.oss-fuzz.com/issues/369003809
assert::fail("lambda: lambda a,a:a", "duplicated parameter name");
assert::fail("[lambda a,a:a]", "duplicated parameter name");
}

#[test]
fn test_double_capture_and_freeze() {
let mut a = Assert::new();
Expand Down
9 changes: 5 additions & 4 deletions starlark_syntax/src/syntax/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,21 +149,22 @@ pub(crate) fn validate_module(stmt: &AstStmt, parser_state: &mut ParserState) {
}
}

fn expr(expr: &AstExpr, parser_state: &mut ParserState) {
match &expr.node {
fn expr(x: &AstExpr, parser_state: &mut ParserState) {
match &x.node {
Expr::Literal(AstLiteral::Ellipsis) => {
if parser_state.dialect.enable_types == DialectTypes::Disable {
parser_state.error(expr.span, "`...` is not allowed in this dialect");
parser_state.error(x.span, "`...` is not allowed in this dialect");
}
}
Expr::Lambda(LambdaP { params, .. }) => {
if !parser_state.dialect.enable_lambda {
parser_state.error(expr.span, "`lambda` is not allowed in this dialect");
parser_state.error(x.span, "`lambda` is not allowed in this dialect");
}
validate_params(params, parser_state);
}
_ => {}
}
x.node.visit_expr(|x| expr(x, parser_state));
}

f(stmt, parser_state, true, false, false);
Expand Down

0 comments on commit 81cfbe7

Please sign in to comment.