-
Notifications
You must be signed in to change notification settings - Fork 0
fix: Return explicit error when dependent rule is not found during deserialization #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
37e86fc
edc574e
3504b87
3620b09
a79e527
64f71ee
7e04537
71a2b4e
37e8317
d8783ba
c148107
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,24 +29,15 @@ extend-ignore-identifiers-re = [ | |
| "RET", | ||
| "prev", | ||
| "normalises", | ||
| "goes", | ||
| "Bare", | ||
| "inout", | ||
| "ba", | ||
| "ede", | ||
| "goes", | ||
| ] | ||
|
Comment on lines
-15
to
33
|
||
|
|
||
| [default.extend-words] | ||
| Bare = "Bare" | ||
| Supress = "Supress" | ||
| teh = "teh" | ||
| Teh = "Teh" | ||
|
|
||
| [files] | ||
| ignore-hidden = false | ||
| ignore-files = true | ||
| extend-exclude = [ | ||
| "CHANGELOG.md", | ||
| "./CHANGELOG.md", | ||
| "/usr/**/*", | ||
| "/tmp/**/*", | ||
|
Comment on lines
38
to
41
|
||
| "/**/node_modules/**", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,11 +67,8 @@ pub trait Language: Clone + std::fmt::Debug + Send + Sync + 'static { | |
| fn extract_meta_var(&self, source: &str) -> Option<MetaVariable> { | ||
| extract_meta_var(source, self.expando_char()) | ||
| } | ||
| /// Return the file language inferred from a filesystem path. | ||
| /// | ||
| /// The *default* implementation is not implemented and will panic if called. | ||
| /// Implementors should override this method and return `Some(Self)` when the | ||
| /// file type is supported and `None` when it is not. | ||
| /// Return the file language from path. Return None if the file type is not supported. | ||
| /// Will panic with an unimplimented error if called and not implemented | ||
| fn from_path<P: AsRef<Path>>(_path: P) -> Option<Self> { | ||
| unimplemented!( | ||
| "Language::from_path is not implemented for type `{}`. \ | ||
|
|
@@ -94,26 +91,12 @@ mod test { | |
| use super::*; | ||
| use crate::tree_sitter::{LanguageExt, StrDoc, TSLanguage}; | ||
|
|
||
| // Shared helpers for test Language impls backed by tree-sitter-typescript. | ||
| fn tsx_kind_to_id(kind: &str) -> u16 { | ||
| let ts_lang: TSLanguage = tree_sitter_typescript::LANGUAGE_TSX.into(); | ||
| ts_lang.id_for_node_kind(kind, /* named */ true) | ||
| } | ||
|
|
||
| fn tsx_field_to_id(field: &str) -> Option<u16> { | ||
| let ts_lang: TSLanguage = tree_sitter_typescript::LANGUAGE_TSX.into(); | ||
| ts_lang.field_id_for_name(field).map(|f| f.get()) | ||
| } | ||
|
|
||
| fn tsx_ts_language() -> TSLanguage { | ||
| tree_sitter_typescript::LANGUAGE_TSX.into() | ||
| } | ||
|
|
||
| #[derive(Clone, Debug)] | ||
| pub struct Tsx; | ||
| impl Language for Tsx { | ||
| fn kind_to_id(&self, kind: &str) -> u16 { | ||
| tsx_kind_to_id(kind) | ||
| let ts_lang: TSLanguage = tree_sitter_typescript::LANGUAGE_TSX.into(); | ||
| ts_lang.id_for_node_kind(kind, /* named */ true) | ||
| } | ||
| fn field_to_id(&self, field: &str) -> Option<u16> { | ||
| self.get_ts_language() | ||
|
|
@@ -126,70 +109,7 @@ mod test { | |
| } | ||
| impl LanguageExt for Tsx { | ||
| fn get_ts_language(&self) -> TSLanguage { | ||
| tsx_ts_language() | ||
| } | ||
| } | ||
|
|
||
| /// A minimal `Language` impl that does *not* override `from_path`, used to | ||
| /// verify that the default implementation panics. | ||
| #[derive(Clone, Debug)] | ||
| struct NoFromPath; | ||
| impl Language for NoFromPath { | ||
| fn kind_to_id(&self, kind: &str) -> u16 { | ||
| tsx_kind_to_id(kind) | ||
| } | ||
| fn field_to_id(&self, field: &str) -> Option<u16> { | ||
| tsx_field_to_id(field) | ||
| } | ||
| #[cfg(feature = "matching")] | ||
| fn build_pattern(&self, builder: &PatternBuilder) -> Result<Pattern, PatternError> { | ||
| builder.build(|src| StrDoc::try_new(src, self.clone())) | ||
| } | ||
| } | ||
| impl LanguageExt for NoFromPath { | ||
| fn get_ts_language(&self) -> TSLanguage { | ||
| tsx_ts_language() | ||
| tree_sitter_typescript::LANGUAGE_TSX.into() | ||
| } | ||
| } | ||
|
|
||
| /// A `Language` impl that *does* override `from_path`, used to verify that | ||
| /// overriding the default works correctly. | ||
| #[derive(Clone, Debug)] | ||
| struct TsxWithFromPath; | ||
| impl Language for TsxWithFromPath { | ||
| fn kind_to_id(&self, kind: &str) -> u16 { | ||
| tsx_kind_to_id(kind) | ||
| } | ||
| fn field_to_id(&self, field: &str) -> Option<u16> { | ||
| tsx_field_to_id(field) | ||
| } | ||
| #[cfg(feature = "matching")] | ||
| fn build_pattern(&self, builder: &PatternBuilder) -> Result<Pattern, PatternError> { | ||
| builder.build(|src| StrDoc::try_new(src, self.clone())) | ||
| } | ||
| fn from_path<P: AsRef<Path>>(path: P) -> Option<Self> { | ||
| path.as_ref() | ||
| .extension() | ||
| .and_then(|e| e.to_str()) | ||
| .filter(|&e| e == "tsx") | ||
| .map(|_| Self) | ||
| } | ||
| } | ||
| impl LanguageExt for TsxWithFromPath { | ||
| fn get_ts_language(&self) -> TSLanguage { | ||
| tsx_ts_language() | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "Language::from_path is not implemented for type")] | ||
| fn default_from_path_panics() { | ||
| let _ = NoFromPath::from_path("some/file.rs"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn overridden_from_path_does_not_panic() { | ||
| assert!(TsxWithFromPath::from_path("component.tsx").is_some()); | ||
| assert!(TsxWithFromPath::from_path("main.rs").is_none()); | ||
| } | ||
| } | ||
|
Comment on lines
90
to
192
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)] | ||
|
|
@@ -79,22 +79,27 @@ 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, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -105,15 +110,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_match_rule(key) { | ||
| return Err(ReferentRuleError::UndefinedUtil(key.to_string())); | ||
| } | ||
|
Comment on lines
+121
to
+125
|
||
| } | ||
| return Ok(()); | ||
|
Comment on lines
116
to
127
|
||
| }; | ||
| // mark the id as seen but not completed | ||
|
|
@@ -165,8 +175,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"); | ||
|
|
@@ -182,8 +191,8 @@ 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 temp_env = RuleRegistration::from_globals(®istration); | ||
| let order = TopologicalSort::get_order(&utils, Some(&temp_env)) | ||
| .map_err(RuleSerializeError::from)?; | ||
|
Comment on lines
-189
to
196
|
||
| for id in order { | ||
| let (lang, core) = utils.get(id).expect("must exist"); | ||
|
|
@@ -204,10 +213,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 { | ||
|
|
@@ -277,7 +287,16 @@ 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(®istration); | ||
| registration | ||
| .insert("global-rule", core.get_matcher(env_dummy).unwrap()) | ||
| .unwrap(); | ||
| DeserializeEnv::new(TypeScript::Tsx) | ||
| .with_globals(®istration) | ||
| .with_utils(&utils)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ mod parse; | |
| mod rewrite; | ||
| mod string_case; | ||
| mod trans; | ||
| use crate::rule::referent_rule::ReferentRuleError; | ||
|
|
||
| use crate::{DeserializeEnv, RuleCore}; | ||
|
|
||
|
|
@@ -70,9 +71,12 @@ impl Transform { | |
| .map(|(key, val)| val.parse(&env.lang).map(|t| (key.to_string(), t))) | ||
| .collect(); | ||
| let map = map?; | ||
| let order = env | ||
| .get_transform_order(&map) | ||
| .map_err(TransformError::Cyclic)?; | ||
| let order = env.get_transform_order(&map).map_err(|e| match e { | ||
| ReferentRuleError::CyclicRule(s) => TransformError::Cyclic(s), | ||
| _ => unreachable!( | ||
| "get_transform_order uses None for env, so only CyclicRule is possible" | ||
| ), | ||
| })?; | ||
|
Comment on lines
+74
to
+79
|
||
| let transforms = order | ||
| .iter() | ||
| .map(|&key| (key.to_string(), map[key].clone())) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -551,6 +551,7 @@ if (true) { | |||||||||||||||||||
| Ok(()) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // TODO: add a symbolic test for Rewrite | ||||||||||||||||||||
|
||||||||||||||||||||
| // TODO: add a symbolic test for Rewrite | |
| #[test] | |
| fn test_rewrite_is_send_sync() -> R { | |
| fn assert_send_sync<T: Send + Sync>() {} | |
| assert_send_sync::<Rewrite>(); | |
| Ok(()) | |
| } | |
| // TODO: consider adding more behavioral tests for Rewrite parsing and execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_typos.tomlremoved the identifier ignore forinout, but this PR also introduces"inout"intoclassifications/_universal_rules.json. Iftyposdoesn't treatinoutas a valid word/identifier, CI will start failing. Consider keepinginoutinextend-ignore-identifiers-reor adding it to allowed words to match the repository's actual vocabulary.