bug: argument lists accept empty slots and build a zero-width ARG node #2

New issue

Closed

opened 2026-09-21 15:26:38 +00:00 by barrettruth · 1 comment

barrettruth commented

2026-09-21 15:26:38 +00:00

Owner

Original issue: source #4
Original author: barrettruth
Original date: 2026-08-26T12:01:07Z

Version

0.1.1

Dialect

Dialect::Bazel, reproduced on BUILD and .bzl. Not dialect-specific.

Which guarantee broke

Wrong tree shape, plus a missing error. Round-trip holds and nothing panics.

Observed

An empty slot in an argument list is accepted silently and produces a
zero-width ARG node:

$ parse("foo(a, , b)\n", Dialect::Bazel)
errors: 0

FILE@0..12
  EXPR_STMT@0..11
    CALL_EXPR@0..11
      IDENT_EXPR@0..3
        IDENT@0..3 "foo"
      ARG_LIST@3..11
        L_PAREN@3..4 "("
        ARG@4..5
          IDENT_EXPR@4..5
            IDENT@4..5 "a"
        COMMA@5..6 ","
        WHITESPACE@6..7 " "
        ARG@7..7                <-- empty
        COMMA@7..8 ","
        WHITESPACE@8..9 " "
        ARG@9..10
          IDENT_EXPR@9..10
            IDENT@9..10 "b"
        R_PAREN@10..11 ")"

foo(, a) and foo(,) are likewise accepted with zero errors.

Why this is a bug and not a tolerated recovery

Lists and dicts already reject the same construct, so this is an argument-list
oversight rather than a deliberate policy:

input	errors
`x = [1, , 2]`	3
`x = {1: 2, , 3: 4}`	4
`foo(a, , b)`	0
`foo(, a)`	0
`foo(,)`	0

A zero-width ARG is also not a well-formed node. Every other ARG in the
tree spans its expression; consumers that walk ARG_LIST — CallExpr::arg,
anything counting positional arguments, anything mapping a cursor offset to an
argument — get a node with no content and no position to report. In a BUILD
file that is cc_library(name = "a", , srcs = [...]) silently presenting an
extra argument.

Precedent

Both reference implementations reject it, at the comma:

# Bazel, Parser.java: syntax error at ',': expected expression

# starlark-rust 0.14.2
error: Parse error: unexpected symbol ',', expected expression
 --> t:1:8
  |
1 | foo(a, , b)
  |        ^
  |

error: Parse error: unexpected symbol ',', expected expression
 --> t:1:5
  |
1 | foo(, a)
  |     ^
  |

Note the anchor is the offending comma, and it is one character wide.

Bazel's own grammar permits a trailing comma before ) and nothing else;
Parser.java carries Trailing comma is allowed only in parenthesized tuples.
for the adjacent case, so the distinction between trailing and empty slot
is one it draws explicitly.

Proposed behaviour

Reject an empty argument slot with the same message the parser already uses
elsewhere for a missing operand, anchored on the comma that opens the empty
slot and one character wide:

input	error	range
`foo(a, , b)`	`expected an expression`	the second `,`, `7..8`
`foo(, a)`	`expected an expression`	the `,`, `4..5`
`foo(,)`	`expected an expression`	the `,`, `4..5`

foo(a,) must stay accepted — a single trailing comma before ) is legal.

The zero-width ARG node should not be produced. Either omit the node and let
the COMMA tokens sit directly in ARG_LIST, or wrap the comma in an ERROR
node, whichever matches how the list and dict paths already recover. Both keep
round-trip intact.

Minimal reproduction

use starlark_cst::{Dialect, parse};

#[test]
fn rejects_empty_argument_slots() {
    for src in ["foo(a, , b)\n", "foo(, a)\n", "foo(,)\n"] {
        let parsed = parse(src, Dialect::Bazel);
        assert_eq!(parsed.syntax().to_string(), src, "round-trip must still hold");
        assert!(!parsed.errors().is_empty(), "no error reported for {src:?}");
    }

    // and the legal trailing comma must stay legal
    let ok = parse("foo(a,)\n", Dialect::Bazel);
    assert!(ok.errors().is_empty());
}

