Skip to content

Commit

Permalink
refactor(semantic): store all data in PostTransformChecker in trans…
Browse files Browse the repository at this point in the history 
…form checker (#5050)

Pure refactor of transform checker. Store all scope data in `PostTransformChecker` so it doesn't need to be passed around. `SemanticData` contains a full set of all semantic data for semantic pass, so we have 2 instances of it for 1. after transform and 2. rebuilt semantic.
  • Loading branch information
@overlookmotel
overlookmotel committed Aug 21, 2024
1 parent 4e1f4ab commit ee7ac8b
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 132 deletions.
247 changes: 128 additions & 119 deletions crates/oxc_semantic/src/post_transform_checker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
//! vs from the fresh semantic analysis.
//!
//! We now have 2 sets of semantic data:
//! * "transformer": Semantic data from after the transformer has run
//! * "rebuild": Semantic data from the fresh semantic analysis
//! * "after transform": Semantic data from after the transformer has run
//! * "rebuilt": Semantic data from the fresh semantic analysis
//!
//! If the transformer has behaved correctly, the state of `ScopeTree` and `SymbolTable` should match
//! between "transformer" and "rebuild".
//! between "after transform" and "rebuilt".
//!
//! ## Complication
//!
Expand Down Expand Up @@ -78,14 +78,14 @@
//! in visitation order. We run `SemanticCollector` once on the AST coming out of the transformer,
//! and a 2nd time on the AST after the fresh semantic analysis.
//!
//! When we ZIP these lists together, we have pairs of `(transformer_id, rebuild_id)` which give the
//! When we ZIP these lists together, we have pairs of `(after_transform_id, rebuilt_id)` which give the
//! mapping between the 2 sets of IDs.
//!
//! ## Other notes
//!
//! See also: <https://github.com/oxc-project/oxc/issues/4790>
use std::{cell::Cell, mem};
use std::cell::Cell;

use oxc_allocator::{Allocator, CloneIn};
#[allow(clippy::wildcard_imports)]
Expand All @@ -100,70 +100,79 @@ use oxc_syntax::{

use crate::{ScopeTree, SemanticBuilder, SymbolTable};

#[derive(Default)]
pub struct PostTransformChecker {
errors: Vec<OxcDiagnostic>,
ids_transformer: SemanticIds,
/// Check `ScopeTree` and `SymbolTable` are correct after transform
pub fn check_semantic_after_transform(
symbols_after_transform: &SymbolTable,
scopes_after_transform: &ScopeTree,
program: &Program<'_>,
) -> Option<Vec<OxcDiagnostic>> {
// Collect `ScopeId`s, `SymbolId`s and `ReferenceId`s from AST after transformer
let mut ids_after_transform = SemanticIds::default();
if let Some(mut errors) = ids_after_transform.check(program) {
errors.insert(0, OxcDiagnostic::error("Semantic Collector failed after transform"));
return Some(errors);
}
let data_after_transform = SemanticData {
symbols: symbols_after_transform,
scopes: scopes_after_transform,
ids: ids_after_transform,
};

// Clone the post-transform AST, re-run semantic analysis on it, and collect `ScopeId`s,
// `SymbolId`s and `ReferenceId`s from AST.
// NB: `CloneIn` sets all `scope_id`, `symbol_id` and `reference_id` fields in AST to `None`,
// so the cloned AST will be "clean" of all semantic data, as if it had come fresh from the parser.
let allocator = Allocator::default();
let program = program.clone_in(&allocator);
let (symbols_rebuilt, scopes_rebuilt) = SemanticBuilder::new("", program.source_type)
.build(&program)
.semantic
.into_symbol_table_and_scope_tree();

let mut ids_rebuilt = SemanticIds::default();
if let Some(mut errors) = ids_rebuilt.check(&program) {
errors.insert(0, OxcDiagnostic::error("Semantic Collector failed after rebuild"));
return Some(errors);
}
let data_rebuilt =
SemanticData { symbols: &symbols_rebuilt, scopes: &scopes_rebuilt, ids: ids_rebuilt };

// Compare post-transform semantic data to semantic data from fresh semantic analysis
let mut checker = PostTransformChecker {
after_transform: data_after_transform,
rebuilt: data_rebuilt,
errors: vec![],
};
checker.check_bindings();
checker.check_symbols();
checker.check_references();

let errors = checker.errors;
(!errors.is_empty()).then_some(errors)
}

impl PostTransformChecker {
pub fn after_transform(
&mut self,
symbols_transformer: &SymbolTable,
scopes_transformer: &ScopeTree,
program: &Program<'_>,
) -> Option<Vec<OxcDiagnostic>> {
// Collect `ScopeId`s, `SymbolId`s and `ReferenceId`s from AST after transformer
self.ids_transformer = SemanticIds::default();
if let Some(errors) = self.ids_transformer.check(program) {
self.errors.push(OxcDiagnostic::error("Semantic Collector failed after transform"));
self.errors.extend(errors);
return Some(mem::take(&mut self.errors));
}

// Clone the post-transform AST, re-run semantic analysis on it, and collect `ScopeId`s,
// `SymbolId`s and `ReferenceId`s from AST.
// NB: `CloneIn` sets all `scope_id`, `symbol_id` and `reference_id` fields in AST to `None`,
// so the cloned AST will be "clean" of all semantic data, as if it had come fresh from the parser.
let allocator = Allocator::default();
let program = program.clone_in(&allocator);
let (symbols_rebuild, scopes_rebuild) = SemanticBuilder::new("", program.source_type)
.build(&program)
.semantic
.into_symbol_table_and_scope_tree();

let mut ids_rebuild = SemanticIds::default();
if let Some(errors) = ids_rebuild.check(&program) {
self.errors.push(OxcDiagnostic::error("Semantic Collector failed after rebuild"));
self.errors.extend(errors);
return Some(mem::take(&mut self.errors));
}

let errors_count = self.errors.len();

// Compare post-transform semantic data to semantic data from fresh semantic analysis
self.check_bindings(scopes_transformer, &scopes_rebuild, &ids_rebuild);

self.check_symbols(symbols_transformer, &symbols_rebuild, &scopes_rebuild, &ids_rebuild);
self.check_references(symbols_transformer, &symbols_rebuild, &ids_rebuild);
struct PostTransformChecker<'s> {
after_transform: SemanticData<'s>,
rebuilt: SemanticData<'s>,
errors: Vec<OxcDiagnostic>,
}

(errors_count != self.errors.len()).then(|| mem::take(&mut self.errors))
}
struct SemanticData<'s> {
symbols: &'s SymbolTable,
scopes: &'s ScopeTree,
ids: SemanticIds,
}

fn check_bindings(
&mut self,
scopes_transformer: &ScopeTree,
scopes_rebuild: &ScopeTree,
ids_rebuild: &SemanticIds,
) {
if self.ids_transformer.scope_ids.len() != ids_rebuild.scope_ids.len() {
impl<'s> PostTransformChecker<'s> {
fn check_bindings(&mut self) {
if self.after_transform.ids.scope_ids.len() != self.rebuilt.ids.scope_ids.len() {
self.errors.push(OxcDiagnostic::error("Scopes mismatch after transform"));
return;
}

// Check whether bindings are the same for scopes in the same visitation order.
for (&scope_id_transformer, &scope_id_rebuild) in
self.ids_transformer.scope_ids.iter().zip(ids_rebuild.scope_ids.iter())
for (&scope_id_after_transform, &scope_id_rebuilt) in
self.after_transform.ids.scope_ids.iter().zip(self.rebuilt.ids.scope_ids.iter())
{
fn get_sorted_bindings(scopes: &ScopeTree, scope_id: ScopeId) -> Vec<CompactStr> {
let mut bindings =
Expand All @@ -172,134 +181,134 @@ impl PostTransformChecker {
bindings
}

let (result_transformer, result_rebuild) =
match (scope_id_transformer, scope_id_rebuild) {
let (result_after_transform, result_rebuilt) =
match (scope_id_after_transform, scope_id_rebuilt) {
(None, None) => continue,
(Some(scope_id_transformer), Some(scope_id_rebuild)) => {
let bindings_transformer =
get_sorted_bindings(scopes_transformer, scope_id_transformer);
let bindings_rebuild =
get_sorted_bindings(scopes_rebuild, scope_id_rebuild);
if bindings_transformer == bindings_rebuild {
(Some(scope_id_after_transform), Some(scope_id_rebuilt)) => {
let bindings_after_transform = get_sorted_bindings(
self.after_transform.scopes,
scope_id_after_transform,
);
let bindings_rebuilt =
get_sorted_bindings(self.rebuilt.scopes, scope_id_rebuilt);
if bindings_after_transform == bindings_rebuilt {
continue;
}
(
format!("{scope_id_transformer:?}: {bindings_transformer:?}"),
format!("{scope_id_rebuild:?}: {bindings_rebuild:?}"),
format!("{scope_id_after_transform:?}: {bindings_after_transform:?}"),
format!("{scope_id_rebuilt:?}: {bindings_rebuilt:?}"),
)
}
(Some(scope_id_transformer), None) => {
let bindings_transformer =
get_sorted_bindings(scopes_transformer, scope_id_transformer);
(Some(scope_id_after_transform), None) => {
let bindings_after_transform = get_sorted_bindings(
self.after_transform.scopes,
scope_id_after_transform,
);
(
format!("{scope_id_transformer:?}: {bindings_transformer:?}"),
format!("{scope_id_after_transform:?}: {bindings_after_transform:?}"),
"No scope".to_string(),
)
}
(None, Some(scope_id_rebuild)) => {
let bindings_rebuild =
get_sorted_bindings(scopes_rebuild, scope_id_rebuild);
(None, Some(scope_id_rebuilt)) => {
let bindings_rebuilt =
get_sorted_bindings(self.rebuilt.scopes, scope_id_rebuilt);
(
"No scope".to_string(),
format!("{scope_id_rebuild:?}: {bindings_rebuild:?}"),
format!("{scope_id_rebuilt:?}: {bindings_rebuilt:?}"),
)
}
};

let message = format!(
"
Bindings mismatch:
after transform: {result_transformer}
rebuilt : {result_rebuild}
after transform: {result_after_transform}
rebuilt : {result_rebuilt}
"
);
self.errors.push(OxcDiagnostic::error(message.trim().to_string()));
}
}

fn check_symbols(
&mut self,
symbols_transformer: &SymbolTable,
symbols_rebuild: &SymbolTable,
scopes_rebuild: &ScopeTree,
ids_rebuild: &SemanticIds,
) {
fn check_symbols(&mut self) {
// Check whether symbols are valid
for symbol_id in ids_rebuild.symbol_ids.iter().copied() {
if symbols_rebuild.get_flags(symbol_id).is_empty() {
let name = symbols_rebuild.get_name(symbol_id);
for symbol_id in self.rebuilt.ids.symbol_ids.iter().copied() {
if self.rebuilt.symbols.get_flags(symbol_id).is_empty() {
let name = self.rebuilt.symbols.get_name(symbol_id);
self.errors.push(OxcDiagnostic::error(format!(
"Expect non-empty SymbolFlags for BindingIdentifier({name})"
)));
if !scopes_rebuild.has_binding(symbols_rebuild.get_scope_id(symbol_id), name) {
if !self
.rebuilt
.scopes
.has_binding(self.rebuilt.symbols.get_scope_id(symbol_id), name)
{
self.errors.push(OxcDiagnostic::error(
format!("Cannot find BindingIdentifier({name}) in the Scope corresponding to the Symbol"),
));
}
}
}

if self.ids_transformer.symbol_ids.len() != ids_rebuild.symbol_ids.len() {
if self.after_transform.ids.symbol_ids.len() != self.rebuilt.ids.symbol_ids.len() {
self.errors.push(OxcDiagnostic::error("Symbols mismatch after transform"));
return;
}

// Check whether symbols match
for (symbol_id_transformer, symbol_id_rebuild) in
self.ids_transformer.symbol_ids.iter().zip(ids_rebuild.symbol_ids.iter())
for (&symbol_id_after_transform, &symbol_id_rebuilt) in
self.after_transform.ids.symbol_ids.iter().zip(self.rebuilt.ids.symbol_ids.iter())
{
let symbol_name_transformer = &symbols_transformer.names[*symbol_id_transformer];
let symbol_name_rebuild = &symbols_rebuild.names[*symbol_id_rebuild];
if symbol_name_transformer != symbol_name_rebuild {
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 {
let message = format!(
"
Symbol mismatch:
after transform: {symbol_id_transformer:?}: {symbol_name_transformer:?}
rebuilt : {symbol_id_rebuild:?}: {symbol_name_rebuild:?}
after transform: {symbol_id_after_transform:?}: {symbol_name_after_transform:?}
rebuilt : {symbol_id_rebuilt:?}: {symbol_name_rebuilt:?}
"
);
self.errors.push(OxcDiagnostic::error(message.trim().to_string()));
}
}
}

fn check_references(
&mut self,
symbols_transformer: &SymbolTable,
symbols_rebuild: &SymbolTable,
ids_rebuild: &SemanticIds,
) {
fn check_references(&mut self) {
// Check whether references are valid
for reference_id in ids_rebuild.reference_ids.iter().copied() {
let reference = symbols_rebuild.get_reference(reference_id);
for reference_id in self.rebuilt.ids.reference_ids.iter().copied() {
let reference = self.rebuilt.symbols.get_reference(reference_id);
if reference.flags().is_empty() {
self.errors.push(OxcDiagnostic::error(format!(
"Expect ReferenceFlags for IdentifierReference({reference_id:?}) to not be empty",
)));
}
}

if self.ids_transformer.reference_ids.len() != ids_rebuild.reference_ids.len() {
if self.after_transform.ids.reference_ids.len() != self.rebuilt.ids.reference_ids.len() {
self.errors.push(OxcDiagnostic::error("ReferenceId mismatch after transform"));
return;
}

// Check whether symbols match
for (reference_id_transformer, reference_id_rebuild) in
self.ids_transformer.reference_ids.iter().zip(ids_rebuild.reference_ids.iter())
for (&reference_id_after_transform, &reference_id_rebuilt) in
self.after_transform.ids.reference_ids.iter().zip(self.rebuilt.ids.reference_ids.iter())
{
let symbol_id_transformer =
symbols_transformer.references[*reference_id_transformer].symbol_id();
let symbol_name_transformer =
symbol_id_transformer.map(|id| symbols_transformer.names[id].clone());
let symbol_id_rebuild = &symbols_rebuild.references[*reference_id_rebuild].symbol_id();
let symbol_name_rebuild = symbol_id_rebuild.map(|id| symbols_rebuild.names[id].clone());
if symbol_name_transformer != symbol_name_rebuild {
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 {
let message = format!(
"
Reference mismatch:
after transform: {reference_id_transformer:?}: {symbol_name_transformer:?}
rebuilt : {reference_id_rebuild:?}: {symbol_name_rebuild:?}
after transform: {reference_id_after_transform:?}: {symbol_name_after_transform:?}
rebuilt : {reference_id_rebuilt:?}: {symbol_name_rebuilt:?}
"
);
self.errors.push(OxcDiagnostic::error(message.trim().to_string()));
Expand Down
6 changes: 2 additions & 4 deletions tasks/coverage/src/driver.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use oxc::diagnostics::OxcDiagnostic;
use oxc::minifier::CompressOptions;
use oxc::parser::{ParseOptions, ParserReturn};
use oxc::semantic::{
post_transform_checker::{PostTransformChecker, SemanticIds},
post_transform_checker::{check_semantic_after_transform, SemanticIds},
SemanticBuilderReturn,
};
use oxc::span::{SourceType, Span};
Expand All @@ -32,8 +32,6 @@ pub struct Driver {
pub panicked: bool,
pub errors: Vec<OxcDiagnostic>,
pub printed: String,
// states
pub checker: PostTransformChecker,
}

impl CompilerInterface for Driver {
Expand Down Expand Up @@ -93,7 +91,7 @@ impl CompilerInterface for Driver {
transformer_return: &mut TransformerReturn,
) -> ControlFlow<()> {
if self.check_semantic {
if let Some(errors) = self.checker.after_transform(
if let Some(errors) = check_semantic_after_transform(
&transformer_return.symbols,
&transformer_return.scopes,
program,
Expand Down
Loading

0 comments on commit ee7ac8b

Please sign in to comment.