fix(semantic)!: ensure program outlives semantic

[valeneiko]

fixes: #8437

In semantic builder make sure Program reference has a lifetime of the Arena.

[graphite-app]

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[codspeed-hq]

CodSpeed Performance Report

Merging #8455 will not alter performance

_{Comparing valeneiko:fix-semantic-lifetimes (d0358eb) with main (04bc259)}

Summary

✅ 32 untouched benchmarks

[Boshen]

pub program: &'a mut Program<'a> from ParserReturn doesn't feel right, let's see if @overlookmotel knows what the underly problem is.

[overlookmotel]

I've written up my thoughts on this problem in #8437 (comment) and #8461.

Personally, I quite like the idea of parser returning &mut Program. BUT that's a large breaking change, and I don't think it's necessary to fix this issue. I think just changing SemanticBuilder::build to take a &'a Program<'a> should be sufficient to squash the bug.

So I suggest we go one step at a time - fix #8437 first, then discuss API changes after. I have some other thoughts about API, and if we're going to make breaking changes, it'd be preferable to do that only once.

Or have I misunderstood? Is parser returning &mut Program necessary to fix the bug?

[valeneiko]

Changing parser return type is not necessary. But we would still need to update all usages of semantic to first copy the program into the allocator and then use the new reference.

let program = alloc.alloc(parserReturn.program);
let semanticReturn = SemanticBuilder::new().build(program);

[valeneiko]

@overlookmotel I've updated the PR to just fix semantic.

[overlookmotel]

Thanks very much for doing this, and thanks for reverting the changes to ParserReturn.

Unfortunately this does make the API harder to use, but that's necessary to make it correct. SemanticBuilderReturn does borrow the Program for the duration of its existence, so we need to make sure it does actually work that way.

I've pushed 1 extra commit to remove unreachable_unchecked. The purpose of that part of the coverage runner is to make sure that Semantic contains what it should do, so it's best not to make unsafe assertions about the very thing we're trying to check.

Apart from that, great!

This is a breaking change, so before we merge this we should make sure it doesn't break downstream consumers in a terrible way. I'll try it out with Rolldown now, and will report back.

[overlookmotel]

Not causing major problems in Rolldown. Given that's a large codebase which makes heavy use of Oxc, likely it's not going to cause too many problems in other downstream projects either.