[red-knot] implement eval_symbol for from-import and class-def

[carljm]

This PR demonstrates resolving an import from one module to a class type from another module!

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

Is allowing multiple threads to race resolution intentional if we don't have a cache hit? Or how does the locking work here between: No cache entry found and writing a new cache entry.

[carljm]

This is a great question. Currently of course it's "not an issue" because I was lazy and just took a mutable reference to the entire db, but as you pointed out above, that's a much bigger problem :) I will have to think more about the right strategy here. Ideally we do want to lock here to avoid multiple threads racing to resolve the same symbol, but ideally we want it to be somewhat fine-grained. But keeping a lock for every symbol might be too much.

[MichaReiser]

Nit

Suggested change

	if let Some(ty) = db.jar().type_store.get_cached_node_type(file_id, node_key) {
	ty
	} else {
	let parsed = db.parse(file_id);
	let ast = parsed.ast();
	let node = StmtClassDef::cast_ref(
	node_key
	.resolve(ast.as_any_node_ref())
	.expect("node key should resolve"),
	)
	.expect("node key should cast");
	let store = &mut db.jar_mut().type_store;
	let ty = store.add_class(file_id, &node.name.id);
	store.cache_node_type(file_id, *node_key, ty);
	ty
	}
	let Some(ty) = db.jar().type_store.get_cached_node_type(file_id, node_key) else {
	let parsed = db.parse(file_id);
	let ast = parsed.ast();
	let node = StmtClassDef::cast_ref(
	node_key
	.resolve(ast.as_any_node_ref())
	.expect("node key should resolve"),
	)
	.expect("node key should cast");
	let store = &mut db.jar_mut().type_store;
	let ty = store.add_class(file_id, &node.name.id);
	store.cache_node_type(file_id, *node_key, ty);
	ty
	}

[carljm]

The suggested change here doesn't work as-is, because this arm of the match needs to actually evaluate to a Type (ty). From what I can tell, it seems like let-else is for cases where the else diverges, but doesn't evaluate to an expression. I could restructure this to use a mutable ty I guess, but that doesn't seem like an improvement over the if-let-else I have now.

Let me know if I'm missing a way for this suggestion to work out better than the if-let-else!

[MichaReiser]

I think this should be a query and not a mutation

• Move up to other queries
• Change self to take a readonly reference.

[carljm]

yes agreed, I'll make this update, and use internal mutability on the type store for the caching instead

[carljm]

Going to do this in a follow up

[MichaReiser]

We can follow up on proper synchronization if we add a FIXME