Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clone in scope API #146

Closed
overlookmotel opened this issue Aug 10, 2024 · 1 comment
Closed

Clone in scope API #146

overlookmotel opened this issue Aug 10, 2024 · 1 comment

Comments

@overlookmotel
Copy link

overlookmotel commented Aug 10, 2024

oxc-project/oxc#4732 introduced CloneIn trait.

Currently it clones SymbolId, ScopeId and ReferenceId along with all other parts of AST being cloned.

I don't think this is correct behavior, but am not sure what the correct behavior should be.

If you clone part of an AST which has been through Semantic, and insert it elsewhere in same AST, that AST will now contain duplicate IDs.

If you insert it into another AST, the IDs will be invalid. It can cause panic if try to look up the symbol for a SymbolId, if the destination AST has less symbols than the source AST, so SymbolId is out of bounds. And if it doesn't panic, it'll be the wrong symbol.

Ideally we should provide another API where you provide the destination AST's ScopeTree and SymbolTable, and what scope the cloned AST fragment will be inserted into, and then it can create new scopes/symbols/references, and patch up all the IDs in the clone automatically for you.

trait CloneInScope<'a>: Sized {
    type Cloned;
    fn clone_in_scope(
        &self,
        parent_scope_id: ScopeId,
        alloc: &'a Allocator,
        scopes: &mut ScopeTree,
        symbols: &mut SymbolTable,
    ) -> Self::Cloned;
}

But in meantime, to avoid invalid IDs, perhaps CloneIn should set scope_id, symbol_id and reference_id fields to None?

@overlookmotel overlookmotel changed the title What should CloneIn do with SymbolId, ScopeId, ReferenceId Clone in scope API Aug 10, 2024
overlookmotel referenced this issue in oxc-project/oxc Aug 15, 2024
…rseCtx` (#4880)

related: #4804

needs from: #4876

The `clone_identifier_reference` method is used to clone an `IdentifierReference` and create a `Reference` and insert it to `SymbolTable`'s `resolved_references`.

The reason we need this is because we need to make sure that `IdentifierReference`'s `reference_id` is unique
overlookmotel referenced this issue in oxc-project/oxc Aug 22, 2024
…5047)

Fix semantic error caused by `clone_in` causing `reference_id` to not exist.

In this PR, I try to use `move_expression` as much as possible instead of `expr.clone_in`. But in some cases, we need to reuse the same expression in multiple places. I have added a `clone_expression` to workaround it

I felt a bit painful we missing a [clone_in_scope](#4804) API
@Boshen
Copy link
Member

Boshen commented Nov 24, 2024

Duplicate of #127

@Boshen Boshen marked this as a duplicate of #127 Nov 24, 2024
@Boshen Boshen closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2024
This issue is being transferred. Timeline may not be complete until it finishes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants