Select the types of activity you want to include in your feed.
Fix incorrect JS for overlapping string prefixes
Pattern matching on overlapping string prefixes with guards could generate incorrect JavaScript. Distinguish hoistable guards from pattern-bound guards so fall-through to shorter prefixes works.
···479479- Fixed a bug where warnings and errors in the arguments of a call to a
480480 function literal would be reported multiple times.
481481 ([John Downey](https://github.com/jtdowney))
482482+483483+- Fixed a bug where pattern matching on overlapping string prefixes with guards
484484+ could generate incorrect JavaScript.
485485+ ([John Downey](https://github.com/jtdowney))
···136136 ///
137137 alternative_index: usize,
138138 checks: Vec<PatternCheck>,
139139- guard: Option<usize>,
139139+ guard: Option<BranchGuard>,
140140 body: Body,
141141}
142142143143+/// A branch's guard, holding the index of the clause it belongs to and whether
144144+/// it references variables bound by the branch's patterns.
145145+#[derive(Clone, Copy, Eq, PartialEq, Debug)]
146146+struct BranchGuard {
147147+ clause_index: usize,
148148+ uses_pattern_bindings: bool,
149149+}
150150+143151impl Branch {
144152 fn new(
145153 clause_index: usize,
146154 alternative_index: usize,
147155 checks: Vec<PatternCheck>,
148148- has_guard: bool,
156156+ guard: Option<BranchGuard>,
149157 ) -> Self {
150158 Self {
151159 clause_index,
152160 alternative_index,
153161 checks,
154154- guard: if has_guard { Some(clause_index) } else { None },
162162+ guard,
155163 body: Body::new(clause_index),
156164 }
157165 }
···23622370 // if the condition evaluates to `True` we can run its body.
23632371 // Otherwise, we'll have to keep looking at the remaining branches
23642372 // to know what to do if this branch doesn't match.
23652365- Some(guard) => {
23732373+ Some(BranchGuard { clause_index, .. }) => {
23662374 let if_true = first_branch.body.clone();
23672375 // All the remaining branches will be compiled and end up
23682376 // in the path of the tree to choose if the guard is false.
23692377 let _ = branches.pop_front();
23702378 let if_false = self.compile(branches);
23712371- Decision::guard(guard, if_true, if_false)
23792379+ Decision::guard(clause_index, if_true, if_false)
23722380 }
23732381 },
23742382 }
···26002608 ///
26012609 fn new_checks(
26022610 &mut self,
26112611+ subject: &Variable,
26032612 for_pattern: &Pattern,
26042613 after_succeding_check: &RuntimeCheck,
26052614 ) -> Vec<PatternCheck> {
···26972706 // we know it already starts with `"wibble"` we can just check that the remaining
26982707 // part after that starts with the missing part of the prefix:
26992708 // `prefix0 is "st" <> rest1`.
27092709+ //
27102710+ // The opposite can also happen. The prefix we've already checked might be longer
27112711+ // than (and start with) the pattern's prefix. This comes up when an earlier branch
27122712+ // matched the longer prefix under a guard that then failed, so control falls
27132713+ // through to this shorter-prefix branch. For example we might be checking the
27142714+ // pattern `"wib" <> rest1` knowing that `"wibble" <> rest0` already succeeded.
27152715+ //
27162716+ // This rests on an invariant established by `check_overlaps`. A shorter prefix is
27172717+ // only ever paired with an already-succeeded longer prefix when that longer prefix
27182718+ // starts with it. Two disjoint prefixes (say `"b"` reached after `"aaa" <> rest0`
27192719+ // succeeded) are never routed here together. So when `prefix1.strip_prefix(prefix0)`
27202720+ // returns `None` below it is precisely because `prefix0` starts with `prefix1`, and
27212721+ // the pattern's prefix is therefore already guaranteed to match. If it binds nothing
27222722+ // we're done. Otherwise we rebind its `rest1` to the value with the (shorter) prefix
27232723+ // stripped off.
27002724 (
27012725 Pattern::StringPrefix {
27022726 prefix: prefix1,
27032703- prefix_name: _,
27272727+ prefix_name,
27042728 rest: rest1,
27052729 },
27062730 RuntimeCheck::StringPrefix {
···27082732 rest: rest0,
27092733 },
27102734 ) => {
27112711- let remaining = prefix1.strip_prefix(prefix0.as_str()).unwrap_or(prefix1);
27122712- // If the prefixes are exactly the same then the only remaining check
27132713- // is for the two remaining bits to be the same.
27142714- if remaining.is_empty() {
27152715- vec![rest0.is(*rest1)]
27352735+ if let Some(remaining) = prefix1.strip_prefix(prefix0.as_str()) {
27362736+ // The pattern's prefix extends (or equals) the one we've already checked.
27372737+ // If the prefixes are exactly the same then the only remaining check is
27382738+ // for the two remaining bits to be the same.
27392739+ if remaining.is_empty() {
27402740+ vec![rest0.is(*rest1)]
27412741+ } else {
27422742+ vec![rest0.is(self.string_prefix_pattern(remaining, *rest1))]
27432743+ }
27442744+ } else if prefix_name.is_none() && matches!(self.pattern(*rest1), Pattern::Discard)
27452745+ {
27462746+ // The prefix we've already checked is longer than the pattern's prefix, so
27472747+ // the pattern is guaranteed to match. As it binds nothing there's nothing
27482748+ // left to do.
27492749+ vec![]
27162750 } else {
27172717- vec![rest0.is(self.string_prefix_pattern(remaining, *rest1))]
27512751+ // The pattern is guaranteed to match but it does bind something, so we
27522752+ // re-issue it on the original value to bind `rest1` to the correctly sliced
27532753+ // string (preserving any prefix name like `"a" as first <> rest1`).
27542754+ let pattern = self.patterns.alloc(Pattern::StringPrefix {
27552755+ prefix: prefix1.clone(),
27562756+ prefix_name: prefix_name.clone(),
27572757+ rest: *rest1,
27582758+ });
27592759+ vec![subject.is(pattern)]
27182760 }
27192761 }
27202762···29252967 .to_runtime_check_kind()
29262968 .expect("no unconditional patterns left");
2927296929282928- let indices_of_overlapping_checks = self.indices_of_overlapping_checks(&kind);
29292929- if indices_of_overlapping_checks.is_empty() {
29302930- // This is a new choice we haven't yet discovered as it is not overlapping
29312931- // with any of the existing ones. So we add it as a possible new path
29322932- // we might have to go down to in the decision tree.
29702970+ let CheckOverlaps {
29712971+ overlapping,
29722972+ needs_new_choice,
29732973+ } = self.check_overlaps(&kind);
29742974+29752975+ // The branch is relevant to every existing choice its check overlaps
29762976+ // with, so we add it (together with any newly discovered checks) to all
29772977+ // of those paths. For a string prefix this also includes any longer
29782978+ // prefixes that start with it. When one of those matched but a guard
29792979+ // then failed, control falls through to this shorter prefix.
29802980+ for index in overlapping.iter() {
29812981+ let (overlapping_check, branches) = self
29822982+ .choices
29832983+ .get_mut(*index)
29842984+ .expect("check to already be a choice");
29852985+29862986+ let mut branch = branch.clone();
29872987+ for new_check in compiler.new_checks(&pattern_check.var, &pattern, overlapping_check) {
29882988+ branch.add_check(new_check);
29892989+ }
29902990+ branches.push_back(branch);
29912991+ }
29922992+29932993+ // Unless the check is fully subsumed by an existing choice, we also add
29942994+ // it as a new path we might have to go down to in the decision tree.
29952995+ // A shorter prefix still needs its own choice even when it overlaps
29962996+ // earlier longer prefixes, so it can match values outside those.
29972997+ if needs_new_choice {
29332998 self.save_index_of_new_choice(kind.clone());
2934299929353000 let check = compiler.fresh_runtime_check(kind, branch_mode);
29362936- for new_check in compiler.new_checks(&pattern, &check) {
30013001+ for new_check in compiler.new_checks(&pattern_check.var, &pattern, &check) {
29373002 branch.add_check(new_check);
29383003 }
29393004 let mut branches = self.fallback.clone();
29403005 branches.push_back(branch);
2941300629423007 self.choices.push((check, branches));
29432943- } else {
29442944- // Otherwise, we know that the check for this branch overlaps with
29452945- // (possibly more than one) existing checks and so is relevant only
29462946- // as part of those existing paths.
29472947- // We'll add the branch with its newly discovered checks only to those
29482948- // paths.
29492949- for index in indices_of_overlapping_checks.iter() {
29502950- let (overlapping_check, branches) = self
29512951- .choices
29522952- .get_mut(*index)
29532953- .expect("check to already be a choice");
29542954-29552955- let mut branch = branch.clone();
29562956- for new_check in compiler.new_checks(&pattern, overlapping_check) {
29572957- branch.add_check(new_check);
29582958- }
29592959- branches.push_back(branch);
29602960- }
29613008 }
2962300929633010 // Then we have to update all variant checks with any new name/module
···29703017 ..
29713018 } = pattern
29723019 {
29732973- for index in indices_of_overlapping_checks {
30203020+ for index in overlapping {
29743021 let (check, _) = self
29753022 .choices
29763023 .get_mut(index)
···30833130 };
30843131 }
3085313230863086- fn indices_of_overlapping_checks(&self, kind: &RuntimeCheckKind) -> Vec<usize> {
31333133+ fn choice_has_guard_using_pattern_bindings(&self, index: usize) -> bool {
31343134+ self.choices.get(index).is_some_and(|(_, branches)| {
31353135+ branches.iter().any(|branch| {
31363136+ branch
31373137+ .guard
31383138+ .is_some_and(|guard| guard.uses_pattern_bindings)
31393139+ })
31403140+ })
31413141+ }
31423142+31433143+ fn check_overlaps(&self, kind: &RuntimeCheckKind) -> CheckOverlaps {
30873144 match kind {
30883145 // All these checks will only overlap with a check that is exactly the
30893146 // same, so we just look up their index in the `indices` map using the
30903090- // kind as the lookup.
31473147+ // kind as the lookup. An identical existing choice fully subsumes this
31483148+ // one, so we only need a new choice when there isn't one already.
30913149 RuntimeCheckKind::Int { .. }
30923150 | RuntimeCheckKind::Float { .. }
30933151 | RuntimeCheckKind::Tuple { .. }
30943152 | RuntimeCheckKind::Variant { .. }
30953153 | RuntimeCheckKind::EmptyList
30963154 | RuntimeCheckKind::NonEmptyList => {
30973097- self.indices.get(kind).cloned().into_iter().collect_vec()
31553155+ let overlapping = self.indices.get(kind).cloned().into_iter().collect_vec();
31563156+ let needs_new_choice = overlapping.is_empty();
31573157+31583158+ CheckOverlaps {
31593159+ overlapping,
31603160+ needs_new_choice,
31613161+ }
30983162 }
3099316331003164 // String patterns are a bit more tricky as they might end up overlapping
···31173181 // That is, we look for a prefix of the pattern we're checking in the prefix
31183182 // trie.
31193183 RuntimeCheckKind::StringPrefix { prefix: value } => {
31203120- ancestors_values(&self.prefix_indices, value).collect_vec()
31843184+ // A prefix pattern overlaps with any of its own prefixes, and also with a
31853185+ // longer prefix it is itself a prefix of. Once that longer prefix matched,
31863186+ // this shorter one is guaranteed to match too.
31873187+ match self.prefix_indices.get_ancestor(value.as_str()) {
31883188+ // One of `value`'s own (shorter) prefixes is already a choice. Any
31893189+ // value that could match `value` is routed into that choice first, so
31903190+ // `value` doesn't need its own. It just joins that choice (and any
31913191+ // longer prefixes nested under it whose guard could fall through to it).
31923192+ Some(_) => {
31933193+ let overlapping = overlap_candidates(&self.prefix_indices, value)
31943194+ .filter(|&(prefix, index)| {
31953195+ if value.starts_with(prefix) {
31963196+ // An ancestor (or the prefix itself). `value` extends it,
31973197+ // so it always belongs in that choice.
31983198+ true
31993199+ } else {
32003200+ // A longer prefix which is only relevant as a fall-through
32013201+ // target, which only happens when it can match its prefix
32023202+ // but fail a guard.
32033203+ prefix.starts_with(value.as_str())
32043204+ && self.choice_has_guard_using_pattern_bindings(index)
32053205+ }
32063206+ })
32073207+ .map(|(_, index)| index)
32083208+ .collect_vec();
32093209+32103210+ CheckOverlaps {
32113211+ overlapping,
32123212+ needs_new_choice: false,
32133213+ }
32143214+ }
32153215+ // No shorter prefix of `value` is a choice yet, so `value` needs its
32163216+ // own choice to match values outside any longer prefixes. It is added to
32173217+ // an existing longer prefix only when that prefix can match but fail a
32183218+ // guard, so control has to fall through to this shorter prefix rather
32193219+ // than escaping to the next choice.
32203220+ None => {
32213221+ let overlapping = descendant_candidates(&self.prefix_indices, value)
32223222+ .filter(|&(prefix, index)| {
32233223+ prefix.starts_with(value.as_str())
32243224+ && self.choice_has_guard_using_pattern_bindings(index)
32253225+ })
32263226+ .map(|(_, index)| index)
32273227+ .collect_vec();
32283228+32293229+ CheckOverlaps {
32303230+ overlapping,
32313231+ needs_new_choice: true,
32323232+ }
32333233+ }
32343234+ }
31213235 }
3122323631233237 // Strings are almost exactly the same, except they could also have an exact
···31253239 // another string pattern (if they're matching on the same value), or with
31263240 // one or more string prefix patterns with a matching prefix.
31273241 RuntimeCheckKind::String { value } => {
32423242+ // Unlike a prefix pattern, an exact literal can only overlap with one of
32433243+ // its own prefixes. A longer prefix can never match an exact literal (the
32443244+ // literal is too short), so we leave those out.
31283245 let first_index = self.indices.get(kind).cloned();
31293129- first_index
32463246+ let overlapping = first_index
31303247 .into_iter()
31313131- .chain(ancestors_values(&self.prefix_indices, value))
31323132- .collect_vec()
32483248+ .chain(
32493249+ overlap_candidates(&self.prefix_indices, value)
32503250+ .filter(|&(prefix, _)| value.starts_with(prefix))
32513251+ .map(|(_, index)| index),
32523252+ )
32533253+ .collect_vec();
32543254+ let needs_new_choice = overlapping.is_empty();
32553255+32563256+ CheckOverlaps {
32573257+ overlapping,
32583258+ needs_new_choice,
32593259+ }
31333260 }
31343261 }
31353262 }
31363263}
3137326431383138-fn ancestors_values(trie: &Trie<String, usize>, key: &str) -> impl Iterator<Item = usize> {
32653265+/// Describes how a branch's runtime check relates to the choices discovered so
32663266+/// far when splitting branches.
32673267+struct CheckOverlaps {
32683268+ /// The indices of the existing choices this branch is relevant to and so
32693269+ /// must be added to
32703270+ overlapping: Vec<usize>,
32713271+ /// Whether the check also needs to become a new choice of its own. A new choice is a
32723272+ /// fresh branch of the decision tree, carrying its own runtime check.
32733273+ ///
32743274+ /// For example, suppose `"wib" <> _` is reached after `"wibble" <> _` is already a
32753275+ /// choice. A string can start with `"wib"` without starting with `"wibble"`
32763276+ /// (for example `"wibz"`), so `"wib"` is not subsumed and needs its own choice to
32773277+ /// match those values. With the order reversed, `"wibble"` would route through
32783278+ /// the existing `"wib"` choice and need no choice of its own.
32793279+ needs_new_choice: bool,
32803280+}
32813281+32823282+fn overlap_candidates<'a>(
32833283+ trie: &'a Trie<String, usize>,
32843284+ key: &str,
32853285+) -> impl Iterator<Item = (&'a str, usize)> {
31393286 trie.get_ancestor(key)
31403287 .into_iter()
31413141- .flat_map(|ancestor| ancestor.values().copied())
32883288+ .flat_map(|ancestor| ancestor.iter())
32893289+ .map(|(prefix, index)| (prefix.as_str(), *index))
32903290+}
32913291+32923292+fn descendant_candidates<'a>(
32933293+ trie: &'a Trie<String, usize>,
32943294+ key: &str,
32953295+) -> impl Iterator<Item = (&'a str, usize)> {
32963296+ trie.get_raw_descendant(key)
32973297+ .into_iter()
32983298+ .flat_map(|descendant| descendant.iter())
32993299+ .map(|(prefix, index)| (prefix.as_str(), *index))
31423300}
3143330131443302#[derive(Debug)]
···33183476 checks.push(var.is(pattern))
33193477 }
3320347833213321- let has_guard = branch.guard.is_some();
33223322- let branch = Branch::new(self.number_of_clauses, alternative_index, checks, has_guard);
34793479+ let guard = branch.guard.as_ref().map(|guard| {
34803480+ let referenced = guard.referenced_variables();
34813481+ let uses_pattern_bindings = patterns.iter().any(|pattern| {
34823482+ pattern
34833483+ .bound_variables()
34843484+ .iter()
34853485+ .any(|bound| referenced.contains(&bound.name()))
34863486+ });
34873487+34883488+ BranchGuard {
34893489+ clause_index: self.number_of_clauses,
34903490+ uses_pattern_bindings,
34913491+ }
34923492+ });
34933493+34943494+ let branch = Branch::new(self.number_of_clauses, alternative_index, checks, guard);
33233495 self.branches.push(branch);
33243496 }
33253497···33383510 .subject_variables
33393511 .first()
33403512 .expect("wrong number of subject variables for pattern");
33413341- let branch = Branch::new(self.number_of_clauses, 0, vec![var.is(pattern)], false);
35133513+ let branch = Branch::new(self.number_of_clauses, 0, vec![var.is(pattern)], None);
33423514 self.number_of_clauses += 1;
33433515 self.branches.push(branch);
33443516 }