> Original issue: source #4 > Original author: `barrettruth` > Original date: 2026-08-26T12:01:07Z ## Version 0.1.1 ## Dialect `Dialect::Bazel`, reproduced on `BUILD` and `.bzl`. Not dialect-specific. ## Which guarantee broke Wrong tree shape, plus a missing error. Round-trip holds and nothing panics. ## Observed An empty slot in an argument list is accepted silently and produces a zero-width `ARG` node: ``` $ parse("foo(a, , b)\n", Dialect::Bazel) errors: 0 FILE@0..12 EXPR_STMT@0..11 CALL_EXPR@0..11 IDENT_EXPR@0..3 IDENT@0..3 "foo" ARG_LIST@3..11 L_PAREN@3..4 "(" ARG@4..5 IDENT_EXPR@4..5 IDENT@4..5 "a" COMMA@5..6 "," WHITESPACE@6..7 " " ARG@7..7 <-- empty COMMA@7..8 "," WHITESPACE@8..9 " " ARG@9..10 IDENT_EXPR@9..10 IDENT@9..10 "b" R_PAREN@10..11 ")" ``` `foo(, a)` and `foo(,)` are likewise accepted with zero errors. ## Why this is a bug and not a tolerated recovery Lists and dicts already reject the same construct, so this is an argument-list oversight rather than a deliberate policy: | input | errors | | --- | --- | | `x = [1, , 2]` | 3 | | `x = {1: 2, , 3: 4}` | 4 | | `foo(a, , b)` | **0** | | `foo(, a)` | **0** | | `foo(,)` | **0** | A zero-width `ARG` is also not a well-formed node. Every other `ARG` in the tree spans its expression; consumers that walk `ARG_LIST` — `CallExpr::arg`, anything counting positional arguments, anything mapping a cursor offset to an argument — get a node with no content and no position to report. In a `BUILD` file that is `cc_library(name = "a", , srcs = [...])` silently presenting an extra argument. ## Precedent Both reference implementations reject it, at the comma: ``` # Bazel, Parser.java: syntax error at ',': expected expression # starlark-rust 0.14.2 error: Parse error: unexpected symbol ',', expected expression --> t:1:8 | 1 | foo(a, , b) | ^ | error: Parse error: unexpected symbol ',', expected expression --> t:1:5 | 1 | foo(, a) | ^ | ``` Note the anchor is the offending comma, and it is one character wide. Bazel's own grammar permits a trailing comma before `)` and nothing else; `Parser.java` carries `Trailing comma is allowed only in parenthesized tuples.` for the adjacent case, so the distinction between *trailing* and *empty slot* is one it draws explicitly. ## Proposed behaviour Reject an empty argument slot with the same message the parser already uses elsewhere for a missing operand, anchored on the comma that opens the empty slot and one character wide: | input | error | range | | --- | --- | --- | | `foo(a, , b)` | `expected an expression` | the second `,`, `7..8` | | `foo(, a)` | `expected an expression` | the `,`, `4..5` | | `foo(,)` | `expected an expression` | the `,`, `4..5` | `foo(a,)` must stay accepted — a single trailing comma before `)` is legal. The zero-width `ARG` node should not be produced. Either omit the node and let the `COMMA` tokens sit directly in `ARG_LIST`, or wrap the comma in an `ERROR` node, whichever matches how the list and dict paths already recover. Both keep round-trip intact. ## Minimal reproduction ```rust use starlark_cst::{Dialect, parse}; #[test] fn rejects_empty_argument_slots() { for src in ["foo(a, , b)\n", "foo(, a)\n", "foo(,)\n"] { let parsed = parse(src, Dialect::Bazel); assert_eq!(parsed.syntax().to_string(), src, "round-trip must still hold"); assert!(!parsed.errors().is_empty(), "no error reported for {src:?}"); } // and the legal trailing comma must stay legal let ok = parse("foo(a,)\n", Dialect::Bazel); assert!(ok.errors().is_empty()); } ```

barrettruth

2026-09-21 15:26:38 +00:00