yeast: Clarify semantics surrounding captured sequences

Copilot's second review sent me down a bit of a rabbit hole.

The fundamental question is the following: what (if anything) does it
mean to capture a query sequence?

That is, consider the following query:
```
(foo ((bar) (baz)) @quux)
```

Here, `@quux` is attached to a subquery that matches a sequence of
nodes, `bar` followerd by `baz`. So, what should this actually capture?
The current implementation treats this in a somewhat surprising way,
equivalent to if we had written
```
(foo ((bar) @quux (baz) @quux))
```
that is, both kinds of child nodes get pushed into the `quux` capture.

To me, this behaviour is very surprising and unintuitive (and since
there is an explicit way to express this anyway, there doesn't seem to
be much reason to prefer the "shorthand" version).

For this reason, the present commit just disallows adding a capture to a
query sequence entirely. A capture is only attached to a single query
node, never a sequence of nodes.

Note that this does not mean that a _capture_ can't contain a sequence
of nodes. Both
```
(foo (bar)* @bars)
```
and
```
(foo ((bar) @bars)*)
```
(which are equivalent) capture all of the `bar` children and put them in
`bars` as a `Vec<Id>`.

In the new AST, we represent the syntax in the order it appears, so
`(bar)* @bars` is a capture of a repeated node match. However, when we
lower this into an executable query, we push the capture in, so it
becomes more like the second version. We're still only capturing a
single node at a time, but we may do so several times due to the
repetition.

(One might consider getting rid of the first syntax entirely, then, but
I think it's more readable to write this as "capture of repeat" rather
than "repeat of capture".)

Finally, in addition to no longer allowing captures to apply to
sequences, we now also do not allow sequences to be empty (i.e. `()`) or
contain a single element. The first would have no effect (an empty
sequence always succeeds), and the second is redundant (a singleton
sequence is equivalent to its single element). All of these checks are
enforced at compile-time.

Author: tausbn

shared/yeast-macros/src/ast.rs

@@ -54,6 +54,13 @@ impl Cardinality {
         multiple: true,
         required: true,
     };
+
+    pub(crate) fn combine(self, other: Self) -> Self {
+        Self {
+            multiple: self.multiple || other.multiple,
+            required: self.required && other.required,
+        }
+    }
 }
 
 /// An input pattern such as `(call method: (identifier) @name)`.
@@ -81,6 +88,27 @@ pub(crate) enum Pattern {
     },
 }
 
