refactor(ast): get rid of unsafe transmutation in VisitMut trait.

[rzvxa]

This will close #2745,

In this PR I attempt to fix this issue using a combination of ideas discussed in the issue mentioned above, I've created this early draft so people can pitch in if there is something I should consider doing.

The first goal of this PR is to resolve the issue with the possible illegal references, As a result of my approach it would also end up with a bunch of walk_* and walk_*_mut functions to help with the abstraction. I want to eliminate enter_node and leave_node functions, but I still haven't started working on it since I first want to familiarize myself with all of its usage throughout the project. I'm hesitating to do it at the moment, When we want to do this it would require quite a bit of refactoring so we should make sure it is probably going to work and end up being a better implementation.

[rzvxa]

@Boshen somehow I can't get tasks to run on my system, Is there any possibility for me to have tasks run on GitHub? If there is no lack of resources for running my tasks remotely, it would help me to continue my work on it easier. Please consider approving this PR for running workflows.

It stopped me dead on track after the initial commit of mine, I need to get it to work before I can continue working on the issue on hand since I'm sure I've already broken some stuff with this change.

I suspect that it is because of external resources, I've found out that older releases(which went around 5 to 8 months back) use submodules to add some external repositories, and I can see there are no more submodules on the main for including them, What is the approach now?

[Dunqing]

@Boshen somehow I can't get tasks to run on my system, Is there any possibility for me to have tasks run on GitHub? If there is no lack of resources for running my tasks remotely, it would help me to continue my work on it easier. Please consider approving this PR for running workflows.

Sure, Since this is your first time contributing to this repo we need to manually approve the run ci.

[codspeed-hq]

CodSpeed Performance Report

Merging #2764 will not alter performance

_{Comparing rzvxa:main (c478207) with main (d9b77d8)}

Summary

✅ 34 untouched benchmarks

[rzvxa]

@Boshen somehow I can't get tasks to run on my system, Is there any possibility for me to have tasks run on GitHub? If there is no lack of resources for running my tasks remotely, it would help me to continue my work on it easier. Please consider approving this PR for running workflows.

Sure, Since this is your first time contributing to this repo we need to manually approve the run ci.

Thanks!

[rzvxa]

This PR should be ready to merge, I've managed to get rid of the transmutation in the VisitMut trait altogether. I've also made a few methods in that trait use a walk function for their underlying implementation. If you guys agree in another PR I can refactor all methods to use walk functions for abstraction.

Let me know if there is any issue, ambiguity, lack of documentation, or clarity in this PR.

[rzvxa]

@Dunqing Hi, Sorry to ping you here. Can you approve workflow actions for this PR once more? I've found a workaround for tests by running them on my fork but I can't get the benchmark result.
If there are no performance drops it should be ready to merge.

[Boshen]

I wonder whether we should copy a VisitMut2 and move this to the transformer, so the transformer is all self-contained.

For two reasons:

• I think there are downstream tools which depend on this, this'll be a huge breaking change.
• The transformer will end-up with a few more iterations of overhaul. There are several people who are looking at the transformer from different angles. All self-contained code will make transitions easier.

I'm on vacation right now, please excuse me for the late response and the trouble.

[rzvxa]

Hello @Boshen, First off I wanna thank you for responding while on vacation. It means a lot!

I think there are downstream tools which depend on this, this'll be a huge breaking change.

I believe if this change goes through despite it being a breaking change it will be a net positive for the third-party tools since we don't enforce unsafe code without letting them know about our assumptions of the code. Plus we are in the minor versions so breaking changes should be expected as it is not a stable release. It is also fairly easy to achieve the same thing without the old design, And if you need the transmutation in a specific transformer or third-party tool? well, then it is on the implementor to handle it, not the core trait itself.
If we don't change how visitors work right now, It going to never change or at least it is going to be left untouched for quite a long time since we will be too scared to even touch it.

The transformer will end-up with a few more iterations of overhaul. There are several people who are looking at the transformer from different angles. All self-contained code will make transitions easier.

I didn't quite understand this statement, Can you let me know why is there going to be additional iteration as a result of this approach?

If we add the walk functions most of the time when changing the logic we are going to do it in walk_functions and those can be used for most use cases unless something really needs deep changes like how SemanticBuilder works. So most people won't need to worry about upcoming breaking changes if they only rely on visit functions and defer walking to the walk function instead of doing it themselves.

