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

[BREAKING CHANGE] Change Atom to Atom<'a> to make it safe #2497

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Conversation

Boshen
Copy link
Member

@Boshen Boshen commented Feb 25, 2024

Part of #2295

This PR splits the Atom type into Atom<'a> and CompactString.

All the AST node strings now use Atom<'a> instead of Atom to signify it belongs to the arena.

It is now up to the user to select which form of the string to use.

This PR essentially removes this really unsafe code

impl<'a> From<&'a str> for Atom {
fn from(s: &'a str) -> Self {
if s.len() <= INLINE_STRING_CAPACITY {
Self(AtomImpl::Inline(InlineString::from(s)))
} else {
// SAFETY: It is unsafe to use this string after the allocator is dropped.
Self(AtomImpl::Arena(unsafe { std::mem::transmute(s) }))
}
}
}

which can lead to
image

@github-actions github-actions bot added the A-ast Area - AST label Feb 25, 2024
@Boshen
Copy link
Member Author

Boshen commented Feb 25, 2024

@overlookmotel Here we go ... I still remember the week it took me to understand lifetimes and adding them to the AST, which is 2 years ago. It should be faster this time ... just follow compiler errors :-)

Once the errors propagate to crates other than the ast and parser crates, I'll decide on the stop whether I want to change them to CompactString.

@github-actions github-actions bot added A-linter Area - Linter A-parser Area - Parser A-semantic Area - Semantic A-minifier Area - Minifier A-transformer Area - Transformer / Transpiler A-codegen Area - Code Generation A-prettier Area - Prettier labels Feb 26, 2024
Copy link

codspeed-hq bot commented Feb 26, 2024

CodSpeed Performance Report

Merging #2497 will improve performances by 5.25%

Comparing atom-rework (9117d99) with main (93742f8)

Summary

⚡ 1 improvements
✅ 26 untouched benchmarks

Benchmarks breakdown

Benchmark main atom-rework Change
semantic[cal.com.tsx] 321.1 ms 305.1 ms +5.25%

@Boshen Boshen changed the title Atom rework [BREAKING CHANGE] Atom rework Feb 26, 2024
@Boshen Boshen changed the title [BREAKING CHANGE] Atom rework [BREAKING CHANGE] Atom to Atom<'a> rework Feb 26, 2024
@Boshen Boshen marked this pull request as ready for review February 26, 2024 10:25
@Boshen Boshen changed the title [BREAKING CHANGE] Atom to Atom<'a> rework [BREAKING CHANGE] Change Atom to Atom<'a> to make it safe Feb 26, 2024
@Boshen Boshen merged commit be6b8b7 into main Feb 26, 2024
23 checks passed
@Boshen Boshen deleted the atom-rework branch February 26, 2024 11:34
IWANABETHATGUY pushed a commit to IWANABETHATGUY/oxc that referenced this pull request May 29, 2024
…oject#2497)

Part of oxc-project#2295

This PR splits the `Atom` type into `Atom<'a>` and `CompactString`.

All the AST node strings now use `Atom<'a>` instead of `Atom` to signify
it belongs to the arena.

It is now up to the user to select which form of the string to use.

This PR essentially removes the really unsafe code 


https://github.com/oxc-project/oxc/blob/93742f89e91bc7c45491b6d2b49a134fa65f2b5c/crates/oxc_span/src/atom.rs#L98-L107

which can lead to 

![image](https://github.com/oxc-project/oxc/assets/1430279/8c513c4f-19b0-4b63-b61c-e07c187c95b5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST A-codegen Area - Code Generation A-linter Area - Linter A-minifier Area - Minifier A-parser Area - Parser A-prettier Area - Prettier A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant