-
-
Notifications
You must be signed in to change notification settings - Fork 496
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
test(semantic): add comprehensive regression test suite (#5976)
# What This PR Does Enhance's `oxc_semantic`'s integration tests with a regression test suite that ensures semantic's contract guarantees hold over all test cases in typescript-eslint's scope snapshot tests. Each test case checks a separate assumption and runs independently from other test cases. This PR sets up the code infrastructure for this test suite and adds two test cases to start us off: 1. Reflexivity tests for `IdentifierReference` and `Reference` 2. Symbol declaration reflexivity tests between declarations in `SymbolTable` and their corresponding node in the AST. Please refer to the doc comments for each of these tests for an in-depth explanation. ## Aren't our existing tests sufficient? `oxc_semantic` is currently tested directly via 1. scope snapshot tests, ported from `typescript-eslint` 2. Hand-written tests using `SemanticTester` in `tests/integration` And indirectly via 3. Conformance test suite over Test262/TypeScript/Babel 4. Linter snapshot tests Shouldn't this be sufficient? I argue not, for two reasons: ## 1. Clarify Contract Ambiguity When using `Semantic`, I often find myself asking these questions? * Does `semantic.symbols().get_declaration(id)` point to a `BindingIdentifer`/`BindingPattern` or the declaration that holds an identifier/pattern? * Will a `Reference`'s `node_id` point me to an `IdentifierReference` or the expression/statement that is holding an `IdentifierReference`? * When will `BindingIdentifier`'s `symbol_id` get populated? can we guarantee that after semantic analysis it will never be `None`? * What actually _is_ the node covered by `semantic.symbols().get_span(id)`? This one really messed me up, and resulted in me creating #4739. * What scope does `Function::scope_id` point to? The one where the function is declared? The one created by its body? The one created by the type annotations but before the function body? Or something else entirely? **These test cases are meant to answer such questions and guarnatee those answers as test cases**. No other existing testing solution currently upholds such promises: they only tell us if code expecting one answer or another produces an unexpected result. However, those parts of the codebase could always be adjusted to conform to new `Semantic` behavior, meaning no contract guarantees are actually upheld. ## 2. Existing Tests Do Not Test The Same Behavior I'll cover each above listed test case one-by-one: 1. For starters, these tests only cover scopes. Additionally, they only tell us **how behavior has changed**, not that **behavior is now incorrect**. 2. These _do_ generally cover the same behaviors, but **are not comprehensive and are difficult to maintain**. These are unit tests that should be used hand-in-hand with this new test suite. 3. The most relevant tests here are for the parser. However, these tests **only tell us if a syntax/parse error was produced**, and tell us nothing about the validity of `Semantic`. 4. Relying on lint rule's output is a a mediiocre proxy of `Semantic`'s behavior at best. They can tell us if changes to `Semantic` break assumptions made by lint rules, but they do not tell us if **those assumptions are the ones we want to uphold to external crates consuming `Semantic`.
- Loading branch information
Showing
292 changed files
with
3,307 additions
and
6 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,165 @@ | ||
//! Conformance tests. | ||
//! | ||
//! Since these cases are a contract-as-code, they _must be well documented_. When adding a new | ||
//! test, please describe what behavior it guarantees in as plain language as possible. | ||
use crate::TestContext; | ||
use std::{borrow::Cow, sync::Arc}; | ||
|
||
use oxc_diagnostics::{GraphicalReportHandler, GraphicalTheme, NamedSource, OxcDiagnostic}; | ||
use oxc_semantic::{AstNode, Semantic, SymbolId}; | ||
|
||
mod test_identifier_reference; | ||
mod test_symbol_declaration; | ||
|
||
pub fn conformance_suite() -> SemanticConformance { | ||
SemanticConformance::default() | ||
.with_test(test_symbol_declaration::SymbolDeclarationTest) | ||
.with_test(test_identifier_reference::IdentifierReferenceTest) | ||
} | ||
|
||
pub trait ConformanceTest { | ||
fn name(&self) -> &'static str; | ||
|
||
#[must_use] | ||
#[allow(dead_code, unused_variables)] | ||
fn run_once(&self, semantic: &Semantic<'_>) -> TestResult { | ||
TestResult::Pass | ||
} | ||
|
||
#[must_use] | ||
#[allow(unused_variables)] | ||
fn run_on_node<'a>(&self, node: &AstNode<'a>, semantic: &Semantic<'a>) -> TestResult { | ||
TestResult::Pass | ||
} | ||
|
||
#[must_use] | ||
#[allow(unused_variables)] | ||
fn run_on_symbol(&self, symbol_id: SymbolId, semantic: &Semantic<'_>) -> TestResult { | ||
TestResult::Pass | ||
} | ||
} | ||
|
||
pub struct SemanticConformance { | ||
tests: Vec<Box<dyn ConformanceTest>>, | ||
reporter: GraphicalReportHandler, | ||
} | ||
|
||
impl Default for SemanticConformance { | ||
fn default() -> Self { | ||
Self { | ||
tests: Vec::new(), | ||
reporter: GraphicalReportHandler::default() | ||
.with_theme(GraphicalTheme::unicode_nocolor()), | ||
} | ||
} | ||
} | ||
|
||
impl SemanticConformance { | ||
/// Add a test case to the conformance suite. | ||
pub fn with_test<Test: ConformanceTest + 'static>(mut self, test: Test) -> Self { | ||
self.tests.push(Box::new(test)); | ||
self | ||
} | ||
|
||
pub fn run_on_source(&self, ctx: &TestContext<'_>) -> String { | ||
let named_source = Arc::new(NamedSource::new( | ||
ctx.path.to_string_lossy(), | ||
ctx.semantic.source_text().to_string(), | ||
)); | ||
|
||
let results = self | ||
.run(&ctx.semantic) | ||
.into_iter() | ||
.map(|diagnostic| diagnostic.with_source_code(Arc::clone(&named_source))) | ||
.collect::<Vec<_>>(); | ||
|
||
if results.is_empty() { | ||
return String::new(); | ||
} | ||
|
||
let mut output = String::new(); | ||
for result in results { | ||
self.reporter.render_report(&mut output, result.as_ref()).unwrap(); | ||
} | ||
|
||
output | ||
} | ||
|
||
fn run(&self, semantic: &Semantic) -> Vec<OxcDiagnostic> { | ||
let mut diagnostics = Vec::new(); | ||
for test in &self.tests { | ||
// Run file-level tests | ||
self.record_results(&mut diagnostics, test.as_ref(), test.run_once(semantic)); | ||
|
||
// Run AST node tests | ||
for node in semantic.nodes() { | ||
self.record_results( | ||
&mut diagnostics, | ||
test.as_ref(), | ||
test.run_on_node(node, semantic), | ||
); | ||
} | ||
|
||
// Run symbol tests | ||
for symbol_id in semantic.symbols().symbol_ids() { | ||
self.record_results( | ||
&mut diagnostics, | ||
test.as_ref(), | ||
test.run_on_symbol(symbol_id, semantic), | ||
); | ||
} | ||
} | ||
|
||
diagnostics | ||
} | ||
|
||
#[allow(clippy::unused_self)] | ||
fn record_results( | ||
&self, | ||
diagnostics: &mut Vec<OxcDiagnostic>, | ||
test: &dyn ConformanceTest, | ||
result: TestResult, | ||
) { | ||
if let TestResult::Fail(reasons) = result { | ||
diagnostics.extend( | ||
reasons.into_iter().map(|reason| reason.with_error_code_scope(test.name())), | ||
); | ||
} | ||
} | ||
} | ||
|
||
#[derive(Debug, Clone)] | ||
pub enum TestResult { | ||
Pass, | ||
Fail(/* reasons */ Vec<OxcDiagnostic>), | ||
} | ||
impl From<String> for TestResult { | ||
fn from(reason: String) -> Self { | ||
TestResult::Fail(vec![OxcDiagnostic::error(Cow::Owned(reason))]) | ||
} | ||
} | ||
impl From<Option<String>> for TestResult { | ||
fn from(result: Option<String>) -> Self { | ||
match result { | ||
Some(reason) => TestResult::Fail(vec![OxcDiagnostic::error(Cow::Owned(reason))]), | ||
None => TestResult::Pass, | ||
} | ||
} | ||
} | ||
|
||
impl From<OxcDiagnostic> for TestResult { | ||
fn from(diagnostic: OxcDiagnostic) -> Self { | ||
TestResult::Fail(vec![diagnostic]) | ||
} | ||
} | ||
impl From<Vec<OxcDiagnostic>> for TestResult { | ||
fn from(diagnostics: Vec<OxcDiagnostic>) -> Self { | ||
TestResult::Fail(diagnostics) | ||
} | ||
} | ||
impl FromIterator<OxcDiagnostic> for TestResult { | ||
fn from_iter<I: IntoIterator<Item = OxcDiagnostic>>(iter: I) -> Self { | ||
TestResult::Fail(iter.into_iter().collect()) | ||
} | ||
} |
77 changes: 77 additions & 0 deletions
77
crates/oxc_semantic/tests/conformance/test_identifier_reference.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
use oxc_ast::{ast::IdentifierReference, AstKind}; | ||
use oxc_diagnostics::OxcDiagnostic; | ||
use oxc_semantic::{NodeId, Reference}; | ||
use oxc_span::GetSpan; | ||
use oxc_syntax::reference::ReferenceId; | ||
|
||
use super::{ConformanceTest, TestResult}; | ||
use crate::Semantic; | ||
|
||
/// Tests reflexivity between [`IdentifierReference`] AST nodes and their corresponding | ||
/// [`Reference`]s. | ||
/// | ||
/// Performs the following checks: | ||
/// 1. All [`IdentifierReference`]s have been populated with a [`ReferenceId`], even if the | ||
/// referenced symbol could not be resolved. | ||
/// | ||
/// 2. When an [`IdentifierReference`] is used to find a [`Reference`] in the symbol table, the AST | ||
/// node id associated with that [`Reference`] should be the [`IdentifierReference`]'s AST node | ||
/// id. | ||
#[derive(Debug, Clone, Default)] | ||
pub struct IdentifierReferenceTest; | ||
|
||
/// [`IdentifierReference::reference_id`] returned [`None`]. | ||
fn missing_reference_id(reference: &IdentifierReference) -> TestResult { | ||
OxcDiagnostic::error("After semantic analysis, all IdentifierReferences should have a reference_id, even if a symbol could not be resolved.") | ||
.with_label(reference.span().label("This reference's reference_id is None")) | ||
.into() | ||
} | ||
|
||
/// The [`NodeId`] of the [`IdentifierReference`] did not match the [`NodeId`] of the | ||
/// [`Reference`]. | ||
fn node_id_mismatch( | ||
identifier_reference_id: NodeId, | ||
identifier_reference: &IdentifierReference, | ||
reference_id: ReferenceId, | ||
reference: &Reference, | ||
) -> TestResult { | ||
OxcDiagnostic::error( | ||
"NodeId mismatch between an IdentifierReference and its corresponding Reference", | ||
) | ||
.with_label( | ||
identifier_reference | ||
.span | ||
.label(format!("This IdentifierReference's NodeId is {identifier_reference_id:?}")), | ||
) | ||
.with_help(format!( | ||
"The Reference with id {reference_id:?} has a NodeId of {:?}", | ||
reference.node_id() | ||
)) | ||
.into() | ||
} | ||
|
||
impl ConformanceTest for IdentifierReferenceTest { | ||
fn name(&self) -> &'static str { | ||
"identifier-reference" | ||
} | ||
|
||
fn run_on_node<'a>( | ||
&self, | ||
node: &oxc_semantic::AstNode<'a>, | ||
semantic: &Semantic<'a>, | ||
) -> TestResult { | ||
let AstKind::IdentifierReference(id) = node.kind() else { | ||
return TestResult::Pass; | ||
}; | ||
let Some(reference_id) = id.reference_id() else { | ||
return missing_reference_id(id); | ||
}; | ||
|
||
let reference = semantic.symbols().get_reference(reference_id); | ||
if reference.node_id() != node.id() { | ||
return node_id_mismatch(node.id(), id, reference_id, reference); | ||
} | ||
|
||
TestResult::Pass | ||
} | ||
} |
130 changes: 130 additions & 0 deletions
130
crates/oxc_semantic/tests/conformance/test_symbol_declaration.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,130 @@ | ||
use oxc_ast::ast::BindingPattern; | ||
use oxc_ast::{ast::BindingIdentifier, AstKind}; | ||
use oxc_diagnostics::OxcDiagnostic; | ||
use oxc_span::{GetSpan, Span}; | ||
use oxc_syntax::symbol::SymbolId; | ||
|
||
use super::{ConformanceTest, TestResult}; | ||
use crate::Semantic; | ||
|
||
/// Verifies that symbol binding relationships between the SymbolTable and AST nodes are reflexive. | ||
/// | ||
/// What does this mean? | ||
/// 1. [`SymbolTable`] stores the AST node id of the node declaring a symbol. | ||
/// 2. That symbol should _always_ be a declaration-like node containing either a | ||
/// [`BindingIdentifier`] or a [`BindingPattern`]. | ||
/// 3. The binding pattern or identifier in that node should be populated (e.g. not [`None`]) and | ||
/// contain the symbol id. | ||
/// | ||
/// [`SymbolTable`]: oxc_semantic::SymbolTable | ||
#[derive(Debug, Clone, Default)] | ||
pub struct SymbolDeclarationTest; | ||
|
||
/// The binding pattern or identifier contained in the declaration node is [`None`]. | ||
/// | ||
/// See: [`BindingIdentifier::symbol_id`] | ||
fn bound_to_statement_with_no_binding_identifier( | ||
symbol_id: SymbolId, | ||
span: Span, | ||
statement_kind: &str, | ||
) -> TestResult { | ||
OxcDiagnostic::error(format!( | ||
"Symbol {symbol_id:?} got bound to a {statement_kind} with no BindingIdentifier" | ||
)) | ||
.with_label(span.label("Symbol was declared here")) | ||
.into() | ||
} | ||
|
||
/// [`BindingIdentifier::symbol_id`] contained [`Some`] value, but it was not the [`SymbolId`] used | ||
/// to find it in the [`SymbolTable`]. | ||
fn symbol_declaration_not_in_ast_node( | ||
expected_id: SymbolId, | ||
binding: &BindingIdentifier, | ||
) -> TestResult { | ||
let bound_id = binding.symbol_id.get(); | ||
OxcDiagnostic::error(format!( | ||
"Expected binding to be bound to {expected_id:?} but it was bound to {bound_id:?}" | ||
)) | ||
.with_label(binding.span()) | ||
.into() | ||
} | ||
|
||
/// Found a non-destructuring [`BindingPattern`] that did not contain a [`BindingIdentifier`]. | ||
fn malformed_binding_pattern(expected_id: SymbolId, pattern: &BindingPattern) -> TestResult { | ||
OxcDiagnostic::error(format!("BindingPattern for {expected_id:?} is not a destructuring pattern but get_binding_identifier() still returned None")) | ||
.with_label(pattern.span().label("BindingPattern is here")) | ||
.into() | ||
} | ||
|
||
fn invalid_declaration_node(kind: AstKind) -> TestResult { | ||
OxcDiagnostic::error(format!("Invalid declaration node kind: {}", kind.debug_name())) | ||
.with_label(kind.span()) | ||
.into() | ||
} | ||
|
||
impl ConformanceTest for SymbolDeclarationTest { | ||
fn name(&self) -> &'static str { | ||
"symbol-declaration" | ||
} | ||
|
||
fn run_on_symbol( | ||
&self, | ||
symbol_id: oxc_semantic::SymbolId, | ||
semantic: &Semantic<'_>, | ||
) -> TestResult { | ||
let declaration_id = semantic.symbols().get_declaration(symbol_id); | ||
let declaration = semantic.nodes().get_node(declaration_id); | ||
let span = semantic.symbols().get_span(symbol_id); | ||
|
||
match declaration.kind() { | ||
AstKind::VariableDeclarator(decl) => check_binding_pattern(symbol_id, &decl.id), | ||
AstKind::CatchParameter(caught) => check_binding_pattern(symbol_id, &caught.pattern), | ||
AstKind::Function(func) => match func.id.as_ref() { | ||
Some(id) => check_binding(symbol_id, id), | ||
None => bound_to_statement_with_no_binding_identifier(symbol_id, span, "Function"), | ||
}, | ||
AstKind::Class(class) => match class.id.as_ref() { | ||
Some(id) => check_binding(symbol_id, id), | ||
None => bound_to_statement_with_no_binding_identifier(symbol_id, span, "Class"), | ||
}, | ||
AstKind::BindingRestElement(rest) => check_binding_pattern(symbol_id, &rest.argument), | ||
AstKind::FormalParameter(param) => check_binding_pattern(symbol_id, ¶m.pattern), | ||
AstKind::ImportSpecifier(import) => check_binding(symbol_id, &import.local), | ||
AstKind::ImportNamespaceSpecifier(import) => check_binding(symbol_id, &import.local), | ||
AstKind::ImportDefaultSpecifier(import) => check_binding(symbol_id, &import.local), | ||
// =========================== TYPESCRIPT =========================== | ||
AstKind::TSImportEqualsDeclaration(import) => check_binding(symbol_id, &import.id), | ||
AstKind::TSTypeParameter(decl) => check_binding(symbol_id, &decl.name), | ||
// NOTE: namespaces do not store the symbol id they create. We may want to add this in | ||
// the future. | ||
AstKind::TSModuleDeclaration(_decl) => TestResult::Pass, | ||
AstKind::TSTypeAliasDeclaration(decl) => check_binding(symbol_id, &decl.id), | ||
AstKind::TSInterfaceDeclaration(decl) => check_binding(symbol_id, &decl.id), | ||
AstKind::TSEnumDeclaration(decl) => check_binding(symbol_id, &decl.id), | ||
// NOTE: enum members do not store the symbol id they create. We may want to add this | ||
// in the future. | ||
AstKind::TSEnumMember(_member) => TestResult::Pass, | ||
invalid_kind => invalid_declaration_node(invalid_kind), | ||
} | ||
} | ||
} | ||
|
||
fn check_binding_pattern(expected_id: SymbolId, binding: &BindingPattern) -> TestResult { | ||
if binding.kind.is_destructuring_pattern() { | ||
return TestResult::Pass; | ||
} | ||
|
||
let Some(id) = binding.kind.get_binding_identifier() else { | ||
return malformed_binding_pattern(expected_id, binding); | ||
}; | ||
|
||
check_binding(expected_id, id) | ||
} | ||
|
||
fn check_binding(expected_id: SymbolId, binding: &BindingIdentifier) -> TestResult { | ||
if binding.symbol_id.get() == Some(expected_id) { | ||
TestResult::Pass | ||
} else { | ||
symbol_declaration_not_in_ast_node(expected_id, binding) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.