From c936b66fbdab51cd15ec31c99d9797ec2cdab690 Mon Sep 17 00:00:00 2001 From: Zanie Date: Fri, 14 Jul 2023 14:08:43 -0500 Subject: [PATCH] Include file permissions in key for cached files --- Cargo.lock | 1 + crates/ruff_cli/Cargo.toml | 1 + crates/ruff_cli/src/cache.rs | 49 +++++++++++++++++++----------- crates/ruff_cli/src/diagnostics.rs | 42 ++++++++++++++++++------- 4 files changed, 65 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4d904a5884caf1..68ca61508401e3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2014,6 +2014,7 @@ dependencies = [ "ruff", "ruff_cache", "ruff_diagnostics", + "ruff_macros", "ruff_python_ast", "ruff_python_formatter", "ruff_python_stdlib", diff --git a/crates/ruff_cli/Cargo.toml b/crates/ruff_cli/Cargo.toml index ca58bdcb42ed3c..7e67c8ced8c456 100644 --- a/crates/ruff_cli/Cargo.toml +++ b/crates/ruff_cli/Cargo.toml @@ -30,6 +30,7 @@ ruff_python_ast = { path = "../ruff_python_ast" } ruff_python_formatter = { path = "../ruff_python_formatter" } ruff_text_size = { workspace = true } ruff_textwrap = { path = "../ruff_textwrap" } +ruff_macros = { path = "../ruff_macros" } annotate-snippets = { version = "0.9.1", features = ["color"] } anyhow = { workspace = true } diff --git a/crates/ruff_cli/src/cache.rs b/crates/ruff_cli/src/cache.rs index fe614e7f8aedb8..46737325e056f7 100644 --- a/crates/ruff_cli/src/cache.rs +++ b/crates/ruff_cli/src/cache.rs @@ -164,21 +164,20 @@ impl Cache { path.strip_prefix(&self.package.package_root).ok() } - /// Get the cached results for a single file at relative `path`. This uses - /// `file_last_modified` to determine if the results are still accurate + /// Get the cached results for a single file at relative `path`. This + /// uses `key` to determine if the results are still accurate. /// (i.e. if the file hasn't been modified since the cached run). /// - /// This returns `None` if `file_last_modified` differs from the cached - /// timestamp or if the cache doesn't contain results for the file. - pub(crate) fn get( - &self, - path: &RelativePath, - file_last_modified: SystemTime, - ) -> Option<&FileCache> { + /// This returns `None` if `key` differs from the cached key or if the + /// cache doesn't contain results for the file. + pub(crate) fn get(&self, path: &RelativePath, key: &T) -> Option<&FileCache> { let file = self.package.files.get(path)?; + let mut hasher = CacheKeyHasher::new(); + key.cache_key(&mut hasher); + // Make sure the file hasn't changed since the cached run. - if file.last_modified != file_last_modified { + if file.key != hasher.finish() { return None; } @@ -188,10 +187,10 @@ impl Cache { } /// Add or update a file cache at `path` relative to the package root. - pub(crate) fn update( + pub(crate) fn update( &self, path: RelativePathBuf, - last_modified: SystemTime, + key: T, messages: &[Message], imports: &ImportMap, ) { @@ -218,8 +217,11 @@ impl Cache { }) .collect(); + let mut hasher = CacheKeyHasher::new(); + key.cache_key(&mut hasher); + let file = FileCache { - last_modified, + key: hasher.finish(), last_seen: AtomicU64::new(self.last_seen_cache), imports: imports.clone(), messages, @@ -244,8 +246,8 @@ struct PackageCache { /// On disk representation of the cache per source file. #[derive(Deserialize, Debug, Serialize)] pub(crate) struct FileCache { - /// Timestamp when the file was last modified before the (cached) check. - last_modified: SystemTime, + /// Key that determines if the cached item is still valid. + key: u64, /// Timestamp when we last linted this file. /// /// Represented as the number of milliseconds since Unix epoch. This will @@ -332,7 +334,6 @@ mod tests { use std::io::{self, Write}; use std::path::{Path, PathBuf}; use std::sync::atomic::AtomicU64; - use std::time::SystemTime; use ruff::settings::{flags, AllSettings}; use ruff_cache::CACHE_DIR_NAME; @@ -487,6 +488,11 @@ mod tests { flip_execute_permission_bit(path)?; Ok(flip_execute_permission_bit) }, + #[cfg(windows)] + |path| { + flip_read_only_permission(path)?; + Ok(flip_read_only_permission) + }, ]; #[cfg(unix)] @@ -498,6 +504,15 @@ mod tests { file.set_permissions(PermissionsExt::from_mode(perms.mode() ^ 0o111)) } + #[cfg(windows)] + #[allow(clippy::items_after_statements)] + fn flip_read_only_permission(path: &Path) -> io::Result<()> { + let file = fs::OpenOptions::new().write(true).open(path)?; + let mut perms = file.metadata()?.permissions(); + perms.set_readonly(!perms.readonly()); + file.set_permissions(perms) + } + for change_file in tests { let cleanup = change_file(&path).unwrap(); @@ -546,7 +561,7 @@ mod tests { cache.package.files.insert( PathBuf::from("old.py"), FileCache { - last_modified: SystemTime::UNIX_EPOCH, + key: 123, last_seen: AtomicU64::new(123), imports: ImportMap::new(), messages: Vec::new(), diff --git a/crates/ruff_cli/src/diagnostics.rs b/crates/ruff_cli/src/diagnostics.rs index f994b7a055e05b..c667b2162f8fa3 100644 --- a/crates/ruff_cli/src/diagnostics.rs +++ b/crates/ruff_cli/src/diagnostics.rs @@ -8,11 +8,15 @@ use std::path::Path; use anyhow::{anyhow, Result}; use colored::Colorize; +use filetime::FileTime; use log::{debug, error}; use ruff_text_size::TextSize; use rustc_hash::FxHashMap; use similar::TextDiff; +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; +use crate::cache::Cache; use ruff::fs; use ruff::jupyter::Notebook; use ruff::linter::{lint_fix, lint_only, FixTable, FixerResult, LinterResult}; @@ -21,11 +25,18 @@ use ruff::message::Message; use ruff::pyproject_toml::lint_pyproject_toml; use ruff::settings::{flags, AllSettings, Settings}; use ruff::source_kind::SourceKind; +use ruff_macros::CacheKey; use ruff_python_ast::imports::ImportMap; use ruff_python_ast::source_code::{LineIndex, SourceCode, SourceFileBuilder}; use ruff_python_stdlib::path::{is_jupyter_notebook, is_project_toml}; -use crate::cache::Cache; +#[derive(CacheKey)] +pub(crate) struct FileCacheKey { + /// Timestamp when the file was last modified before the (cached) check. + file_last_modified: FileTime, + /// Permissions of the file before the (cached) check. + file_permissions_mode: u32, +} #[derive(Debug, Default, PartialEq)] pub(crate) struct Diagnostics { @@ -115,12 +126,26 @@ pub(crate) fn lint_path( let relative_path = cache .relative_path(path) .expect("wrong package cache for file"); - let last_modified = path.metadata()?.modified()?; - if let Some(cache) = cache.get(relative_path, last_modified) { + + // Construct a cache key for the file + let metadata = path.metadata()?; + + #[cfg(unix)] + let permissions = metadata.permissions().mode(); + #[cfg(windows)] + let permissions: u32 = metadata.permissions().readonly().into(); + let cache_key = FileCacheKey { + file_last_modified: FileTime::from_last_modification_time(&metadata), + file_permissions_mode: permissions, + }; + + if let Some(cache) = cache.get(relative_path, &cache_key) { return Ok(cache.as_diagnostics(path)); } - Some((cache, relative_path, last_modified)) + // Stash the file metadata for later so when we update the cache it reflects the prerun + // information + Some((cache, relative_path, cache_key)) } _ => None, }; @@ -216,15 +241,10 @@ pub(crate) fn lint_path( let imports = imports.unwrap_or_default(); - if let Some((cache, relative_path, file_last_modified)) = caching { + if let Some((cache, relative_path, key)) = caching { // We don't cache parsing errors. if parse_error.is_none() { - cache.update( - relative_path.to_owned(), - file_last_modified, - &messages, - &imports, - ); + cache.update(relative_path.to_owned(), key, &messages, &imports); } }