[red-knot] Use a distinct type for module search paths in the module resolver

[AlexWaygood]

Summary

This PR cleans up some of the settings validation and types in the module resolver. Namely, at various points in the crate, we use a type alias, a ModuleResolutionPathBuf or an Arc<ModuleResolutionPathBuf> to refer to a module search path. This PR goes some way to cleaning that up by introducing a newtype wrapper in path.rs for module-resolution paths that specifically represent search paths.

The main motivation for this change is because module search paths and other module-resolution paths have different invariants, so should be validated differently. This PR has been split off from #12376, which bundled the validation changes in as the same PR, to ease review.

Copying from the PR description of #12376:

This PR goes halfway to addressing @carljm's review comments in #12141 (comment). We now have a distinct type for representing search paths specifically outside of path.rs. However, internally, path.rs still doesn't make much distinction between search paths and other kinds of paths: it doesn't have to, because I implemented Deref for the new ModuleSearchPath struct; all methods available on ModuleResolutionPathBuf are implicitly available on ModuleSearchPath. This is somewhat lazy and unprincipled of me, and I've left a TODO comment to get rid of it. I spent much of yesterday trying to get to a more principled solution, but it ends up being a massive diff that rewrites much of path.rs, and I don't think it's a priority for now. I think this is a nice cleanup that clarifies the types and improves settings validation in the meantime, without being a massive rewrite that would be time-consuming to write and review.

Test Plan

cargo test -p red_knot_module_resolver

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

I let @carljm approve this change. I like that some Arc<ModuleSearchPath> are gone. I'm otherwise not quiet sure what the benefit is.

Nit: We can reduce the visibility of ModuleSearchPathBuf and Ref to pub(super).

[MichaReiser]

I think we should inline the

ruff/crates/red_knot_module_resolver/src/path.rs

Lines 171 to 212 in dc8ae36

	#[must_use]
	pub(crate) fn extra(path: impl Into<SystemPathBuf>) -> Option<Self> {
	let path = path.into();
	path.extension()
	.map_or(true, |ext| matches!(ext, "py" | "pyi"))
	.then_some(Self(ModuleResolutionPathBufInner::Extra(path)))
	}
	#[must_use]
	pub(crate) fn first_party(path: impl Into<SystemPathBuf>) -> Option<Self> {
	let path = path.into();
	path.extension()
	.map_or(true, |ext| matches!(ext, "pyi" | "py"))
	.then_some(Self(ModuleResolutionPathBufInner::FirstParty(path)))
	}
	#[must_use]
	pub(crate) fn standard_library(path: FilePath) -> Option<Self> {
	path.extension()
	.map_or(true, |ext| ext == "pyi")
	.then_some(Self(ModuleResolutionPathBufInner::StandardLibrary(path)))
	}
	#[must_use]
	pub(crate) fn site_packages(path: impl Into<SystemPathBuf>) -> Option<Self> {
	let path = path.into();
	path.extension()
	.map_or(true, |ext| matches!(ext, "pyi" | "py"))
	.then_some(Self(ModuleResolutionPathBufInner::SitePackages(path)))
	}
	#[must_use]
	pub(crate) fn editable_installation_root(
	system: &dyn System,
	path: impl Into<SystemPathBuf>,
	) -> Option<Self> {
	let path = path.into();
	// TODO: Add Salsa invalidation to this system call:
	system
	.is_directory(&path)
	.then_some(Self(ModuleResolutionPathBufInner::EditableInstall(path)))
	}

and remove the definitions from ModuleResolutionPathBuf. The methods on ModuleResolutionPathBuf are only used in tests and I suspect that the tests should probably use ModuleResolutionPath instead.

[AlexWaygood]

This makes sense but I'm going to postpone it for now. The idea of this PR was that it would be a small, incremental change that would be easy to review. Addressing this would require lots of changes to tests, and I think this PR is an improvement over the status quo as it stands.

[AlexWaygood]

I'm otherwise not quiet sure what the benefit is.

Yeah, it's harder to see the benefit when the validation changes are decoupled from the change to use a new type. This is partly why I originally put them together in one PR, in #12376.

[carljm]

This looks like a good start that brings a bit of extra naming clarity, but doesn't do the real refactor yet, just as advertised.

I agree with both of Micha's comments.

I'm fine landing this and holding off on any more refactor for now, until/unless there are actual problems caused by the current structure.