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

[red-knot] intern types separately from type inference result #12061

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Jun 27, 2024

Initial exploration of what it might look like to intern types independent of the type inference result, so the granularity of type inference and the granularity of type interning don't have to be coupled.

The goal of this is to make it possible to add definition_ty queries, but this PR doesn't do that yet.

This PR also doesn't work yet :) Just for illustration, I'm using Arc::get_mut in the places where we need to mutate the FileTypeStore, but we can't actually do this because Salsa is still holding on to a reference to the Arc also. To make this work, we'll need to introduce interior mutability to the FileTypeStore, which is a bit tricky along with wanting to return references to type structs stored inside it. The old TypeStore in the old red_knot crate shows one approach to this, but I'm not sure it's the best option.

Having the FileTypeStore returned by a salsa query at all might require the Salsa "mutable arena" feature discussed in salsa-rs/salsa#490 (comment) -- if so, for now we could instead just keep our our own dashmap of VfsFile to FileTypeStore on the Db, but not actually in Salsa.

Some nice things about interning types ourselves: a lot fewer lifetimes (no lifetime on Type enum anymore), and we can eliminate the TypingContext indirection layer and just use Db in all APIs.

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Jun 28, 2024
&self.module_type
}

pub(super) fn class_ty(&self, id: FileClassTypeId) -> &ClassType {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning a refence would work if we use an actual Arena that allocates a new vector when it runs out of space instead of resizing the existing one (which would invalidate previously returned Ids).

Or more generally. The way to return references here is by lifting the lifetime of types outside of type store by having the actual data owned further up.

@MichaReiser
Copy link
Member

I do like splitting out the type internet from the type inference result. I think it makes sense to have them separate and also allows more effective reuse.

The part that's unclear to me and that you already mentioned on your write up is how to collect unused types. How can we prevent that this is an ever-growing arena?

We could try to track type references by using an AtomicUsize (not an Arc because that would heap allocate each type!) It would give us very fine-grained reclamation of types, at least if we use a slab like arena that supports reusing IDs. An other possibility could be to rely on Salsa by using one interner per file, package or some other granularity, and the interner would be retrieved by calling a salsa-query. This won't give us type level collection, but salsa would evict the entire type store if e.g. the file gets deleted (or all files in a package).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants