From 7156fd2f9124767e62687ae7b2ebc917230650e4 Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Thu, 22 Aug 2024 02:34:42 +0000 Subject: [PATCH] refactor(semantic): transform checker `Pair` structure (#5053) Introduce `Pair` structure containing a pair values from the "after transform" and "rebuilt" semantics, to reduce repetitive code. --- .../src/post_transform_checker.rs | 268 +++++++++++------- 1 file changed, 173 insertions(+), 95 deletions(-) diff --git a/crates/oxc_semantic/src/post_transform_checker.rs b/crates/oxc_semantic/src/post_transform_checker.rs index 71d0b2b17d8e9..e1cfbea299594 100644 --- a/crates/oxc_semantic/src/post_transform_checker.rs +++ b/crates/oxc_semantic/src/post_transform_checker.rs @@ -85,7 +85,7 @@ //! //! See also: -use std::cell::Cell; +use std::{cell::Cell, fmt::Debug}; use oxc_allocator::{Allocator, CloneIn}; #[allow(clippy::wildcard_imports)] @@ -162,14 +162,99 @@ struct SemanticData<'s> { ids: SemanticIds, } +/// Pair of values from after transform and rebuilt +struct Pair { + after_transform: T, + rebuilt: T, +} + +impl Pair { + fn new(after_transform: T, rebuilt: T) -> Self { + Self { after_transform, rebuilt } + } + + fn from_tuple(values: (T, T)) -> Self { + Self::new(values.0, values.1) + } + + fn parts(&self) -> (&T, &T) { + (&self.after_transform, &self.rebuilt) + } + + fn into_parts(self) -> (T, T) { + (self.after_transform, self.rebuilt) + } +} + +impl Pair { + fn is_match(&self) -> bool { + self.after_transform == self.rebuilt + } + + fn is_mismatch(&self) -> bool { + !self.is_match() + } +} + +impl AsRef> for Pair { + fn as_ref(&self) -> &Self { + self + } +} + +#[allow(clippy::expl_impl_clone_on_copy)] +impl Clone for Pair { + fn clone(&self) -> Self { + Self::new(self.after_transform.clone(), self.rebuilt.clone()) + } +} + +impl Copy for Pair {} + +/// Errors collection #[derive(Default)] struct Errors(Vec); impl Errors { + /// Add an error string fn push>(&mut self, message: S) { self.0.push(OxcDiagnostic::error(message.as_ref().trim().to_string())); } + /// Add an error for a mismatch between a pair of values, with IDs + fn push_mismatch(&mut self, title: &str, ids: Ids, values: Values) + where + Id: Debug, + Value: Debug, + Ids: AsRef>, + Values: AsRef>, + { + let (id_after_transform, id_rebuilt) = ids.as_ref().parts(); + let (value_after_transform, value_rebuilt) = values.as_ref().parts(); + let str_after_transform = format!("{id_after_transform:?}: {value_after_transform:?}"); + let str_rebuilt = format!("{id_rebuilt:?}: {value_rebuilt:?}"); + self.push_mismatch_strs(title, Pair::new(str_after_transform, str_rebuilt)); + } + + /// Add an error for a mismatch between a pair of values + fn push_mismatch_strs(&mut self, title: &str, values: Values) + where + Value: AsRef, + Values: AsRef>, + { + let (value_after_transform, value_rebuilt) = values.as_ref().parts(); + let value_after_transform = value_after_transform.as_ref(); + let value_rebuilt = value_rebuilt.as_ref(); + self.push(format!( + " +{title}: +after transform: {value_after_transform} +rebuilt : {value_rebuilt} + " + )); + } + + /// Get errors fn get(self) -> Option> { if self.0.is_empty() { None @@ -181,81 +266,60 @@ impl Errors { impl<'s> PostTransformChecker<'s> { fn check_scopes(&mut self) { - if self.after_transform.ids.scope_ids.len() != self.rebuilt.ids.scope_ids.len() { + if self.get_static_pair(|data| data.ids.scope_ids.len()).is_mismatch() { self.errors.push("Scopes mismatch after transform"); } - let PostTransformChecker { after_transform, rebuilt, .. } = self; - for (&scope_id_after_transform, &scope_id_rebuilt) in - after_transform.ids.scope_ids.iter().zip(rebuilt.ids.scope_ids.iter()) + for scope_ids in self + .after_transform + .ids + .scope_ids + .iter() + .copied() + .zip(self.rebuilt.ids.scope_ids.iter().copied()) + .map(Pair::from_tuple) { // Check bindings are the same - fn get_sorted_bindings(scopes: &ScopeTree, scope_id: ScopeId) -> Vec { + fn get_sorted_bindings(data: &SemanticData, scope_id: ScopeId) -> Vec { let mut bindings = - scopes.get_bindings(scope_id).keys().cloned().collect::>(); + data.scopes.get_bindings(scope_id).keys().cloned().collect::>(); bindings.sort_unstable(); bindings } - let (result_after_transform, result_rebuilt) = - match (scope_id_after_transform, scope_id_rebuilt) { - (None, None) => continue, - (Some(scope_id_after_transform), Some(scope_id_rebuilt)) => { - let bindings_after_transform = - get_sorted_bindings(after_transform.scopes, scope_id_after_transform); - let bindings_rebuilt = - get_sorted_bindings(rebuilt.scopes, scope_id_rebuilt); - if bindings_after_transform == bindings_rebuilt { - continue; - } - ( - format!("{scope_id_after_transform:?}: {bindings_after_transform:?}"), - format!("{scope_id_rebuilt:?}: {bindings_rebuilt:?}"), - ) + let scope_ids = match scope_ids.into_parts() { + (None, None) => continue, + (Some(scope_id_after_transform), Some(scope_id_rebuilt)) => { + let scope_ids = Pair::new(scope_id_after_transform, scope_id_rebuilt); + let bindings = self.get_pair(scope_ids, get_sorted_bindings); + if bindings.is_match() { + continue; } - (Some(scope_id_after_transform), None) => { - let bindings_after_transform = - get_sorted_bindings(after_transform.scopes, scope_id_after_transform); - ( - format!("{scope_id_after_transform:?}: {bindings_after_transform:?}"), - "No scope".to_string(), - ) - } - (None, Some(scope_id_rebuilt)) => { - let bindings_rebuilt = - get_sorted_bindings(rebuilt.scopes, scope_id_rebuilt); - ( - "No scope".to_string(), - format!("{scope_id_rebuilt:?}: {bindings_rebuilt:?}"), - ) - } - }; - - self.errors.push(format!( - " -Bindings mismatch: -after transform: {result_after_transform} -rebuilt : {result_rebuilt} - " - )); - - let (Some(scope_id_after_transform), Some(scope_id_rebuilt)) = - (scope_id_after_transform, scope_id_rebuilt) - else { - continue; + self.errors.push_mismatch("Bindings mismatch", scope_ids, bindings); + scope_ids + } + (Some(scope_id), None) => { + let bindings = get_sorted_bindings(&self.after_transform, scope_id); + self.errors.push_mismatch_strs( + "Bindings mismatch", + Pair::new(format!("{scope_id:?}: {bindings:?}").as_str(), "No scope"), + ); + continue; + } + (None, Some(scope_id)) => { + let bindings = get_sorted_bindings(&self.rebuilt, scope_id); + self.errors.push_mismatch_strs( + "Bindings mismatch", + Pair::new("No scope", format!("{scope_id:?}: {bindings:?}").as_str()), + ); + continue; + } }; // Check flags match - let flags_after_transform = after_transform.scopes.get_flags(scope_id_after_transform); - let flags_rebuilt = rebuilt.scopes.get_flags(scope_id_rebuilt); - if flags_after_transform != flags_rebuilt { - self.errors.push(format!( - " -Scope flags mismatch: -after transform: {scope_id_after_transform:?}: {flags_after_transform:?} -rebuilt : {scope_id_rebuilt:?}: {flags_rebuilt:?} - " - )); + let flags = self.get_pair(scope_ids, |data, scope_id| data.scopes.get_flags(scope_id)); + if flags.is_mismatch() { + self.errors.push_mismatch("Scope flags mismatch", scope_ids, flags); } } } @@ -279,26 +343,25 @@ rebuilt : {scope_id_rebuilt:?}: {flags_rebuilt:?} } } - if self.after_transform.ids.symbol_ids.len() != self.rebuilt.ids.symbol_ids.len() { + if self.get_static_pair(|data| data.ids.symbol_ids.len()).is_mismatch() { self.errors.push("Symbols mismatch after transform"); return; } // Check whether symbols match - for (&symbol_id_after_transform, &symbol_id_rebuilt) in - self.after_transform.ids.symbol_ids.iter().zip(self.rebuilt.ids.symbol_ids.iter()) + for symbol_ids in self + .after_transform + .ids + .symbol_ids + .iter() + .copied() + .zip(self.rebuilt.ids.symbol_ids.iter().copied()) + .map(Pair::from_tuple) { - let symbol_name_after_transform = - &self.after_transform.symbols.names[symbol_id_after_transform]; - let symbol_name_rebuilt = &self.rebuilt.symbols.names[symbol_id_rebuilt]; - if symbol_name_after_transform != symbol_name_rebuilt { - self.errors.push(format!( - " -Symbol mismatch: -after transform: {symbol_id_after_transform:?}: {symbol_name_after_transform:?} -rebuilt : {symbol_id_rebuilt:?}: {symbol_name_rebuilt:?} - " - )); + let symbol_names = + self.get_pair(symbol_ids, |data, symbol_id| data.symbols.names[symbol_id].clone()); + if symbol_names.is_mismatch() { + self.errors.push_mismatch("Symbol mismatch", symbol_ids, symbol_names); } } } @@ -314,34 +377,49 @@ rebuilt : {symbol_id_rebuilt:?}: {symbol_name_rebuilt:?} } } - if self.after_transform.ids.reference_ids.len() != self.rebuilt.ids.reference_ids.len() { + if self.get_static_pair(|data| data.ids.reference_ids.len()).is_mismatch() { self.errors.push("ReferenceId mismatch after transform"); return; } // Check whether symbols match - for (&reference_id_after_transform, &reference_id_rebuilt) in - self.after_transform.ids.reference_ids.iter().zip(self.rebuilt.ids.reference_ids.iter()) + for reference_ids in self + .after_transform + .ids + .reference_ids + .iter() + .copied() + .zip(self.rebuilt.ids.reference_ids.iter().copied()) + .map(Pair::from_tuple) { - let symbol_id_after_transform = - self.after_transform.symbols.references[reference_id_after_transform].symbol_id(); - let symbol_name_after_transform = - symbol_id_after_transform.map(|id| self.after_transform.symbols.names[id].clone()); - let symbol_id_rebuilt = - &self.rebuilt.symbols.references[reference_id_rebuilt].symbol_id(); - let symbol_name_rebuilt = - symbol_id_rebuilt.map(|id| self.rebuilt.symbols.names[id].clone()); - if symbol_name_after_transform != symbol_name_rebuilt { - self.errors.push(format!( - " -Reference mismatch: -after transform: {reference_id_after_transform:?}: {symbol_name_after_transform:?} -rebuilt : {reference_id_rebuilt:?}: {symbol_name_rebuilt:?} - " - )); + let symbol_ids = self.get_pair(reference_ids, |data, reference_id| { + data.symbols.references[reference_id].symbol_id() + }); + let symbol_names = self.get_pair(symbol_ids, |data, symbol_id| { + symbol_id.map(|symbol_id| data.symbols.names[symbol_id].clone()) + }); + if symbol_names.is_mismatch() { + self.errors.push_mismatch("Reference mismatch", reference_ids, symbol_names); } } } + + /// Get same data from `after_transform` and `rebuilt` from a pair of IDs + fn get_pair R>( + &self, + ids: Pair, + getter: F, + ) -> Pair { + Pair::new( + getter(&self.after_transform, ids.after_transform), + getter(&self.rebuilt, ids.rebuilt), + ) + } + + /// Get same data from `after_transform` and `rebuilt` + fn get_static_pair R>(&self, getter: F) -> Pair { + Pair::new(getter(&self.after_transform), getter(&self.rebuilt)) + } } /// Collection of `ScopeId`s, `SymbolId`s and `ReferenceId`s from an AST.