+impl Pattern {
+    /// The cardinality of an ordinary node capture around this pattern.
+    ///
+    /// Sequences require explicit inner captures.
+    pub(crate) fn capture_cardinality(&self) -> Option<Cardinality> {
+        match self {
+            Pattern::Any { .. } | Pattern::Node { .. } | Pattern::Unnamed(_) => {
+                Some(Cardinality::SINGLE)
+            }
+            Pattern::Capture { pattern, .. } => pattern.capture_cardinality(),
+            Pattern::Repeated {
+                pattern,
+                cardinality,
+            } => pattern
+                .capture_cardinality()
+                .map(|inner| inner.combine(*cardinality)),
+            Pattern::Sequence(_) => None,
+        }
+    }
+}
+
 #[derive(Clone)]
 pub(crate) struct Capture {
     /// Capture identifier after `@` or `@@`.

shared/yeast-macros/src/lower.rs

@@ -128,6 +128,11 @@ impl Pattern {
     fn lower_list(&self) -> Vec<TokenStream> {
         match self {
             Pattern::Sequence(patterns) => patterns.iter().flat_map(Pattern::lower_list).collect(),
+            Pattern::Capture { capture, pattern }
+                if matches!(pattern.as_ref(), Pattern::Repeated { .. }) =>
+            {
+                pattern.lower_list_with_capture(&capture.name.to_string())
+            }
             Pattern::Repeated {
                 pattern,
                 cardinality,
@@ -152,6 +157,41 @@ impl Pattern {
             }
         }
     }
+
+    fn lower_list_with_capture(&self, capture: &str) -> Vec<TokenStream> {
+        match self {
+            Pattern::Repeated {
+                pattern,
+                cardinality,
+            } => {
+                let children = pattern.lower_list_with_capture(capture);
+                let repetition = match (cardinality.multiple, cardinality.required) {
+                    (true, false) => quote! { yeast::query::Rep::ZeroOrMore },
+                    (true, true) => quote! { yeast::query::Rep::OneOrMore },
+                    (false, false) => quote! { yeast::query::Rep::ZeroOrOne },
+                    (false, true) => unreachable!("single patterns are not wrapped as repeated"),
+                };
+                vec![quote! {
+                    yeast::query::QueryListElem::Repeated {
+                        children: vec![#(#children),*],
+                        rep: #repetition,
+                    }
+                }]
+            }
+            pattern => {
+                let pattern = pattern.lower();
+                vec![quote! {
+                    yeast::query::QueryListElem::SingleNode(
+                        yeast::query::QueryNode::Capture {
+                            capture: #capture,
+                            node: Box::new(#pattern),
+                        }
+                    )
+                }]
+            }
+        }
+    }
+
     fn captures(&self) -> Vec<BoundCapture> {
         let mut captures = Vec::new();
         self.collect_captures(Cardinality::SINGLE, &mut captures);
@@ -168,9 +208,12 @@ impl Pattern {
             }
             Pattern::Capture { capture, pattern } => {
                 pattern.collect_captures(cardinality, captures);
+                let captured = pattern
+                    .capture_cardinality()
+                    .expect("capture patterns are validated during parsing");
                 captures.push(BoundCapture {
                     capture: capture.clone(),
-                    cardinality,
+                    cardinality: combine_cardinality(cardinality, captured),
                 });
             }
             Pattern::Sequence(patterns) => {

shared/yeast-macros/src/rule_parse.rs

@@ -30,14 +30,7 @@ pub(crate) fn parse_rule(input: TokenStream) -> Result<Rule> {
 
 fn parse_pattern_with_capture(tokens: &mut Tokens) -> Result<Pattern> {
     let pattern = parse_pattern_atom(tokens)?;
-    if peek_is_at(tokens) {
-        Ok(Pattern::Capture {
-            capture: consume_capture(tokens)?,
-            pattern: Box::new(pattern),
-        })
-    } else {
-        Ok(pattern)
-    }
+    maybe_capture(tokens, pattern)
 }
 
 fn parse_pattern_atom(tokens: &mut Tokens) -> Result<Pattern> {
@@ -116,11 +109,11 @@ fn parse_pattern_fields(tokens: &mut Tokens) -> Result<Vec<(String, Pattern)>> {
             };
             let pattern = if peek_is_repetition(tokens) {
                 let cardinality = expect_cardinality(tokens)?;
-                let pattern = capture_repeated_pattern(tokens, atom)?;
-                Pattern::Repeated {
-                    pattern: Box::new(pattern),
+                let repeated = Pattern::Repeated {
+                    pattern: Box::new(atom),
                     cardinality,
-                }
+                };
+                maybe_capture(tokens, repeated)?
             } else {
                 maybe_capture(tokens, atom)?
             };
@@ -163,13 +156,27 @@ fn parse_pattern_list(tokens: &mut Tokens) -> Result<Vec<Pattern>> {
                 let repeated = if is_single_pattern {
                     parse_parenthesized_pattern(&mut inner)?
                 } else {
-                    Pattern::Sequence(parse_pattern_list(&mut inner)?)
+                    let patterns = parse_pattern_list(&mut inner)?;
+                    if patterns.is_empty() {
+                        return Err(syn::Error::new(
+                            group.span(),
+                            "empty query groups are not allowed",
+                        ));
+                    }
+                    if patterns.len() == 1 {
+                        return Err(syn::Error::new(
+                            group.span(),
+                            "single-pattern query groups are not allowed; \
+                             remove the redundant parentheses",
+                        ));
+                    }
+                    Pattern::Sequence(patterns)
                 };
-                let repeated = capture_repeated_pattern(tokens, repeated)?;
-                patterns.push(Pattern::Repeated {
+                let repeated = Pattern::Repeated {
                     pattern: Box::new(repeated),
                     cardinality,
-                });
+                };
+                patterns.push(maybe_capture(tokens, repeated)?);
             } else {
                 let pattern = parse_parenthesized_pattern(&mut inner)?;
                 patterns.push(maybe_capture(tokens, pattern)?);
@@ -203,6 +210,12 @@ fn parse_pattern_list(tokens: &mut Tokens) -> Result<Vec<Pattern>> {
 
 fn maybe_capture(tokens: &mut Tokens, pattern: Pattern) -> Result<Pattern> {
     if peek_is_at(tokens) {
+        if pattern.capture_cardinality().is_none() {
+            return Err(syn::Error::new_spanned(
+                tokens.peek().unwrap().clone(),
+                "cannot capture a query sequence; capture the desired nodes explicitly",
+            ));
+        }
         Ok(Pattern::Capture {
             capture: consume_capture(tokens)?,
             pattern: Box::new(pattern),
@@ -218,46 +231,11 @@ fn maybe_repeat(tokens: &mut Tokens, pattern: Pattern) -> Result<Pattern> {
     }
 
     let cardinality = expect_cardinality(tokens)?;
-    let pattern = capture_repeated_pattern(tokens, pattern)?;
-    Ok(Pattern::Repeated {
+    let repeated = Pattern::Repeated {
         pattern: Box::new(pattern),
         cardinality,
-    })
-}
-
-fn capture_repeated_pattern(tokens: &mut Tokens, pattern: Pattern) -> Result<Pattern> {
-    if !peek_is_at(tokens) {
-        return Ok(pattern);
-    }
-
-    if matches!(
-        &pattern,
-        Pattern::Sequence(patterns)
-            if patterns.iter().any(|pattern| matches!(pattern, Pattern::Repeated { .. }))
-    ) {
-        return Err(syn::Error::new_spanned(
-            tokens.peek().unwrap().clone(),
-            "cannot capture a repeated group containing a nested repetition; \
-             capture the desired inner patterns explicitly",
-        ));
-    }
-
-    let capture = consume_capture(tokens)?;
-    Ok(match pattern {
-        Pattern::Sequence(patterns) => Pattern::Sequence(
-            patterns
-                .into_iter()
-                .map(|pattern| Pattern::Capture {
-                    capture: capture.clone(),
-                    pattern: Box::new(pattern),
-                })
-                .collect(),
-        ),
-        pattern => Pattern::Capture {
-            capture,
-            pattern: Box::new(pattern),
-        },
-    })
+    };
+    maybe_capture(tokens, repeated)
 }
 
 fn parse_replacement(input: TokenStream) -> Result<Replacement> {
@@ -517,10 +495,16 @@ mod tests {
             Pattern::Sequence(patterns)
                 if matches!(
                     patterns.as_slice(),
-                    [Pattern::Repeated {
+                    [Pattern::Capture {
                         pattern,
-                        cardinality: Cardinality::ZERO_OR_MORE,
-                    }] if matches!(pattern.as_ref(), Pattern::Capture { .. })
+                        ..
+                    }] if matches!(
+                        pattern.as_ref(),
+                        Pattern::Repeated {
+                            cardinality: Cardinality::ZERO_OR_MORE,
+                            ..
+                        }
+                    )
                 )
         ));
     }
@@ -539,15 +523,74 @@ mod tests {
     }
 
     #[test]
-    fn rejects_capture_on_repeated_group_with_nested_repetition() {
-        let result = parse_pattern(quote!((root ((item)* (separator))* @items)));
+    fn repeated_capture_wraps_the_repetition_in_the_ast() {
+        let pattern = parse_pattern(quote!((array (identifier)* @items))).unwrap();
+
+        let Pattern::Node { fields, .. } = pattern else {
+            panic!("expected array node pattern");
+        };
+        let Pattern::Sequence(children) = &fields[0].1 else {
+            panic!("expected child sequence");
+        };
+        let Pattern::Capture {
+            capture,
+            pattern: repeated,
+        } = &children[0]
+        else {
+            panic!("expected capture around the repetition");
+        };
+        assert_eq!(capture.name, "items");
+        assert!(matches!(
+            repeated.as_ref(),
+            Pattern::Repeated {
+                cardinality: Cardinality::ZERO_OR_MORE,
+                ..
+            }
+        ));
+    }
+
+    #[test]
+    fn rejects_one_element_repeated_group() {
+        let result = parse_pattern(quote!((array ((identifier))* @items)));
+        let Err(error) = result else {
+            panic!("expected redundant query group to be rejected");
+        };
+        assert_eq!(
+            error.to_string(),
+            "single-pattern query groups are not allowed; remove the redundant parentheses"
+        );
+    }
+
+    #[test]
+    fn rejects_empty_repeated_group() {
+        let result = parse_pattern(quote!((array ()*)));
+        let Err(error) = result else {
+            panic!("expected empty query group to be rejected");
+        };
+        assert_eq!(error.to_string(), "empty query groups are not allowed");
+    }
+
+    #[test]
+    fn rejects_capture_of_query_sequence() {
+        let result = maybe_capture(
+            &mut quote!(@item).into_iter().peekable(),
+            Pattern::Sequence(vec![
+                Pattern::Node {
+                    kind: "identifier".to_string(),
+                    fields: Vec::new(),
+                },
+                Pattern::Node {
+                    kind: "integer".to_string(),
+                    fields: Vec::new(),
+                },
+            ]),
+        );
         let Err(error) = result else {
-            panic!("expected nested repetition capture to be rejected");
+            panic!("expected multi-node sequence capture to be rejected");
         };
         assert_eq!(
             error.to_string(),
-            "cannot capture a repeated group containing a nested repetition; \
-             capture the desired inner patterns explicitly"
+            "cannot capture a query sequence; capture the desired nodes explicitly"
         );
     }
 }

shared/yeast/src/query.rs

@@ -143,7 +143,8 @@ impl QueryListElem {
         match self {
             QueryListElem::Repeated { children, rep } => {
                 if children.is_empty() {
-                    // Empty repetition always succeeds without consuming
+                    // Macro parsing rejects empty groups. Retain safe
+                    // zero-width behavior for manually constructed queries.
                     return Ok(*rep != Rep::OneOrMore);
                 }