Skip to content

Commit

Permalink
Auto merge of #17227 - Veykril:build-deps-changed-hashes, r=Veykril
Browse files Browse the repository at this point in the history 
fix: Hash file contents to verify whether file actually changed

Fixes #16580
  • Loading branch information
@bors
bors committed May 14, 2024
2 parents 52135cb + 1ca97ba commit 77c7886
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 97 deletions.
5 changes: 2 additions & 3 deletions crates/hir-def/src/item_tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
//!
//! In general, any item in the `ItemTree` stores its `AstId`, which allows mapping it back to its
//! surface syntax.
#![allow(unexpected_cfgs)]

mod lower;
mod pretty;
Expand Down Expand Up @@ -467,13 +468,12 @@ macro_rules! mod_items {
pub enum GenericModItem {
$(
$(
#[cfg_attr(FALSE, $generic_params)]
#[cfg_attr(ignore_fragment, $generic_params)]
$typ(FileItemTreeId<$typ>),
)?
)+
}

#[allow(unexpected_cfgs)]
impl From<GenericModItem> for ModItem {
fn from(id: GenericModItem) -> ModItem {
match id {
Expand All @@ -494,7 +494,6 @@ macro_rules! mod_items {
}

$(
#[allow(unexpected_cfgs)]
impl From<FileItemTreeId<$typ>> for ModItem {
fn from(id: FileItemTreeId<$typ>) -> ModItem {
ModItem::$typ(id)
Expand Down
4 changes: 2 additions & 2 deletions crates/load-cargo/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,8 +361,8 @@ fn load_crate_graph(
}
}
let changes = vfs.take_changes();
for file in changes {
if let vfs::Change::Create(v) | vfs::Change::Modify(v) = file.change {
for (_, file) in changes {
if let vfs::Change::Create(v, _) | vfs::Change::Modify(v, _) = file.change {
if let Ok(text) = String::from_utf8(v) {
analysis_change.change_file(file.file_id, Some(text))
}
Expand Down
59 changes: 14 additions & 45 deletions crates/rust-analyzer/src/global_state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//!
//! Each tick provides an immutable snapshot of the state as `WorldSnapshot`.

use std::{collections::hash_map::Entry, time::Instant};
use std::time::Instant;

use crossbeam_channel::{unbounded, Receiver, Sender};
use flycheck::FlycheckHandle;
Expand All @@ -25,7 +25,7 @@ use project_model::{
use rustc_hash::{FxHashMap, FxHashSet};
use tracing::{span, Level};
use triomphe::Arc;
use vfs::{AnchoredPathBuf, ChangedFile, Vfs};
use vfs::{AnchoredPathBuf, Vfs};

use crate::{
config::{Config, ConfigError},
Expand Down Expand Up @@ -254,7 +254,6 @@ impl GlobalState {

pub(crate) fn process_changes(&mut self) -> bool {
let _p = span!(Level::INFO, "GlobalState::process_changes").entered();
let mut file_changes = FxHashMap::<_, ChangedFile>::default();
let (change, modified_rust_files, workspace_structure_change) = {
let mut change = ChangeWithProcMacros::new();
let mut guard = self.vfs.write();
Expand All @@ -266,44 +265,13 @@ impl GlobalState {
// downgrade to read lock to allow more readers while we are normalizing text
let guard = RwLockWriteGuard::downgrade_to_upgradable(guard);
let vfs: &Vfs = &guard.0;
// We need to fix up the changed events a bit. If we have a create or modify for a file
// id that is followed by a delete we actually skip observing the file text from the
// earlier event, to avoid problems later on.
for changed_file in changed_files {
use vfs::Change::*;
match file_changes.entry(changed_file.file_id) {
Entry::Occupied(mut o) => {
let change = o.get_mut();
match (&mut change.change, changed_file.change) {
// latter `Delete` wins
(change, Delete) => *change = Delete,
// merge `Create` with `Create` or `Modify`
(Create(prev), Create(new) | Modify(new)) => *prev = new,
// collapse identical `Modify`es
(Modify(prev), Modify(new)) => *prev = new,
// equivalent to `Modify`
(change @ Delete, Create(new)) => {
*change = Modify(new);
}
// shouldn't occur, but collapse into `Create`
(change @ Delete, Modify(new)) => {
stdx::never!();
*change = Create(new);
}
// shouldn't occur, but keep the Create
(prev @ Modify(_), new @ Create(_)) => *prev = new,
}
}
Entry::Vacant(v) => _ = v.insert(changed_file),
}
}

let mut workspace_structure_change = None;
// A file was added or deleted
let mut has_structure_changes = false;
let mut bytes = vec![];
let mut modified_rust_files = vec![];
for file in file_changes.into_values() {
for file in changed_files.into_values() {
let vfs_path = vfs.file_path(file.file_id);
if let Some(path) = vfs_path.as_path() {
has_structure_changes |= file.is_created_or_deleted();
Expand All @@ -326,16 +294,17 @@ impl GlobalState {
self.diagnostics.clear_native_for(file.file_id);
}

let text = if let vfs::Change::Create(v) | vfs::Change::Modify(v) = file.change {
String::from_utf8(v).ok().map(|text| {
// FIXME: Consider doing normalization in the `vfs` instead? That allows
// getting rid of some locking
let (text, line_endings) = LineEndings::normalize(text);
(text, line_endings)
})
} else {
None
};
let text =
if let vfs::Change::Create(v, _) | vfs::Change::Modify(v, _) = file.change {
String::from_utf8(v).ok().map(|text| {
// FIXME: Consider doing normalization in the `vfs` instead? That allows
// getting rid of some locking
let (text, line_endings) = LineEndings::normalize(text);
(text, line_endings)
})
} else {
None
};
// delay `line_endings_map` changes until we are done normalizing the text
// this allows delaying the re-acquisition of the write lock
bytes.push((file.file_id, text));
Expand Down
4 changes: 4 additions & 0 deletions crates/stdx/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ pub fn is_ci() -> bool {
option_env!("CI").is_some()
}

pub fn hash_once<Hasher: std::hash::Hasher + Default>(thing: impl std::hash::Hash) -> u64 {
std::hash::BuildHasher::hash_one(&std::hash::BuildHasherDefault::<Hasher>::default(), thing)
}

#[must_use]
#[allow(clippy::print_stderr)]
pub fn timeit(label: &'static str) -> impl Drop {
Expand Down
119 changes: 72 additions & 47 deletions crates/vfs/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,19 @@ pub mod loader;
mod path_interner;
mod vfs_path;

use std::{fmt, mem};
use std::{fmt, hash::BuildHasherDefault, mem};

use crate::path_interner::PathInterner;

pub use crate::{
anchored_path::{AnchoredPath, AnchoredPathBuf},
vfs_path::VfsPath,
};
use indexmap::{map::Entry, IndexMap};
pub use paths::{AbsPath, AbsPathBuf};

use rustc_hash::FxHasher;
use stdx::hash_once;
use tracing::{span, Level};

/// Handle to a file in [`Vfs`]
Expand Down Expand Up @@ -93,20 +96,13 @@ impl nohash_hasher::IsEnabled for FileId {}
pub struct Vfs {
interner: PathInterner,
data: Vec<FileState>,
// FIXME: This should be a HashMap<FileId, ChangeFile>
// right now we do a nasty deduplication in GlobalState::process_changes that would be a lot
// easier to handle here on insertion.
changes: Vec<ChangedFile>,
// The above FIXME would then also get rid of this probably
created_this_cycle: Vec<FileId>,
changes: IndexMap<FileId, ChangedFile, BuildHasherDefault<FxHasher>>,
}

#[derive(Copy, Clone, Debug, PartialEq, PartialOrd)]
pub enum FileState {
/// The file has been created this cycle.
Created,
/// The file exists.
Exists,
/// The file exists with the given content hash.
Exists(u64),
/// The file is deleted.
Deleted,
}
Expand All @@ -129,23 +125,23 @@ impl ChangedFile {
/// Returns `true` if the change is [`Create`](ChangeKind::Create) or
/// [`Delete`](Change::Delete).
pub fn is_created_or_deleted(&self) -> bool {
matches!(self.change, Change::Create(_) | Change::Delete)
matches!(self.change, Change::Create(_, _) | Change::Delete)
}

/// Returns `true` if the change is [`Create`](ChangeKind::Create).
pub fn is_created(&self) -> bool {
matches!(self.change, Change::Create(_))
matches!(self.change, Change::Create(_, _))
}

/// Returns `true` if the change is [`Modify`](ChangeKind::Modify).
pub fn is_modified(&self) -> bool {
matches!(self.change, Change::Modify(_))
matches!(self.change, Change::Modify(_, _))
}

pub fn kind(&self) -> ChangeKind {
match self.change {
Change::Create(_) => ChangeKind::Create,
Change::Modify(_) => ChangeKind::Modify,
Change::Create(_, _) => ChangeKind::Create,
Change::Modify(_, _) => ChangeKind::Modify,
Change::Delete => ChangeKind::Delete,
}
}
Expand All @@ -155,9 +151,9 @@ impl ChangedFile {
#[derive(Eq, PartialEq, Debug)]
pub enum Change {
/// The file was (re-)created
Create(Vec<u8>),
Create(Vec<u8>, u64),
/// The file was modified
Modify(Vec<u8>),
Modify(Vec<u8>, u64),
/// The file was deleted
Delete,
}
Expand All @@ -176,9 +172,7 @@ pub enum ChangeKind {
impl Vfs {
/// Id of the given path if it exists in the `Vfs` and is not deleted.
pub fn file_id(&self, path: &VfsPath) -> Option<FileId> {
self.interner
.get(path)
.filter(|&it| matches!(self.get(it), FileState::Exists | FileState::Created))
self.interner.get(path).filter(|&it| matches!(self.get(it), FileState::Exists(_)))
}

/// File path corresponding to the given `file_id`.
Expand All @@ -196,9 +190,7 @@ impl Vfs {
pub fn iter(&self) -> impl Iterator<Item = (FileId, &VfsPath)> + '_ {
(0..self.data.len())
.map(|it| FileId(it as u32))
.filter(move |&file_id| {
matches!(self.get(file_id), FileState::Exists | FileState::Created)
})
.filter(move |&file_id| matches!(self.get(file_id), FileState::Exists(_)))
.map(move |file_id| {
let path = self.interner.lookup(file_id);
(file_id, path)
Expand All @@ -217,41 +209,74 @@ impl Vfs {
let state = self.get(file_id);
let change_kind = match (state, contents) {
(FileState::Deleted, None) => return false,
(FileState::Deleted, Some(v)) => Change::Create(v),
(FileState::Exists | FileState::Created, None) => Change::Delete,
(FileState::Exists | FileState::Created, Some(v)) => Change::Modify(v),
};
self.data[file_id.0 as usize] = match change_kind {
Change::Create(_) => {
self.created_this_cycle.push(file_id);
FileState::Created
(FileState::Deleted, Some(v)) => {
let hash = hash_once::<FxHasher>(&*v);
Change::Create(v, hash)
}
(FileState::Exists(_), None) => Change::Delete,
(FileState::Exists(hash), Some(v)) => {
let new_hash = hash_once::<FxHasher>(&*v);
if new_hash == hash {
return false;
}
Change::Modify(v, new_hash)
}
// If the file got created this cycle, make sure we keep it that way even
// if a modify comes in
Change::Modify(_) if matches!(state, FileState::Created) => FileState::Created,
Change::Modify(_) => FileState::Exists,
Change::Delete => FileState::Deleted,
};

let mut set_data = |change_kind| {
self.data[file_id.0 as usize] = match change_kind {
&Change::Create(_, hash) | &Change::Modify(_, hash) => FileState::Exists(hash),
Change::Delete => FileState::Deleted,
};
};

let changed_file = ChangedFile { file_id, change: change_kind };
self.changes.push(changed_file);
match self.changes.entry(file_id) {
// two changes to the same file in one cycle, merge them appropriately
Entry::Occupied(mut o) => {
use Change::*;

match (&mut o.get_mut().change, changed_file.change) {
// newer `Delete` wins
(change, Delete) => *change = Delete,
// merge `Create` with `Create` or `Modify`
(Create(prev, old_hash), Create(new, new_hash) | Modify(new, new_hash)) => {
*prev = new;
*old_hash = new_hash;
}
// collapse identical `Modify`es
(Modify(prev, old_hash), Modify(new, new_hash)) => {
*prev = new;
*old_hash = new_hash;
}
// equivalent to `Modify`
(change @ Delete, Create(new, new_hash)) => {
*change = Modify(new, new_hash);
}
// shouldn't occur, but collapse into `Create`
(change @ Delete, Modify(new, new_hash)) => {
stdx::never!();
*change = Create(new, new_hash);
}
// shouldn't occur, but keep the Create
(prev @ Modify(_, _), new @ Create(_, _)) => *prev = new,
}
set_data(&o.get().change);
}
Entry::Vacant(v) => set_data(&v.insert(changed_file).change),
};

true
}

/// Drain and returns all the changes in the `Vfs`.
pub fn take_changes(&mut self) -> Vec<ChangedFile> {
let _p = span!(Level::INFO, "Vfs::take_changes").entered();
for file_id in self.created_this_cycle.drain(..) {
if self.data[file_id.0 as usize] == FileState::Created {
// downgrade the file from `Created` to `Exists` as the cycle is done
self.data[file_id.0 as usize] = FileState::Exists;
}
}
pub fn take_changes(&mut self) -> IndexMap<FileId, ChangedFile, BuildHasherDefault<FxHasher>> {
mem::take(&mut self.changes)
}

/// Provides a panic-less way to verify file_id validity.
pub fn exists(&self, file_id: FileId) -> bool {
matches!(self.get(file_id), FileState::Exists | FileState::Created)
matches!(self.get(file_id), FileState::Exists(_))
}

/// Returns the id associated with `path`
Expand Down

0 comments on commit 77c7886

Please sign in to comment.