-
Notifications
You must be signed in to change notification settings - Fork 784
Port --isolatedDeclarations related node builder logic
#1693
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
base: main
Are you sure you want to change the base?
Conversation
… than expression ones so widening is handled correctly
…cker data is ready, reducing literal expression diffs
f734c03 to
21a3832
Compare
--isolatedModules and related node builder logic--isolatedDeclarations and related node builder logic
--isolatedDeclarations and related node builder logic--isolatedModules and related node builder logic
--isolatedModules and related node builder logic--isolatedDeclarations and related node builder logic
|
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 |
…on/better comparitor to fallback to standard serialization less aggressively
|
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.
Yes. Copying, comparing, and transforming.
Yep. It's all analogous to logic in
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 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. |
|
Oh, before I forget to note it for myself and others, this isn't full ID itself still needs the error generation logic ported. See the |
--isolatedDeclarations and related node builder logic--isolatedDeclarations related node builder logic
Heavily WIP with a bunch still TODO - I've spent a lot of time disentangling the circular
hoststructure ID previously used that really doesn't port easily togoand 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 inexpressionToTypeNode.tswithout 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).reuseNodethat 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 thereuseNodeimprovements, 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 uninstantiatedTs copied without regard for correctness)). I also need to add the sanity-checking that used to be (sometimes) done by thecanReuseTypeNodechain of host calls (which should now just be a check in a single branch of the newpsudoTypeToNode, 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.