red-knot: improve internal documentation in module.rs

[AlexWaygood]

This PR improves the internal documentation for various methods and structs in module.rs. The main purpose of this is to make sure I properly understand everything that the current code is doing :-)

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

Thanks. I'll have to copy those comments over.

I also renamed a couple of things in my PR (root => search_path) and move them around but I can deal with this when it comes to merging my PR.

[MichaReiser]

Another reason is that it is a specific concept that is worth its own name and it also stores additional data that isn't available on a path (except if you mean ModulePath)

[AlexWaygood]

Right. At first I thought this struct was really a ModuleID struct because it seemed to be a thin wrapper around a primitive type. But now I see that the primitive type really is the ID for the module, and that the struct is correctly named. Which means that this comment is not quite accurate!

[AlexWaygood]

Hmm, but that isn't quite right either, I see? Since the modules field on the ModuleResolver struct maps Module objects to ModuleData objects, and its the ModuleData objects that seem to hold the interesting information about the module.

In general I feel slightly confused about whether this is meant to represent:

• a struct that can be queried directly to obtain interesting information about a module (this is what would be implied by the name Module to me); or,
• a cheap ID that can be used to easily retrieve some other object that can be queried directly to obtain interesting information about a module (similar to FileID elsewhere in redknot, or to BindingID in ruff_python_semantic/binding.rs).

Currently it feels like it maybe sits awkwardly between the two concepts

[AlexWaygood]

I think I'll just remove this comment and try to refactor this into something I like more ;)

[MichaReiser]

The way module is defined is very intentional. Not necessarily the fields it stores and its name but that it is a thin wrapper around a u32 with methods to query its fields. From an API perspective, this is the Module and an outside client doesn't need to be concerned about how it stores the data internally.

The reason why we shouldn't change this representation significantly is because this exactly maps to Salsa. You can have a look at my PR and you'll find something very similar:

• ResolvedModule is a newtype wrapper around a u32. This is important for fast cache lookups and to avoid awkward lifetimes. It also ensures that we can store a ResolvedModule very cheaply, it's just 4 bytes.
• It has accessor functions that take db as an argument and return the field data

We can't change this without breaking Salsa compatibility. Ideally, you try to keep the API roughly unchanged (or use something similar to the salsa PR). But feel free to restructure the internal data structures however you want.

[AlexWaygood]

Thanks. I was confused by the names of variables such as this in module.rs, which seemed to imply to me that Module instances did not in fact represent modules -- that they only represented module IDs that could be used to lookup modules, and that it was in fact ModuleData instances that represented modules. Here the id variable has type Module, and the module variable has type ModuleData:

ruff/crates/red_knot/src/module.rs

Lines 491 to 500 in 7ce17b7

	fn remove_module_by_id(&mut self, id: Module) -> Arc<ModuleData> {
	let (_, module) = self.modules.remove(&id).unwrap();
	self.by_name.remove(&module.name).unwrap();
	// It's possible that multiple paths map to the same id. Search all other paths referencing the same module id.
	self.by_file.retain(|_, current_id| *current_id != id);
	module
	}

I'll create another PR to try to clarify things further in the internal documentation.

[carljm]

This is excellent, thank you!!