fix(traverse)!: TraverseCtx::ancestor with level 0 = equivalent to …

…`parent` (#5294)

Change meaning of `level` passed to `TraverseCtx` from "levels above current" to "levels above parent". `ctx.parent()`'s equivalent was `ctx.ancestor(1)`, now it's `ctx.ancestor(0)`.

This prevents out of bounds read on `ctx.ancestor(0)` (UB), which was made possible by #5286.

crates/oxc_transformer/src/es2020/nullish_coalescing_operator.rs

@@ -99,11 +99,11 @@ impl<'a> Traverse<'a> for NullishCoalescingOperator<'a> {
             return;
         }
 
-        // ctx.ancestor(1) is AssignmentPattern
-        // ctx.ancestor(2) is BindingPattern;
-        // ctx.ancestor(3) is FormalParameter
+        // ctx.ancestor(0) is AssignmentPattern
+        // ctx.ancestor(1) is BindingPattern
+        // ctx.ancestor(2) is FormalParameter
         let is_parent_formal_parameter =
-            matches!(ctx.ancestor(3), Ancestor::FormalParameterPattern(_));
+            matches!(ctx.ancestor(2), Ancestor::FormalParameterPattern(_));
 
         let current_scope_id = if is_parent_formal_parameter {
             ctx.create_child_scope_of_current(ScopeFlags::Arrow | ScopeFlags::Function)

crates/oxc_traverse/src/context/ancestry.rs

@@ -57,16 +57,29 @@ impl<'a> TraverseAncestry<'a> {
 
     /// Get ancestor of current node.
     ///
-    /// `level` is number of levels above.
-    /// `ancestor(1)` is equivalent to `parent()`.
+    /// `level` is number of levels above parent.
+    /// `ancestor(0)` is equivalent to `parent()` (but better to use `parent()` as it's more efficient).
     ///
     /// If `level` is out of bounds (above `Program`), returns `Ancestor::None`.
     #[inline]
     pub fn ancestor<'t>(&'t self, level: usize) -> Ancestor<'a, 't> {
-        if level < self.stack.len() {
-            // SAFETY: We just checked that `level < self.stack.len()` so `self.stack.len() - level`
-            // cannot wrap around or be out of bounds
-            let ancestor = unsafe { *self.stack.get_unchecked(self.stack.len() - level) };
+        // Behavior with different values:
+        // `len = 1, level = 0` -> return `Ancestor::None` from else branch
+        // `len = 1, level = 1` -> return `Ancestor::None` from else branch (out of bounds)
+        // `len = 3, level = 0` -> return parent (index 2)
+        // `len = 3, level = 1` -> return grandparent (index 1)
+        // `len = 3, level = 2` -> return `Ancestor::None` from else branch
+        // `len = 3, level = 3` -> return `Ancestor::None` from else branch (out of bounds)
+
+        // `self.stack.len()` is always at least 1, so `self.stack.len() - 1` cannot wrap around.
+        // `level <= last_index` would also work here, but `level < last_index` avoids a read from memory
+        // when that read would just get `Ancestor::None` anyway.
+        debug_assert!(!self.stack.is_empty());
+        let last_index = self.stack.len() - 1;
+        if level < last_index {
+            // SAFETY: We just checked that `level < last_index` so `last_index - level` cannot wrap around,
+            // and `last_index - level` must be a valid index
+            let ancestor = unsafe { *self.stack.get_unchecked(last_index - level) };
 
             // Shrink `Ancestor`'s `'t` lifetime to lifetime of `&'t self`.
             // SAFETY: The `Ancestor` is guaranteed valid for `'t`. It is not possible to obtain

crates/oxc_traverse/src/context/mod.rs

@@ -140,8 +140,8 @@ impl<'a> TraverseCtx<'a> {
 
     /// Get ancestor of current node.
     ///
-    /// `level` is number of levels above.
-    /// `ancestor(1)` is equivalent to `parent()`.
+    /// `level` is number of levels above parent.
+    /// `ancestor(0)` is equivalent to `parent()` (but better to use `parent()` as it's more efficient).
     ///
     /// If `level` is out of bounds (above `Program`), returns `Ancestor::None`.
     ///

crates/oxc_traverse/src/lib.rs

@@ -134,7 +134,7 @@ mod compile_fail_tests;
 ///         }
 ///
 ///         // Read grandparent
-///         if let Ancestor::ExpressionStatementExpression(stmt_ref) = ctx.ancestor(2) {
+///         if let Ancestor::ExpressionStatementExpression(stmt_ref) = ctx.ancestor(1) {
 ///             // This is legal
 ///             println!("expression stmt's span: {:?}", stmt_ref.span());
 ///