Skip to content

Conversation

@weswigham
Copy link
Member

Heavily WIP with a bunch still TODO - I've spent a lot of time disentangling the circular host structure ID previously used that really doesn't port easily to go and inlining whatever unneeded abstractions I can. In the service of that, there is one new abstraction to decouple ID node lookup logic from the actual node/error generation - the psuedochecker (which, true to its' name, calculates psuedotypes), which mostly corresponds to the node lookup logic in expressionToTypeNode.ts without a lot of the intermingled error handling and node construction. This way the ID type node lookup logic is still "known checker-free", while avoiding trying to dip into the checker/node builder in it.

The primary thing I'm still working on is the replacement for (*nodeBuilderImpl).reuseNode that actually validates that the node is reusable (and triggers the node builder recovery scope otherwise), but this is a non-crashy checkpoint. It can't be merged as a WIP because without the reuseNode improvements, it's a fairly big step backwards in terms of output alignment (the ID logic very aggressively tries to reuse a ton of nodes that need some massaging to work at a different location, resulting in some pretty nonfunctional declarations (lots of uninstantiated Ts copied without regard for correctness)). I also need to add the sanity-checking that used to be (sometimes) done by the canReuseTypeNode chain of host calls (which should now just be a check in a single branch of the new psudoTypeToNode, ensuring it's more reliably checked).

The other-other thing that I really should do as part of this is reenable the node reuse baselines (to track where we do better/worse/different than strada); but that's probably better served as a followup PR, since that will add underlines to every diff in the repo.

@weswigham weswigham force-pushed the isolated-modules-related branch from f734c03 to 21a3832 Compare October 23, 2025 19:50
@jakebailey jakebailey changed the title Port --isolatedModules and related node builder logic Port --isolatedDeclarations and related node builder logic Jan 7, 2026
@jakebailey jakebailey changed the title Port --isolatedDeclarations and related node builder logic Port --isolatedModules and related node builder logic Jan 7, 2026
@weswigham weswigham changed the title Port --isolatedModules and related node builder logic Port --isolatedDeclarations and related node builder logic Jan 8, 2026
@jakebailey
Copy link
Member

I skimmed this and didn't find anything objectional; I think it's interesting that the actually large part of this is just copying logic...

I assume the "pseudo" concepts are really just a formalization of something that ID did previously?

What diffs introduced are interesting to show things that are broken? I tried looking at new ones, and most did not seem bad, though emitMethodCalledNew is one I noticed being wrong.

…on/better comparitor to fallback to standard serialization less aggressively
@weswigham
Copy link
Member Author

weswigham commented Jan 9, 2026

I've done a few light bugfixes and the fourslash change here, and then opened #2459 as a separate PR with the baseline update commit. If you wanna review the logic further, do it here, since the review interface will still work on this PR, but that's where the merge/CI will ultimately happen.

I skimmed this and didn't find anything objectional; I think it's interesting that the actually large part of this is just copying logic...

Yes. Copying, comparing, and transforming.

I assume the "pseudo" concepts are really just a formalization of something that ID did previously?

Yep. It's all analogous to logic in expressionToTypeNode.ts in strada without all the host reentry (instead, we have pseudoTypeToNode/Type in the real node builder to do similar checks without host cycles) - it's the type node inference logic. It's basically the collection of inferences/checking logic any emitter would need to have to have ID emit.

What diffs introduced are interesting to show things that are broken? I tried looking at new ones, and most did not seem bad, though emitMethodCalledNew is one I noticed being wrong.

Well, with my most recent commit I should have swapped it from over-copying to under-copying as the default state of affairs in the baselines, but it should be similar to strada. However, we should be able to do much better than corsa with this strategy. Now I'm invoking pseudoTypeToType and doing a == comparison on the result and the actual declaration type to see if the pseudotype-derived node will actually resolve correctly; that's going to over-invalidate structural types right now (see comments in pseudoTypeToType), but otherwise should work OK. The best thing to do would be to write a bespoke isPsuedoTypeEqualToType that can handle all the possible equivalency patterns, but this gets all the simple ones (direct type reference nodes, keyword types, and the like). This is basically the replacement for the canReuseTypeNode and canReuseTypeNodeAnnotation calls from strada, which are the root of the issues with ID-added logic over-preserving nodes in strada (namely, the former not doing enough validations).

In any case, this is probably good enough to start bugfixing from, and, with the most recent change, not a massive step back from current corsa behavior and rather an incremental push towards more ID node reuse without adding all the broken modalities from strada, too - we just need more implementations and fixes to match some of the looser strada behavior safely.

@weswigham
Copy link
Member Author

Oh, before I forget to note it for myself and others, this isn't full --isolatedDeclarations support - just the node building logic it introduced that non-ID code can incidentally rely on.

ID itself still needs the error generation logic ported. See the !!! still in ReportInferenceFallback in the declarations transformer. I can work on adding that here, if we'd like, but the baseline diffs are already absolutely ungodly without the error baseline changes...

@weswigham weswigham changed the title Port --isolatedDeclarations and related node builder logic Port --isolatedDeclarations related node builder logic Jan 9, 2026
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

Successfully merging this pull request may close these issues.

2 participants