yeast: Clarify semantics surrounding captured sequences · github/codeql@953f73f · GitHub
Skip to content

Commit 953f73f

Browse files
tausbntausbn
committed
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.
1 parent 324de63 commit 953f73f

3 files changed

Lines changed: 77 additions & 54 deletions

File tree

‎shared/yeast-macros/src/ast.rs‎

Lines changed: 10 additions & 0 deletions

‎shared/yeast-macros/src/rule_parse.rs‎

Lines changed: 65 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,21 @@ fn parse_pattern_list(tokens: &mut Tokens) -> Result<Vec<Pattern>> {
146146
if is_single_pattern {
147147
parse_parenthesized_pattern(&mut inner)?
148148
} else {
149-
Pattern::Sequence(parse_pattern_list(&mut inner)?)
149+
let patterns = parse_pattern_list(&mut inner)?;
150+
if patterns.is_empty() {
151+
return Err(syn::Error::new(
152+
group.span(),
153+
"empty query groups are not allowed",
154+
));
155+
}
156+
if patterns.len() == 1 {
157+
return Err(syn::Error::new(
158+
group.span(),
159+
"single-pattern query groups are not allowed; \
160+
remove the redundant parentheses",
161+
));
162+
}
163+
Pattern::Sequence(patterns)
150164
}
151165
} else {
152166
parse_parenthesized_pattern(&mut inner)?
@@ -183,6 +197,12 @@ fn maybe_capture(
183197
cardinality: Cardinality,
184198
) -> Result<Pattern> {
185199
if peek_is_at(tokens) {
200+
if !pattern.matches_single_node() {
201+
return Err(syn::Error::new_spanned(
202+
tokens.peek().unwrap().clone(),
203+
"cannot capture a query sequence; capture the desired nodes explicitly",
204+
));
205+
}
186206
let (name, raw) = consume_capture(tokens)?;
187207
Ok(Pattern::Capture {
188208
name,
@@ -201,53 +221,12 @@ fn maybe_capture(
201221
}
202222

203223
fn maybe_repeat(tokens: &mut Tokens, pattern: Pattern) -> Result<Pattern> {
204-
if !peek_is_repetition(tokens) {
205-
return maybe_capture(tokens, pattern, Cardinality::SINGLE);
206-
}
207-
208-
let cardinality = expect_cardinality(tokens)?;
209-
if !peek_is_at(tokens) {
210-
return Ok(Pattern::Repeated {
211-
pattern: Box::new(pattern),
212-
cardinality,
213-
});
214-
}
215-
216-
if matches!(
217-
&pattern,
218-
Pattern::Sequence(patterns)
219-
if patterns.iter().any(|pattern| matches!(pattern, Pattern::Repeated { .. }))
220-
) {
221-
return Err(syn::Error::new_spanned(
222-
tokens.peek().unwrap().clone(),
223-
"cannot capture a repeated group containing a nested repetition; \
224-
capture the desired inner patterns explicitly",
225-
));
226-
}
227-
228-
let (name, raw) = consume_capture(tokens)?;
229-
Ok(match pattern {
230-
Pattern::Sequence(patterns) => Pattern::Repeated {
231-
pattern: Box::new(Pattern::Sequence(
232-
patterns
233-
.into_iter()
234-
.map(|pattern| Pattern::Capture {
235-
name: name.clone(),
236-
raw,
237-
pattern: Box::new(pattern),
238-
cardinality: Cardinality::SINGLE,
239-
})
240-
.collect(),
241-
)),
242-
cardinality,
243-
},
244-
pattern => Pattern::Capture {
245-
name,
246-
raw,
247-
pattern: Box::new(pattern),
248-
cardinality,
249-
},
250-
})
224+
let cardinality = if peek_is_repetition(tokens) {
225+
expect_cardinality(tokens)?
226+
} else {
227+
Cardinality::SINGLE
228+
};
229+
maybe_capture(tokens, pattern, cardinality)
251230
}
252231

253232
fn parse_replacement(input: TokenStream) -> Result<Replacement> {
@@ -553,15 +532,48 @@ mod tests {
553532
}
554533

555534
#[test]
556-
fn rejects_capture_on_repeated_group_with_nested_repetition() {
557-
let result = parse_pattern(quote!((root ((item)* (separator))* @items)));
535+
fn rejects_one_element_repeated_group() {
536+
let result = parse_pattern(quote!((array ((identifier))* @items)));
537+
let Err(error) = result else {
538+
panic!("expected redundant query group to be rejected");
539+
};
540+
assert_eq!(
541+
error.to_string(),
542+
"single-pattern query groups are not allowed; remove the redundant parentheses"
543+
);
544+
}
545+
546+
#[test]
547+
fn rejects_empty_repeated_group() {
548+
let result = parse_pattern(quote!((array ()*)));
549+
let Err(error) = result else {
550+
panic!("expected empty query group to be rejected");
551+
};
552+
assert_eq!(error.to_string(), "empty query groups are not allowed");
553+
}
554+
555+
#[test]
556+
fn rejects_capture_of_query_sequence() {
557+
let result = maybe_capture(
558+
&mut quote!(@item).into_iter().peekable(),
559+
Pattern::Sequence(vec![
560+
Pattern::Node {
561+
kind: "identifier".to_string(),
562+
fields: Vec::new(),
563+
},
564+
Pattern::Node {
565+
kind: "integer".to_string(),
566+
fields: Vec::new(),
567+
},
568+
]),
569+
Cardinality::SINGLE,
570+
);
558571
let Err(error) = result else {
559-
panic!("expected nested repetition capture to be rejected");
572+
panic!("expected multi-node sequence capture to be rejected");
560573
};
561574
assert_eq!(
562575
error.to_string(),
563-
"cannot capture a repeated group containing a nested repetition; \
564-
capture the desired inner patterns explicitly"
576+
"cannot capture a query sequence; capture the desired nodes explicitly"
565577
);
566578
}
567579
}

‎shared/yeast/src/query.rs‎

Lines changed: 2 additions & 1 deletion

0 commit comments

Comments
 (0)