Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions crates/rule-engine/src/rule/deserialize_env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ fn into_map<L: Language>(
.collect()
}

type OrderResult<T> = Result<T, String>;
type OrderResult<T> = Result<T, ReferentRuleError>;

/// A struct to store information to deserialize rules.
#[derive(Clone, Debug)]
Expand Down Expand Up @@ -79,22 +79,24 @@ struct TopologicalSort<'a, T: DependentRule> {
order: Vec<&'a str>,
// bool stands for if the rule has completed visit
seen: RapidMap<&'a str, bool>,
env: Option<&'a RuleRegistration>,
}

impl<'a, T: DependentRule> TopologicalSort<'a, T> {
fn get_order(maps: &RapidMap<String, T>) -> OrderResult<Vec<&str>> {
let mut top_sort = TopologicalSort::new(maps);
fn get_order(maps: &'a RapidMap<String, T>, env: Option<&'a RuleRegistration>) -> OrderResult<Vec<&'a str>> {
let mut top_sort = TopologicalSort::new(maps, env);
for key in maps.keys() {
top_sort.visit(key)?;
}
Ok(top_sort.order)
}

fn new(maps: &'a RapidMap<String, T>) -> Self {
fn new(maps: &'a RapidMap<String, T>, env: Option<&'a RuleRegistration>) -> Self {
Self {
maps,
order: vec![],
seen: RapidMap::default(),
env,
}
}

Expand All @@ -105,15 +107,20 @@ impl<'a, T: DependentRule> TopologicalSort<'a, T> {
return if completed {
Ok(())
} else {
Err(key.to_string())
Err(ReferentRuleError::CyclicRule(key.to_string()))
};
}
let Some(item) = self.maps.get(key) else {
// key can be found elsewhere
// e.g. if key is rule_id
// if rule_id not found in global, it can be a local rule
// if rule_id not found in local, it can be a global rule
// TODO: add check here and return Err if rule not found
if let Some(env) = self.env {
// Note: We only check if the key is completely missing
if !env.contains_rule(key) {
return Err(ReferentRuleError::UndefinedUtil(key.to_string()));
}
Comment on lines +121 to +125
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new presence check only consults contains_match_rule (local/global match rules). If the intent is to also error on missing rewriter references during deserialization (as described in the PR), this logic doesn’t cover rewriters, and missing rewriters can still be silently ignored elsewhere. Consider extending the validation to cover the rewriter scope as well (or clarifying the PR scope if rewriters are intentionally out of band).

Copilot uses AI. Check for mistakes.
}
return Ok(());
Comment on lines 116 to 127
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TopologicalSort::visit now returns UndefinedUtil when a dependency key is missing from maps and also absent from the env registration. There isn't a unit test in this module that asserts with_utils fails with RuleSerializeError::MatchesReference(UndefinedUtil(_)) for an undefined local util dependency (e.g. a: { matches: missing }). Adding such a test would lock in the new behavior and prevent regressions back to silent ignores.

Copilot uses AI. Check for mistakes.
};
// mark the id as seen but not completed
Expand Down Expand Up @@ -165,8 +172,7 @@ impl<L: Language> DeserializeEnv<L> {
self,
utils: &RapidMap<String, SerializableRule>,
) -> Result<Self, RuleSerializeError> {
let order = TopologicalSort::get_order(utils)
.map_err(ReferentRuleError::CyclicRule)
let order = TopologicalSort::get_order(utils, Some(&self.registration))
.map_err(RuleSerializeError::MatchesReference)?;
for id in order {
let rule = utils.get(id).expect("must exist");
Expand All @@ -182,8 +188,7 @@ impl<L: Language> DeserializeEnv<L> {
) -> Result<GlobalRules, RuleCoreError> {
let registration = GlobalRules::default();
let utils = into_map(utils);
let order = TopologicalSort::get_order(&utils)
.map_err(ReferentRuleError::CyclicRule)
let order = TopologicalSort::get_order(&utils, None)
.map_err(RuleSerializeError::from)?;
Comment on lines -189 to 196
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse_global_utils calls TopologicalSort::get_order(&utils, None), which means a matches: some-missing-global inside a global util rule will still be treated as an external dependency and won’t error during ordering. Because global rules are parsed with CheckHint::Global (which skips verify_util), this can leave undefined global dependencies undetected and cause silent non-matches at runtime. Passing an env (e.g. a temporary RuleRegistration::from_globals(&registration)) into get_order or otherwise making missing dependencies error for global utils would align with the PR’s goal of failing on missing referenced rules.

Copilot uses AI. Check for mistakes.
for id in order {
let (lang, core) = utils.get(id).expect("must exist");
Expand All @@ -204,10 +209,11 @@ impl<L: Language> DeserializeEnv<L> {
}

pub(crate) fn get_transform_order<'a>(
&self,
&'a self,
trans: &'a RapidMap<String, Trans<MetaVariable>>,
) -> Result<Vec<&'a str>, String> {
TopologicalSort::get_order(trans)
) -> Result<Vec<&'a str>, ReferentRuleError> {
// Transformations don't need env rule registration checks, pass None
TopologicalSort::get_order(trans, None)
}

pub fn with_globals(self, globals: &GlobalRules) -> Self {
Expand Down Expand Up @@ -277,7 +283,11 @@ local-rule:
)
.expect("failed to parse utils");
// should not panic
DeserializeEnv::new(TypeScript::Tsx).with_utils(&utils)?;
let registration = GlobalRules::default();
let core: crate::rule_core::SerializableRuleCore = from_str("rule: {pattern: '123'}").unwrap();
let env_dummy = DeserializeEnv::new(TypeScript::Tsx).with_globals(&registration);
registration.insert("global-rule", core.get_matcher(env_dummy).unwrap()).unwrap();
DeserializeEnv::new(TypeScript::Tsx).with_globals(&registration).with_utils(&utils)?;
Ok(())
}

Expand Down
10 changes: 10 additions & 0 deletions crates/rule-engine/src/rule/referent_rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ impl<R> Registration<R> {
// it only insert new item to the RapidMap. It is safe to cast the raw ptr.
unsafe { &mut *(Arc::as_ptr(&self.0) as *mut RapidMap<String, R>) }
}

pub(crate) fn contains_key(&self, id: &str) -> bool {
self.0.contains_key(id)
}
}
pub type GlobalRules = Registration<RuleCore>;

Expand Down Expand Up @@ -83,6 +87,12 @@ impl RuleRegistration {
RegistrationRef { local, global }
}

pub(crate) fn contains_rule(&self, id: &str) -> bool {
self.local.contains_key(id)
|| self.global.contains_key(id)
|| self.rewriters.contains_key(id)
}
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RuleRegistration::contains_rule treats rewriter IDs as valid dependencies. However, matches references are resolved only against local + global rules (ReferentRule never consults rewriters), so this can incorrectly allow a local util rule to depend on a rewriter ID and still be treated as “present”, reintroducing silent failures at runtime. Consider either removing rewriters from contains_rule, or splitting this into two APIs (e.g. contains_match_rule vs contains_rewriter) and using the match-specific one for deserialization dependency checks.

Copilot uses AI. Check for mistakes.

pub(crate) fn insert_local(&self, id: &str, rule: Rule) -> Result<(), ReferentRuleError> {
if rule.check_cyclic(id) {
return Err(ReferentRuleError::CyclicRule(id.into()));
Expand Down
3 changes: 2 additions & 1 deletion crates/rule-engine/src/transform/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ mod parse;
mod rewrite;
mod string_case;
mod trans;
use crate::rule::referent_rule::ReferentRuleError;

use crate::{DeserializeEnv, RuleCore};

Expand Down Expand Up @@ -72,7 +73,7 @@ impl Transform {
let map = map?;
let order = env
.get_transform_order(&map)
.map_err(TransformError::Cyclic)?;
.map_err(|e| match e { ReferentRuleError::CyclicRule(s) => TransformError::Cyclic(s), _ => TransformError::Cyclic(e.to_string()) })?;
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transform::deserialize maps any non-CyclicRule ReferentRuleError into TransformError::Cyclic(...). Today get_transform_order(..., None) can only emit CyclicRule, but this mapping will produce a misleading “cyclic dependency” error message if additional ReferentRuleError variants ever become possible here. Consider matching only CyclicRule (and treating other variants as unreachable) or introducing a dedicated TransformError variant for non-cyclic dependency errors.

Copilot uses AI. Check for mistakes.
let transforms = order
.iter()
.map(|&key| (key.to_string(), map[key].clone()))
Expand Down
Loading