From deba6f5eebfa794ea076755bf74f79cd7681f342 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 21 Jan 2025 11:45:42 +0100 Subject: [PATCH] [red-knot] Anchor relative paths in configurations --- crates/red_knot/src/main.rs | 19 +- crates/red_knot/tests/cli.rs | 137 +++++++++++++- crates/red_knot/tests/file_watching.rs | 15 +- crates/red_knot_project/src/metadata.rs | 13 +- .../red_knot_project/src/metadata/options.rs | 50 ++--- .../src/metadata/pyproject.rs | 7 +- crates/red_knot_project/src/metadata/value.rs | 173 ++++++++++++++++++ 7 files changed, 371 insertions(+), 43 deletions(-) create mode 100644 crates/red_knot_project/src/metadata/value.rs diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index e1e647bb9c0c9e..e898d37618d287 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -7,6 +7,7 @@ use colored::Colorize; use crossbeam::channel as crossbeam_channel; use python_version::PythonVersion; use red_knot_project::metadata::options::{EnvironmentOptions, Options}; +use red_knot_project::metadata::value::RelativePathBuf; use red_knot_project::watch; use red_knot_project::watch::ProjectWatcher; use red_knot_project::{ProjectDatabase, ProjectMetadata}; @@ -69,22 +70,16 @@ struct Args { } impl Args { - fn to_options(&self, cli_cwd: &SystemPath) -> Options { + fn to_options(&self) -> Options { Options { environment: Some(EnvironmentOptions { python_version: self.python_version.map(Into::into), - venv_path: self - .venv_path - .as_ref() - .map(|venv_path| SystemPath::absolute(venv_path, cli_cwd)), - typeshed: self - .typeshed - .as_ref() - .map(|typeshed| SystemPath::absolute(typeshed, cli_cwd)), + venv_path: self.venv_path.as_ref().map(RelativePathBuf::cli), + typeshed: self.typeshed.as_ref().map(RelativePathBuf::cli), extra_paths: self.extra_search_path.as_ref().map(|extra_search_paths| { extra_search_paths .iter() - .map(|path| SystemPath::absolute(path, cli_cwd)) + .map(RelativePathBuf::cli) .collect() }), ..EnvironmentOptions::default() @@ -158,8 +153,8 @@ fn run() -> anyhow::Result { .transpose()? .unwrap_or_else(|| cli_base_path.clone()); - let system = OsSystem::new(cwd.clone()); - let cli_options = args.to_options(&cwd); + let system = OsSystem::new(cwd); + let cli_options = args.to_options(); let mut workspace_metadata = ProjectMetadata::discover(system.current_directory(), &system)?; workspace_metadata.apply_cli_options(cli_options.clone()); diff --git a/crates/red_knot/tests/cli.rs b/crates/red_knot/tests/cli.rs index c7cffa07f60214..9feecc6105c422 100644 --- a/crates/red_knot/tests/cli.rs +++ b/crates/red_knot/tests/cli.rs @@ -6,7 +6,7 @@ use tempfile::TempDir; /// Specifying an option on the CLI should take precedence over the same setting in the /// project's configuration. #[test] -fn test_config_override() -> anyhow::Result<()> { +fn config_override() -> anyhow::Result<()> { let tempdir = TempDir::new()?; std::fs::write( @@ -51,6 +51,141 @@ print(sys.last_exc) Ok(()) } +/// Paths specified on the CLI are relative to the current working directory and not the project root. +/// +/// We test this by adding an extra search path from the CLI to the libs directory when +/// running the CLI from the child directory (using relative paths). +/// +/// Project layout: +/// ``` +/// - libs +/// |- utils.py +/// - child +/// | - test.py +/// - pyproject.toml +/// ``` +/// +/// And the command is run in the `child` directory. +#[test] +fn cli_arguments_are_relative_to_the_current_directory() -> anyhow::Result<()> { + let tempdir = TempDir::new()?; + + let project_dir = tempdir.path(); + + let libs = project_dir.join("libs"); + std::fs::create_dir_all(&libs).context("Failed to create `libs` directory")?; + + let child = project_dir.join("child"); + std::fs::create_dir(&child).context("Failed to create `child` directory")?; + + std::fs::write( + tempdir.path().join("pyproject.toml"), + r#" +[tool.knot.environment] +python-version = "3.11" +"#, + ) + .context("Failed to write `pyproject.toml`")?; + + std::fs::write( + libs.join("utils.py"), + r#" +def add(a: int, b: int) -> int: + a + b +"#, + ) + .context("Failed to write `utils.py`")?; + + std::fs::write( + child.join("test.py"), + r#" +from utils import add + +stat = add(10, 15) +"#, + ) + .context("Failed to write `child/test.py`")?; + + insta::with_settings!({filters => vec![(&*tempdir_filter(&tempdir), "/")]}, { + assert_cmd_snapshot!(knot().current_dir(child).arg("--extra-search-path").arg("../libs"), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + "); + }); + + Ok(()) +} + +/// Paths specified in a configuration file are relative to the project root. +/// +/// We test this by adding `libs` (as a relative path) to the extra search path in the configuration and run +/// the CLI from a subdirectory. +/// +/// Project layout: +/// ``` +/// - libs +/// |- utils.py +/// - child +/// | - test.py +/// - pyproject.toml +/// ``` +#[test] +fn paths_in_configuration_files_are_relative_to_the_project_root() -> anyhow::Result<()> { + let tempdir = TempDir::new()?; + + let project_dir = tempdir.path(); + + let libs = project_dir.join("libs"); + std::fs::create_dir_all(&libs).context("Failed to create `libs` directory")?; + + let child = project_dir.join("child"); + std::fs::create_dir(&child).context("Failed to create `child` directory")?; + + std::fs::write( + tempdir.path().join("pyproject.toml"), + r#" +[tool.knot.environment] +python-version = "3.11" +extra-paths = ["libs"] +"#, + ) + .context("Failed to write `pyproject.toml`")?; + + std::fs::write( + libs.join("utils.py"), + r#" +def add(a: int, b: int) -> int: + a + b +"#, + ) + .context("Failed to write `utils.py`")?; + + std::fs::write( + child.join("test.py"), + r#" +from utils import add + +stat = add(10, 15) +"#, + ) + .context("Failed to write `child/test.py`")?; + + insta::with_settings!({filters => vec![(&*tempdir_filter(&tempdir), "/")]}, { + assert_cmd_snapshot!(knot().current_dir(child), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + "); + }); + + Ok(()) +} + fn knot() -> Command { Command::new(get_cargo_bin("red_knot")) } diff --git a/crates/red_knot/tests/file_watching.rs b/crates/red_knot/tests/file_watching.rs index 49788e9785d874..3f55064f9d8002 100644 --- a/crates/red_knot/tests/file_watching.rs +++ b/crates/red_knot/tests/file_watching.rs @@ -6,6 +6,7 @@ use std::time::{Duration, Instant}; use anyhow::{anyhow, Context}; use red_knot_project::metadata::options::{EnvironmentOptions, Options}; use red_knot_project::metadata::pyproject::{PyProject, Tool}; +use red_knot_project::metadata::value::RelativePathBuf; use red_knot_project::watch::{directory_watcher, ChangeEvent, ProjectWatcher}; use red_knot_project::{Db, ProjectDatabase, ProjectMetadata}; use red_knot_python_semantic::{resolve_module, ModuleName, PythonPlatform, PythonVersion}; @@ -791,7 +792,7 @@ fn search_path() -> anyhow::Result<()> { let mut case = setup_with_options([("bar.py", "import sub.a")], |root_path, _project_path| { Some(Options { environment: Some(EnvironmentOptions { - extra_paths: Some(vec![root_path.join("site_packages")]), + extra_paths: Some(vec![RelativePathBuf::cli(root_path.join("site_packages"))]), ..EnvironmentOptions::default() }), ..Options::default() @@ -832,7 +833,7 @@ fn add_search_path() -> anyhow::Result<()> { // Register site-packages as a search path. case.update_options(Options { environment: Some(EnvironmentOptions { - extra_paths: Some(vec![site_packages.clone()]), + extra_paths: Some(vec![RelativePathBuf::cli("site_packages")]), ..EnvironmentOptions::default() }), ..Options::default() @@ -855,7 +856,7 @@ fn remove_search_path() -> anyhow::Result<()> { let mut case = setup_with_options([("bar.py", "import sub.a")], |root_path, _project_path| { Some(Options { environment: Some(EnvironmentOptions { - extra_paths: Some(vec![root_path.join("site_packages")]), + extra_paths: Some(vec![RelativePathBuf::cli(root_path.join("site_packages"))]), ..EnvironmentOptions::default() }), ..Options::default() @@ -951,7 +952,7 @@ fn changed_versions_file() -> anyhow::Result<()> { |root_path, _project_path| { Some(Options { environment: Some(EnvironmentOptions { - typeshed: Some(root_path.join("typeshed")), + typeshed: Some(RelativePathBuf::cli(root_path.join("typeshed"))), ..EnvironmentOptions::default() }), ..Options::default() @@ -1375,10 +1376,12 @@ mod unix { Ok(()) }, - |_root, project| { + |_root, _project| { Some(Options { environment: Some(EnvironmentOptions { - extra_paths: Some(vec![project.join(".venv/lib/python3.12/site-packages")]), + extra_paths: Some(vec![RelativePathBuf::cli( + ".venv/lib/python3.12/site-packages", + )]), python_version: Some(PythonVersion::PY312), ..EnvironmentOptions::default() }), diff --git a/crates/red_knot_project/src/metadata.rs b/crates/red_knot_project/src/metadata.rs index 52e25566b8f1b8..c249fc291bcde9 100644 --- a/crates/red_knot_project/src/metadata.rs +++ b/crates/red_knot_project/src/metadata.rs @@ -1,15 +1,18 @@ use red_knot_python_semantic::ProgramSettings; use ruff_db::system::{System, SystemPath, SystemPathBuf}; use ruff_python_ast::name::Name; +use std::sync::Arc; use thiserror::Error; use crate::combine::Combine; use crate::metadata::pyproject::{Project, PyProject, PyProjectError}; +use crate::metadata::value::ValueSource; use options::KnotTomlError; use options::Options; pub mod options; pub mod pyproject; +pub mod value; #[derive(Debug, PartialEq, Eq)] #[cfg_attr(test, derive(serde::Serialize))] @@ -87,7 +90,10 @@ impl ProjectMetadata { let pyproject_path = project_root.join("pyproject.toml"); let pyproject = if let Ok(pyproject_str) = system.read_to_string(&pyproject_path) { - match PyProject::from_toml_str(&pyproject_str) { + match PyProject::from_toml_str( + &pyproject_str, + ValueSource::File(Arc::new(pyproject_path.clone())), + ) { Ok(pyproject) => Some(pyproject), Err(error) => { return Err(ProjectDiscoveryError::InvalidPyProject { @@ -103,7 +109,10 @@ impl ProjectMetadata { // A `knot.toml` takes precedence over a `pyproject.toml`. let knot_toml_path = project_root.join("knot.toml"); if let Ok(knot_str) = system.read_to_string(&knot_toml_path) { - let options = match Options::from_toml_str(&knot_str) { + let options = match Options::from_toml_str( + &knot_str, + ValueSource::File(Arc::new(knot_toml_path.clone())), + ) { Ok(options) => options, Err(error) => { return Err(ProjectDiscoveryError::InvalidKnotToml { diff --git a/crates/red_knot_project/src/metadata/options.rs b/crates/red_knot_project/src/metadata/options.rs index df3cf02d9c89de..283fca9b948450 100644 --- a/crates/red_knot_project/src/metadata/options.rs +++ b/crates/red_knot_project/src/metadata/options.rs @@ -1,7 +1,8 @@ +use crate::metadata::value::{RelativePathBuf, ValueSource, ValueSourceGuard}; use red_knot_python_semantic::{ ProgramSettings, PythonPlatform, PythonVersion, SearchPathSettings, SitePackages, }; -use ruff_db::system::{System, SystemPath, SystemPathBuf}; +use ruff_db::system::{System, SystemPath}; use ruff_macros::Combine; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -16,7 +17,8 @@ pub struct Options { } impl Options { - pub(crate) fn from_toml_str(content: &str) -> Result { + pub(crate) fn from_toml_str(content: &str, source: ValueSource) -> Result { + let _guard = ValueSourceGuard::new(source); let options = toml::from_str(content)?; Ok(options) } @@ -44,19 +46,19 @@ impl Options { project_root: &SystemPath, system: &dyn System, ) -> SearchPathSettings { - let src_roots = - if let Some(src_root) = self.src.as_ref().and_then(|src| src.root.as_deref()) { - vec![src_root.to_path_buf()] + let src_roots = if let Some(src_root) = self.src.as_ref().and_then(|src| src.root.as_ref()) + { + vec![src_root.absolute(project_root, system)] + } else { + let src = project_root.join("src"); + + // Default to `src` and the project root if `src` exists and the root hasn't been specified. + if system.is_directory(&src) { + vec![project_root.to_path_buf(), src] } else { - let src = project_root.join("src"); - - // Default to `src` and the project root if `src` exists and the root hasn't been specified. - if system.is_directory(&src) { - vec![project_root.to_path_buf(), src] - } else { - vec![project_root.to_path_buf()] - } - }; + vec![project_root.to_path_buf()] + } + }; let (extra_paths, python, typeshed) = self .environment @@ -71,11 +73,17 @@ impl Options { .unwrap_or_default(); SearchPathSettings { - extra_paths: extra_paths.unwrap_or_default(), + extra_paths: extra_paths + .unwrap_or_default() + .into_iter() + .map(|path| path.absolute(project_root, system)) + .collect(), src_roots, - typeshed, + typeshed: typeshed.map(|path| path.absolute(project_root, system)), site_packages: python - .map(|venv_path| SitePackages::Derived { venv_path }) + .map(|venv_path| SitePackages::Derived { + venv_path: venv_path.absolute(project_root, system), + }) .unwrap_or(SitePackages::Known(vec![])), } } @@ -91,23 +99,23 @@ pub struct EnvironmentOptions { /// List of user-provided paths that should take first priority in the module resolution. /// Examples in other type checkers are mypy's MYPYPATH environment variable, /// or pyright's stubPath configuration setting. - pub extra_paths: Option>, + pub extra_paths: Option>, /// Optional path to a "typeshed" directory on disk for us to use for standard-library types. /// If this is not provided, we will fallback to our vendored typeshed stubs for the stdlib, /// bundled as a zip file in the binary - pub typeshed: Option, + pub typeshed: Option, // TODO: Rename to python, see https://github.com/astral-sh/ruff/issues/15530 /// The path to the user's `site-packages` directory, where third-party packages from ``PyPI`` are installed. - pub venv_path: Option, + pub venv_path: Option, } #[derive(Debug, Default, Clone, Eq, PartialEq, Combine, Serialize, Deserialize)] #[serde(rename_all = "kebab-case", deny_unknown_fields)] pub struct SrcOptions { /// The root of the project, used for finding first-party modules. - pub root: Option, + pub root: Option, } #[derive(Error, Debug)] diff --git a/crates/red_knot_project/src/metadata/pyproject.rs b/crates/red_knot_project/src/metadata/pyproject.rs index bcf6b2ed953099..b2924399b659c1 100644 --- a/crates/red_knot_project/src/metadata/pyproject.rs +++ b/crates/red_knot_project/src/metadata/pyproject.rs @@ -4,6 +4,7 @@ use std::ops::Deref; use thiserror::Error; use crate::metadata::options::Options; +use crate::metadata::value::{ValueSource, ValueSourceGuard}; /// A `pyproject.toml` as specified in PEP 517. #[derive(Deserialize, Serialize, Debug, Default, Clone)] @@ -28,7 +29,11 @@ pub enum PyProjectError { } impl PyProject { - pub(crate) fn from_toml_str(content: &str) -> Result { + pub(crate) fn from_toml_str( + content: &str, + source: ValueSource, + ) -> Result { + let _guard = ValueSourceGuard::new(source); toml::from_str(content).map_err(PyProjectError::TomlSyntax) } } diff --git a/crates/red_knot_project/src/metadata/value.rs b/crates/red_knot_project/src/metadata/value.rs new file mode 100644 index 00000000000000..8d731f0b86a06c --- /dev/null +++ b/crates/red_knot_project/src/metadata/value.rs @@ -0,0 +1,173 @@ +use ruff_db::system::{System, SystemPath, SystemPathBuf}; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use std::cell::RefCell; +use std::fmt; +use std::hash::{Hash, Hasher}; +use std::sync::Arc; + +use crate::combine::Combine; +use crate::Db; + +#[derive(Clone, Debug)] +pub enum ValueSource { + /// Value loaded from a project's configuration file. + /// + /// Ideally, we'd use [`ruff_db::files::File`] but we can't because the database hasn't been + /// created when loading the configuration. + File(Arc), + /// The value comes from a CLI argument, while it's left open if specified using a short argument, + /// long argument (`--extra-paths`) or `--config key=value`. + Cli, +} + +impl fmt::Display for ValueSource { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::File(p) => fmt::Display::fmt(p, f), + Self::Cli => write!(f, "--config cli option"), + } + } +} + +thread_local! { + /// Serde doesn't provide any easy means to pass a value to a [`Deserialize`] implementation, + /// but we want to associate each deserialized [`RelativePath`] with the source from + /// where it origins. We use a thread local variable to work around this limitation. + /// + /// Use the [`ValueSourceGuard`] to initialize the thread local before calling into any + /// deserialization code. It ensures that the thread local variable gets cleaned up + /// once deserialization is done (once the guard gets dropped). + static VALUE_SOURCE: RefCell> = const { RefCell::new(None) }; +} + +/// Guard to safely change the [`VALUE_SOURCE`] for the current thread. +#[must_use] +pub(super) struct ValueSourceGuard { + prev_value: Option, +} + +impl ValueSourceGuard { + pub(super) fn new(source: ValueSource) -> Self { + let prev = VALUE_SOURCE.replace(Some(source)); + Self { prev_value: prev } + } +} + +impl Drop for ValueSourceGuard { + fn drop(&mut self) { + VALUE_SOURCE.set(self.prev_value.take()); + } +} + +/// A possibly relative path in a configuration file. +/// +/// Relative paths in configuration files or from CLI options +/// require different anchoring: +/// +/// * CLI: The path is relative to the current working directory +/// * Configuration file: The path is relative to the project's root. +#[derive(Debug, Clone)] +pub struct RelativePathBuf { + path: SystemPathBuf, + source: ValueSource, +} + +impl RelativePathBuf { + pub fn new(path: impl AsRef, source: ValueSource) -> Self { + Self { + path: path.as_ref().to_path_buf(), + source, + } + } + + pub fn cli(path: impl AsRef) -> Self { + Self::new(path, ValueSource::Cli) + } + + /// Returns the relative path as specified by the user. + pub fn path(&self) -> &SystemPath { + &self.path + } + + /// Returns the owned relative path. + pub fn into_path_buf(self) -> SystemPathBuf { + self.path + } + + /// Resolves the absolute path for `self` based on from where the value origins. + pub fn absolute_with_db(&self, db: &dyn Db) -> SystemPathBuf { + self.absolute(db.project().root(db), db.system()) + } + + /// Resolves the absolute path for `self` based on from where the value origins. + pub fn absolute(&self, project_root: &SystemPath, system: &dyn System) -> SystemPathBuf { + let relative_to = match &self.source { + ValueSource::File(_) => project_root, + ValueSource::Cli => system.current_directory(), + }; + + SystemPath::absolute(&self.path, relative_to) + } +} + +// TODO(micha): Derive most of those implementations once `RelativePath` uses `Value`. +// and use `serde(transparent, deny_unknown_fields)` +impl Combine for RelativePathBuf { + fn combine(self, _other: Self) -> Self { + self + } + + #[inline(always)] + fn combine_with(&mut self, _other: Self) {} +} + +impl Hash for RelativePathBuf { + fn hash(&self, state: &mut H) { + self.path.hash(state); + } +} + +impl PartialEq for RelativePathBuf { + fn eq(&self, other: &Self) -> bool { + self.path.eq(&other.path) + } +} + +impl Eq for RelativePathBuf {} + +impl PartialOrd for RelativePathBuf { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for RelativePathBuf { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.path.cmp(&other.path) + } +} + +impl Serialize for RelativePathBuf { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + self.path.serialize(serializer) + } +} + +impl<'de> Deserialize<'de> for RelativePathBuf { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let path = SystemPathBuf::deserialize(deserializer)?; + Ok(VALUE_SOURCE.with_borrow(|source| { + let source = source + .clone() + .expect("Thread local `VALUE_SOURCE` to be set. Use `ValueSourceGuard` to set the value source before calling serde/toml `from_str`."); + + Self { path, source } + })) + } +}