From b5e8eff8dacaf8376df5c2eb30c7b8fcbd109b1b Mon Sep 17 00:00:00 2001 From: camc314 <18101008+camc314@users.noreply.github.com> Date: Fri, 9 Oct 2026 09:40:06 +0000 Subject: [PATCH] feat(parser)!: assign node IDs during parsing (#27411) Assign unique node IDs while the parser constructs AST nodes, reserving zero for Program. IDs follow construction order and may have gaps from discarded nodes or reparsing; semantic analysis still replaces them with dense IDs for its node store. This is a breaking change because consumers can no longer assume node IDs are unset after parsing. --- crates/oxc_ast/src/builder/mod.rs | 2 +- crates/oxc_parser/src/ast_builder.rs | 48 +++++++++ crates/oxc_parser/src/lib.rs | 21 +++- crates/oxc_parser/tests/node_ids.rs | 151 +++++++++++++++++++++++++++ crates/oxc_syntax/src/node.rs | 4 +- tasks/coverage/src/driver.rs | 29 ++++- 6 files changed, 243 insertions(+), 12 deletions(-) create mode 100644 crates/oxc_parser/src/ast_builder.rs create mode 100644 crates/oxc_parser/tests/node_ids.rs diff --git a/crates/oxc_ast/src/builder/mod.rs b/crates/oxc_ast/src/builder/mod.rs index 464cafe842..dbc57df7a0 100644 --- a/crates/oxc_ast/src/builder/mod.rs +++ b/crates/oxc_ast/src/builder/mod.rs @@ -123,7 +123,7 @@ pub trait GetAstBuilder<'a> { /// AST builder which assigns dummy [`NodeId`]s to AST nodes. /// /// For use where no `NodeId`s are required on AST nodes as they are built -/// e.g. parser, because `NodeId`s are assigned later when building `Semantic`. +/// e.g. generated nodes whose IDs will be assigned when rebuilding `Semantic`. pub struct AstBuilder<'a> { /// The memory allocator used to allocate AST nodes in the arena. allocator: &'a Allocator, diff --git a/crates/oxc_parser/src/ast_builder.rs b/crates/oxc_parser/src/ast_builder.rs new file mode 100644 index 0000000000..f9dc1c5a7a --- /dev/null +++ b/crates/oxc_parser/src/ast_builder.rs @@ -0,0 +1,48 @@ +use std::cell::Cell; + +use oxc_allocator::{Allocator, GetAllocator}; +use oxc_ast::builder::{AstBuild, GetAstBuilder}; +use oxc_syntax::node::NodeId; + +/// Assign IDs as nodes are constructed, reserving zero for Program. +/// +/// IDs follow construction order, rather than visitor order, and may have gaps +/// from discarded nodes. The counter is deliberately not rewound during +/// speculation or top-level await reparsing: nodes constructed after an earlier +/// checkpoint can still survive in the returned AST. +pub struct ParserAstBuilder<'a> { + allocator: &'a Allocator, + next_node_id: Cell, +} + +impl<'a> ParserAstBuilder<'a> { + pub fn new(allocator: &'a Allocator) -> Self { + Self { allocator, next_node_id: Cell::new(1) } + } +} + +impl<'a> AstBuild<'a> for ParserAstBuilder<'a> { + #[inline] + fn node_id(&self) -> NodeId { + let index = self.next_node_id.get(); + assert!(index <= NodeId::MAX_INDEX, "Too many AST nodes"); + self.next_node_id.set(index + 1); + NodeId::new(index) + } +} + +impl<'a> GetAstBuilder<'a> for ParserAstBuilder<'a> { + type Builder = Self; + + #[inline] + fn builder(&self) -> &Self { + self + } +} + +impl<'a> GetAllocator<'a> for ParserAstBuilder<'a> { + #[inline] + fn allocator(&self) -> &'a Allocator { + self.allocator + } +} diff --git a/crates/oxc_parser/src/lib.rs b/crates/oxc_parser/src/lib.rs index 2a87410874..b822e16a7f 100644 --- a/crates/oxc_parser/src/lib.rs +++ b/crates/oxc_parser/src/lib.rs @@ -66,6 +66,7 @@ use std::any::Any; +mod ast_builder; pub mod config; mod context; mod cursor; @@ -98,6 +99,7 @@ use oxc_syntax::module_record::ModuleRecord; pub use crate::lexer::{Kind, Token}; use crate::{ + ast_builder::ParserAstBuilder, config::{ LexerConfig, NoTokensParserConfig, ParserConfig, RuntimeParserConfig, TokensParserConfig, }, @@ -165,6 +167,10 @@ pub struct ParserReturn<'a> { /// /// Will be empty (e.g. no statements, directives, etc) if the parser encountered a fatal error. /// + /// Every node has a unique node ID within this AST. Program has ID zero; + /// other IDs follow construction order and may have gaps from discarded nodes. + /// Semantic analysis replaces these with its own dense node IDs. + /// /// ## Validity /// It is possible for the AST to be present and semantically invalid. This will happen if /// 1. The [`Parser`] encounters a recoverable syntax error @@ -415,6 +421,9 @@ mod parser_parse { /// Parse a single [`Expression`]. /// + /// Every node receives a unique, nonzero node ID. IDs follow construction + /// order and may have gaps from discarded nodes. + /// /// # Example /// /// ```rust @@ -654,7 +663,7 @@ struct ParserImpl<'a, C: ParserConfig> { ctx: Context, /// Ast builder for creating AST nodes - ast: AstBuilder<'a>, + ast: ParserAstBuilder<'a>, /// Module Record Builder module_record_builder: ModuleRecordBuilder<'a>, @@ -690,7 +699,7 @@ impl<'a, C: ParserConfig> ParserImpl<'a, C> { prev_token_end: 0, state: ParserState::new(), ctx: Self::default_context(source_type, options), - ast: AstBuilder::new(allocator), + ast: ParserAstBuilder::new(allocator), module_record_builder: ModuleRecordBuilder::new(allocator, source_type), is_ts: source_type.is_typescript(), } @@ -847,7 +856,9 @@ impl<'a, C: ParserConfig> ParserImpl<'a, C> { hashbang, directives, statements, - self, + // The default builder assigns ID zero (NodeId::ROOT) to Program. + // ParserAstBuilder starts at one, keeping every child ID distinct. + &AstBuilder::new(self.allocator()), ) } @@ -1017,10 +1028,10 @@ impl<'a, C: ParserConfig> GetAllocator<'a> for ParserImpl<'a, C> { } impl<'a, C: ParserConfig> GetAstBuilder<'a> for ParserImpl<'a, C> { - type Builder = AstBuilder<'a>; + type Builder = ParserAstBuilder<'a>; #[inline] - fn builder(&self) -> &AstBuilder<'a> { + fn builder(&self) -> &ParserAstBuilder<'a> { &self.ast } } diff --git a/crates/oxc_parser/tests/node_ids.rs b/crates/oxc_parser/tests/node_ids.rs new file mode 100644 index 0000000000..71727b3966 --- /dev/null +++ b/crates/oxc_parser/tests/node_ids.rs @@ -0,0 +1,151 @@ +use rustc_hash::FxHashSet; + +use oxc_allocator::Allocator; +use oxc_ast::{AstKind, AstType, ast::Program}; +use oxc_ast_visit::Visit; +use oxc_parser::{ + ParseOptions, Parser, + config::{NoTokensParserConfig, ParserConfig, RuntimeParserConfig, TokensParserConfig}, +}; +use oxc_span::{GetSpan, SourceType, Span}; +use oxc_syntax::node::NodeId; + +#[derive(Default)] +struct NodeIds { + seen: FxHashSet, + nodes: Vec<(AstType, Span, NodeId)>, +} + +impl<'a> Visit<'a> for NodeIds { + fn enter_node(&mut self, kind: AstKind<'a>) { + let id = kind.node_id(); + if matches!(kind, AstKind::Program(_)) { + assert_eq!(id, NodeId::ROOT); + } else { + assert_ne!(id, NodeId::DUMMY, "Unassigned ID for {kind:?}"); + } + assert!(self.seen.insert(id), "Duplicate ID for {kind:?}"); + self.nodes.push((kind.ty(), kind.span(), id)); + } +} + +fn collect(program: &Program<'_>) -> Vec<(AstType, Span, NodeId)> { + let mut ids = NodeIds::default(); + ids.visit_program(program); + ids.nodes +} + +fn check(source: &str, source_type: SourceType, config: C, preserve_parens: bool) { + let allocator = Allocator::default(); + let parsed = Parser::new(&allocator, source, source_type) + .with_config(config) + .with_options(ParseOptions { preserve_parens, ..ParseOptions::default() }) + .parse(); + assert!(parsed.diagnostics.is_empty(), "{source}: {:?}", parsed.diagnostics); + assert!(!parsed.fatal_error, "{source}"); + let nodes = collect(&parsed.program); + assert_eq!(nodes[0].0, AstType::Program); +} + +#[test] +fn assigns_ids_in_all_parser_configs() { + let cases = [ + ("", SourceType::mjs()), + ("/* comments only */", SourceType::mjs()), + ("#!/usr/bin/env node\n'use strict';\nvalue;", SourceType::mjs()), + ("function f([first, ...rest]) { return [first, , `${rest.length}`]; }", SourceType::mjs()), + ( + "type Pair = [left: T, right: T]; const pair: Pair = [1, 2];", + SourceType::ts(), + ), + ( + "const element = <>{/* comment */}{value};", + SourceType::jsx(), + ), + ("const element: JSX.Element = prop={value} />;", SourceType::tsx()), + ]; + for (source, source_type) in cases { + for preserve_parens in [false, true] { + check(source, source_type, NoTokensParserConfig, preserve_parens); + check(source, source_type, TokensParserConfig, preserve_parens); + for tokens in [false, true] { + check(source, source_type, RuntimeParserConfig::new(tokens), preserve_parens); + } + } + } +} + +#[test] +fn ids_survive_speculation_and_cover_grammar() { + for source in [ + "const f = (a = call()) => a; const value = (a + b);", + "const f = async (a = call()) => a; const value = async(a + b);", + "const f = (a: T): T => a; const value = (a as number);", + "({ a: [b = c, ...rest], d = e } = value); [first, , ...rest] = value;", + "const value = ((a ? (b): string => b : c));", + ] { + for preserve_parens in [false, true] { + check(source, SourceType::ts(), NoTokensParserConfig, preserve_parens); + } + } +} + +#[test] +fn ids_survive_unambiguous_await_reparsing() { + for source in [ + "before(); await /x/u; between(); await /y/g; export {}; tail();", + "before(); await /a/ /b/g; between(); await /x/u; export {}; tail();", + "await\nawait /x/u; export {};", + "const x = await /x/u; between(); const y = await /y/g; export {};", + "#!/usr/bin/env node\n'use strict';\nawait /* regexp */ /x/u; export {};", + ] { + check(source, SourceType::unambiguous(), NoTokensParserConfig, true); + check(source, SourceType::unambiguous(), TokensParserConfig, true); + } +} + +#[test] +fn assigns_ids_to_recovered_nodes() { + for (source, source_type) in [ + ("interface I { [key: boolean]: string; }", SourceType::ts()), + ("interface I extends (value) {}", SourceType::ts()), + ("const value = <>
;", SourceType::jsx()), + ] { + let allocator = Allocator::default(); + let parsed = Parser::new(&allocator, source, source_type).parse(); + assert!(!parsed.fatal_error, "{source}: {:?}", parsed.diagnostics); + assert!(!parsed.diagnostics.is_empty(), "{source}"); + assert!(collect(&parsed.program).len() > 1); + } +} + +#[test] +fn fatal_errors_return_only_the_root_node() { + let allocator = Allocator::default(); + let parsed = Parser::new(&allocator, "before(); const value = ;", SourceType::mjs()).parse(); + assert!(parsed.fatal_error); + assert_eq!(collect(&parsed.program), [(AstType::Program, Span::default(), NodeId::ROOT)]); +} + +#[test] +fn expression_parser_assigns_nonzero_ids() { + for source in ["left + right", "(a + b)", "({ a: [b = c, ...rest] } = value)"] { + let allocator = Allocator::default(); + let expression = + Parser::new(&allocator, source, SourceType::mjs()).parse_expression().unwrap(); + let mut ids = NodeIds::default(); + ids.visit_expression(&expression); + assert_ne!(ids.nodes, []); + } +} + +#[test] +fn each_parse_starts_a_new_id_sequence() { + let allocator = Allocator::default(); + let source = "'use strict'; function f(a) { return a + 1; }"; + let parse = || Parser::new(&allocator, source, SourceType::mjs()).parse().program; + let first = parse(); + Parser::new(&allocator, "unrelated();", SourceType::mjs()).parse(); + let second = parse(); + assert_eq!(collect(&first), collect(&second)); +} diff --git a/crates/oxc_syntax/src/node.rs b/crates/oxc_syntax/src/node.rs index bb1a5bacb0..d6513f4f14 100644 --- a/crates/oxc_syntax/src/node.rs +++ b/crates/oxc_syntax/src/node.rs @@ -55,7 +55,9 @@ impl SemanticId for NodeId {} /// Get the [`NodeId`] of given AST node. /// -/// Only use this method on a post-semantic AST where [`NodeId`]s are always defined. +/// Parser-assigned IDs are unique within the parsed AST but may have gaps and +/// do not follow visitor order. Semantic analysis replaces them with dense IDs +/// that index its node store. Synthetically-created nodes may have [`NodeId::DUMMY`]. pub trait GetNodeId { /// Get the [`NodeId`] for an AST node. fn node_id(&self) -> NodeId; diff --git a/tasks/coverage/src/driver.rs b/tasks/coverage/src/driver.rs index 60dfc57940..2dc3d8fad1 100644 --- a/tasks/coverage/src/driver.rs +++ b/tasks/coverage/src/driver.rs @@ -6,10 +6,10 @@ use oxc::{ CompilerInterface, allocator::Allocator, ast::{ - Comment, + AstKind, Comment, ast::{Program, RegExpLiteral}, }, - ast_visit::{VisitJs, walk_js}, + ast_visit::{Visit, walk}, codegen::{CodegenOptions, CodegenReturn}, diagnostics::{Diagnostics, OxcDiagnostic}, minifier::CompressOptions, @@ -17,6 +17,7 @@ use oxc::{ regular_expression::{LiteralParser, Options}, semantic::SemanticBuilderReturn, span::{ContentEq, SourceType, Span}, + syntax::node::NodeId, transformer::{TransformOptions, TransformerReturn}, }; use oxc_tasks_transform_checker::{check_semantic_after_transform, check_semantic_ids}; @@ -173,11 +174,17 @@ struct CheckASTNodes<'a> { driver: &'a mut Driver, source_text: &'a str, allocator: Allocator, + node_ids: FxHashSet, } impl<'a> CheckASTNodes<'a> { fn new(driver: &'a mut Driver, source_text: &'a str) -> Self { - Self { driver, source_text, allocator: Allocator::default() } + Self { + driver, + source_text, + allocator: Allocator::default(), + node_ids: FxHashSet::default(), + } } fn check(&mut self, program: &Program<'a>) { @@ -185,7 +192,19 @@ impl<'a> CheckASTNodes<'a> { } } -impl<'a> VisitJs<'a> for CheckASTNodes<'a> { +impl<'a> Visit<'a> for CheckASTNodes<'a> { + fn enter_node(&mut self, kind: AstKind<'a>) { + let id = kind.node_id(); + // These are AST invariants, including for recovered syntax errors. + // Reporting them as diagnostics would let negative fixtures hide a failure. + assert_eq!( + id == NodeId::ROOT, + matches!(kind, AstKind::Program(_)), + "Only Program may have parser node ID zero: {kind:?}" + ); + assert!(self.node_ids.insert(id), "Duplicate parser node ID: {kind:?}"); + } + // TODO: This is too slow // fn visit_span(&mut self, span: &Span) { // let Span { start, end, .. } = span; @@ -198,7 +217,7 @@ impl<'a> VisitJs<'a> for CheckASTNodes<'a> { /// Idempotency test for printing regular expressions. fn visit_reg_exp_literal(&mut self, literal: &RegExpLiteral<'a>) { - walk_js::walk_reg_exp_literal(self, literal); + walk::walk_reg_exp_literal(self, literal); let Some(pattern) = &literal.regex.pattern.pattern else { return;