refactor(traverse)!: remove TraverseCtx::clone_identifier_reference (#7266)

Remove `TraverseCtx::clone_identifier_reference`.

This API encourages unnecessary repeated lookups of the `SymbolId` for a reference. It is preferable to use `MaybeBoundIdentifier` which only looks up the `SymbolId` once.
This commit is contained in:
overlookmotel 2024-11-13 15:12:34 +00:00
parent 8c754b1b43
commit e84ea2c3c6
4 changed files with 12 additions and 27 deletions

View file

@ -33,7 +33,7 @@ use oxc_ast::{ast::*, NONE};
use oxc_semantic::{ReferenceFlags, ScopeFlags, SymbolFlags};
use oxc_span::SPAN;
use oxc_syntax::operator::{AssignmentOperator, BinaryOperator, LogicalOperator};
use oxc_traverse::{Ancestor, Traverse, TraverseCtx};
use oxc_traverse::{Ancestor, MaybeBoundIdentifier, Traverse, TraverseCtx};
use crate::TransformCtx;
@ -142,9 +142,10 @@ impl<'a, 'ctx> Traverse<'a> for NullishCoalescingOperator<'a, 'ctx> {
impl<'a, 'ctx> NullishCoalescingOperator<'a, 'ctx> {
fn clone_expression(expr: &Expression<'a>, ctx: &mut TraverseCtx<'a>) -> Expression<'a> {
match expr {
Expression::Identifier(ident) => Expression::Identifier(
ctx.ast.alloc(ctx.clone_identifier_reference(ident, ReferenceFlags::Read)),
),
Expression::Identifier(ident) => {
let binding = MaybeBoundIdentifier::from_identifier_reference(ident, ctx);
binding.create_spanned_expression(ident.span, ReferenceFlags::Read, ctx)
}
_ => expr.clone_in(ctx.ast.allocator),
}
}

View file

@ -59,7 +59,7 @@ use oxc_ast::ast::*;
use oxc_semantic::{ReferenceFlags, SymbolFlags};
use oxc_span::SPAN;
use oxc_syntax::operator::{AssignmentOperator, LogicalOperator};
use oxc_traverse::{BoundIdentifier, Traverse, TraverseCtx};
use oxc_traverse::{BoundIdentifier, MaybeBoundIdentifier, Traverse, TraverseCtx};
use crate::TransformCtx;
@ -293,14 +293,15 @@ impl<'a, 'ctx> LogicalAssignmentOperators<'a, 'ctx> {
/// Clone an expression
///
/// If it is an identifier, clone the identifier by [TraverseCtx::clone_identifier_reference], otherwise, use [CloneIn].
/// If it is an identifier, clone the identifier via [MaybeBoundIdentifier], otherwise, use [CloneIn].
///
/// TODO: remove this once <https://github.com/oxc-project/oxc/issues/4804> is resolved.
fn clone_expression(expr: &Expression<'a>, ctx: &mut TraverseCtx<'a>) -> Expression<'a> {
match expr {
Expression::Identifier(ident) => Expression::Identifier(
ctx.ast.alloc(ctx.clone_identifier_reference(ident, ReferenceFlags::Read)),
),
Expression::Identifier(ident) => {
let binding = MaybeBoundIdentifier::from_identifier_reference(ident, ctx);
binding.create_spanned_expression(ident.span, ReferenceFlags::Read, ctx)
}
_ => expr.clone_in(ctx.ast.allocator),
}
}

View file

@ -26,9 +26,7 @@ use super::BoundIdentifier;
/// it for later use.
/// * `MaybeBoundIdentifier` re-uses the same `Atom` for all `BindingIdentifier` / `IdentifierReference`s
/// created from it.
/// * `MaybeBoundIdentifier` looks up the `SymbolId` for the reference only once,
/// rather than `TraverseCtx::clone_identifier_reference` which looks it up every time you create
/// an `IdentifierReference`.
/// * `MaybeBoundIdentifier` looks up the `SymbolId` for the reference only once.
#[derive(Debug, Clone)]
pub struct MaybeBoundIdentifier<'a> {
pub name: Atom<'a>,

View file

@ -530,21 +530,6 @@ impl<'a> TraverseCtx<'a> {
self.scoping.delete_reference_for_identifier(ident);
}
/// Clone `IdentifierReference` based on the original reference's `SymbolId` and name.
///
/// This method makes a lookup of the `SymbolId` for the reference. If you need to create multiple
/// `IdentifierReference`s for the same binding, it is better to look up the `SymbolId` only once,
/// and generate `IdentifierReference`s with `TraverseCtx::create_reference_id`.
pub fn clone_identifier_reference(
&mut self,
ident: &IdentifierReference<'a>,
flags: ReferenceFlags,
) -> IdentifierReference<'a> {
let reference = self.symbols().get_reference(ident.reference_id());
let symbol_id = reference.symbol_id();
self.create_reference_id(ident.span, ident.name.clone(), symbol_id, flags)
}
/// Determine whether evaluating the specific input `node` is a consequenceless reference.
///
/// i.e. evaluating it won't result in potentially arbitrary code from being run.