I honestly believe we can even remove enter_node leave_node, enter_scope, and leave_scope, All of these methods can be implemented by the user instead of being called by walk_functions from within the trait, Localizing the Visitor, in this case, means that people have a basic visitor that can visit any node if they want to handle scope or node events they can use these methods instead of having specific methods for those events. It would make specialized traits local to their own transformer/crate.

---

TL;DR I think since implementing a custom VisitMut with walk functions introduced in #2776 isn't going to be such a chore, People can implement their own custom implementations if there is a need for a specific visitor in a specific case.

[rzvxa]

Localizing the Visitor, in this case, means that people have a basic visitor that can visit any node if they want to handle scope or node events they can use these methods instead of having specific methods for those events. It would make specialized traits local to their own transformer/crate.

I just want to add this, We can have a local trait called ScopeVisit which adds the scope events and is a super trait of the Visit.

[rzvxa]

In future, we can also experiment with the use of reference counting for AstNode's reference to the node itself. This way we shouldn't need the transmutation to begin with at the cost of some runtime performance.

[rzvxa]

Sorry for speaking in such short comments, I'm just thinking about this PR out loud so you know my thought process and can give me some insight if there is something that I'm not aware of.

Take this for example

impl<'a> VisitMut<'a> for NewTarget<'a> {
    fn enter_node(&mut self, kind: AstKind<'a>) {
        if let Some(kind) = self.get_kind(kind) {
            self.kinds.push(kind);
        }
    }

    fn leave_node(&mut self, kind: AstKind<'a>) {
        if self.get_kind(kind).is_some() {
            self.kinds.pop();
        }
    }
}

While it does implement VisitMut it doesn't need it to work. If it was going to actually walk the nodes itself it would've been a lot slower.
I've just taken the idea which was already there and rewrote it as such:

impl<'a> NewTarget<'a> {
    pub(crate) fn enter_method_definition(&mut self, def: &MethodDefinition<'a>) {
        let kind = match def.kind {
            MethodDefinitionKind::Get
            | MethodDefinitionKind::Set
            | MethodDefinitionKind::Method => NewTargetKind::Method,
            MethodDefinitionKind::Constructor => NewTargetKind::Constructor,
        };
        self.push(kind);
    }

    pub(crate) fn leave_method_definition(&mut self, _: &MethodDefinition) {
        self.pop();
    }

    pub(crate) fn enter_object_property(&mut self, prop: &ObjectProperty<'a>) {
        if prop.method {
            self.push(NewTargetKind::Method);
        }
    }

    pub(crate) fn leave_object_property(&mut self, prop: &ObjectProperty<'a>) {
        if prop.method {
            self.pop();
        }
    }

    pub(crate) fn enter_function(&mut self, func: &Function<'a>) {
        if let Some(kind) = self.function_new_target_kind(func) {
            self.push(kind);
        }
    }

    pub(crate) fn leave_function(&mut self, func: &Function<'a>) {
        if self.function_new_target_kind(func).is_some() {
            self.pop();
        }
    }

    fn function_new_target_kind(&self, func: &Function<'a>) -> Option<NewTargetKind<'a>> {
        // oxc visitor `MethodDefinitionKind` will enter `Function` node, here need to exclude it
        if let Some(kind) = self.kinds.last() {
            if !matches!(kind, NewTargetKind::Function(_)) {
                return None;
            }
        }
        func.id.as_ref().map(|id| NewTargetKind::Function(Some(id.name.clone())))
    }
}

Now with walk functions, we can have the main transformer(transformer/lib.rs) implement something like this:

    // from Transformer implementation of VisitMut
    fn visit_function(&mut self, func: &mut Function<'a>, flags: Option<ScopeFlags>) {
        self.es2015_new_target.as_mut().map(|t| t.enter_function(func));
        walk_function_mut(self, func, flags);
        self.es2015_new_target.as_mut().map(|t| t.leave_function(func));
    }

It is a bit more verbose as you can see, But it is more readable, does less work, compiles easier(including compiler optimizations), and is more future-proof. For example, it won't break if we add an additional AstKind and don't have a default case, or even worse we handle the default case which makes a place that needs change go unnoticed.

[rzvxa]

I would like to elaborate on this statement.

does less work, compiles easier(including compiler optimizations)

In the example I've mentioned above it wouldn't be noticeable, But when walking huge arrays if we don't call enter and leave functions on every node it is going to let the compiler and CPU better use cache lines which in theory should minimize the cache misses while iterating the child nodes.

I believe if this early benchmark improvement isn't a false positive, It is probably because of the reason I've talked about here.

[Boshen]

Let's get the boat sailing.