Introduce `visit_span` on `VisitMut` and `Visit`

[hyf0]

Rolldown currently uses Spans as the way to identifier ast nodes. But it would become a problem, if the ast contains duplicate Spans.

So

we could have a new ast pass the will fill up empty span and ensure the uniqueness. https://github.com/rolldown/rolldown/pull/1899#issuecomment-2274936578

I want to ensure the uniqueness of Span of every node. So far if I want to do that with the VisitMut, I have to enumerate ever node. But with visit_span, it would be pretty easy.

[overlookmotel]

We'd need to make sure adding visit_span doesn't degrade performance. It'll be called for every single AST node, and most visitors don't need it. Hopefully the compiler will be smart enough to inline no-op visit_span calls and remove them entirely, but we need to make sure.

Also, for the Rolldown use case, I also imagine using visit_span will be pretty expensive. If you know only certain AST node types can have empty/non-unique spans, it's likely to be much faster to do the checks in visit_module_declaration (as you're doing now in https://github.com/rolldown/rolldown/pull/1927) and avoid the uniqueness check on every single other node.

[hyf0]

If you know only certain AST node types can have empty/non-unique spans

After we support custom transform, we can't be sure what certain nodes will have empty/non-unique spans.

It's true we could ensure the uniqueness of span only on AST nodes we care about. But that will lead more weakness in the whole process since we need to sync the logic in different places.

I also consider this as a workaround fix, feel like not worth to put more hard effort on rolldown's side. But adding visit_span is like thing the oxc will added in the end(I could be wrong about this part), so I raise this issue.

[IWANABETHATGUY]

We'd need to make sure adding visit_span doesn't degrade performance. It'll be called for every single AST node, and most visitors don't need it. Hopefully the compiler will be smart enough to inline no-op visit_span calls and remove them entirely, but we need to make sure.

Also, for the Rolldown use case, I also imagine using visit_span will be pretty expensive. If you know only certain AST node types can have empty/non-unique spans, it's likely to be much faster to do the checks in visit_module_declaration (as you're doing now in rolldown/rolldown#1927) and avoid the uniqueness check on every single other node.

Maybe oxc could add a feature to enable visit each span, since visit span is indeed not a common usage

[overlookmotel]

After we support custom transform, we can't be sure what certain nodes will have empty/non-unique spans.

Ah ha. Now I understand the problem.

Personally, I think Span is the wrong thing to be using for uniquely identifying AST nodes. They're just not unique.

But, since you are using them for that, I think we can try adding visit_span to visitors as you requested. Hopefully compiler will successfully elide visit_span calls when visit_span is a no-op, so it'll have zero cost unless used, and if so we can merge it without a feature.

[overlookmotel]

@rzvxa Are we able to add visit_span to Visit and VisitMut without a corresponding walk_span (i.e. no enter_node / leave_node calls) without some kind of special case in codegen? Another attr like #[visit(no_walk)]?

@Boshen Do you think we should add visit_span as requested? Or should we implement #4807 and suggest Rolldown use that instead?

[rzvxa]

It isn't hard to implement if we decide to do it this way.

[hyf0]

This is not required/urgent request. I raise this issue mainly due to I feel like this is thing oxc need to support in the end. So You could just stablize this feature as the way it should.

I did only find one usage in swc's codebase. Other than that, the real-world usages of visit_span seems still unknown.

https://github.com/swc-project/swc/blob/ff4ba2b078103871cd3fcd80a9d26f0ac13d8c84/crates/swc_ecma_transforms_react/src/refresh/mod.rs#L238-L268

[IWANABETHATGUY]

Identifying attached comments with span is not the right way, it would get in trouble if user wants to attach comments with a newly added Ast node. Hope LLVM is smart enough to eliminate the unused call.

[overlookmotel]

Identifying attached comments with span is not the right way, it would get in trouble if user wants to attach comments with a newly added Ast node.

Good point. So right now there's no way for a transform to attach comments to a newly-created AST node, is there?

Hope LLVM is smart enough to eliminate the unused call.

I would hope so. I've just learned not to assume I can predict the compiler! Sometimes it defies one's reasonable expectations. Only way to know for sure is to measure it.

[IWANABETHATGUY]

Good point. So right now there's no way for a transform to attach comments to a newly-created AST node, is there?

IIRC, no.