From 7e58b59914d6da32b0e661de31a45abf48d66dfe Mon Sep 17 00:00:00 2001 From: Shunsuke Shibayama Date: Sat, 15 Jul 2023 22:52:23 +0900 Subject: [PATCH] fix: canonicalize path --- crates/els/rename.rs | 9 ++-- crates/erg_common/pathutil.rs | 4 +- crates/erg_compiler/module/global.rs | 2 +- crates/erg_compiler/module/graph.rs | 77 ++++++++++++++++++++------- crates/erg_compiler/module/promise.rs | 24 ++++++++- 5 files changed, 87 insertions(+), 29 deletions(-) diff --git a/crates/els/rename.rs b/crates/els/rename.rs index 637e7175b..45e09231e 100644 --- a/crates/els/rename.rs +++ b/crates/els/rename.rs @@ -2,6 +2,7 @@ use std::collections::HashMap; use std::path::PathBuf; use std::time::SystemTime; +use erg_common::pathutil::NormalizedPathBuf; use erg_common::traits::{Locational, Stream}; use erg_compiler::artifact::IncompleteArtifact; use erg_compiler::erg_parser::parse::Parsable; @@ -146,26 +147,26 @@ impl Server { /// self is __included__ pub fn dependencies_of(&self, uri: &NormalizedUrl) -> Vec { let graph = self.get_graph().unwrap(); - let path = util::uri_to_path(uri); + let path = NormalizedPathBuf::from(util::uri_to_path(uri)); graph.sort().unwrap(); let self_node = graph.get_node(&path).unwrap(); graph .ref_inner() .iter() .filter(|node| node.id == path || self_node.depends_on(&node.id)) - .map(|node| NormalizedUrl::new(Url::from_file_path(&node.id).unwrap())) + .map(|node| NormalizedUrl::new(Url::from_file_path(node.id.to_path_buf()).unwrap())) .collect() } /// self is __not included__ pub fn dependents_of(&self, uri: &NormalizedUrl) -> Vec { let graph = self.get_graph().unwrap(); - let path = util::uri_to_path(uri); + let path = NormalizedPathBuf::from(util::uri_to_path(uri)); graph .ref_inner() .iter() .filter(|node| node.depends_on(&path)) - .map(|node| NormalizedUrl::new(Url::from_file_path(&node.id).unwrap())) + .map(|node| NormalizedUrl::new(Url::from_file_path(node.id.to_path_buf()).unwrap())) .collect() } } diff --git a/crates/erg_common/pathutil.rs b/crates/erg_common/pathutil.rs index 61c94ebc7..f189dd930 100644 --- a/crates/erg_common/pathutil.rs +++ b/crates/erg_common/pathutil.rs @@ -10,7 +10,7 @@ use crate::normalize_path; /// `PathBuf` may give false equivalence decisions in non-case-sensitive file systems. /// Use this for dictionary keys, etc. /// See also: `els::util::NormalizedUrl` -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Default)] pub struct NormalizedPathBuf(PathBuf); impl> From

for NormalizedPathBuf { @@ -41,7 +41,7 @@ impl Deref for NormalizedPathBuf { impl NormalizedPathBuf { pub fn new(path: PathBuf) -> Self { - NormalizedPathBuf(normalize_path(path)) + NormalizedPathBuf(normalize_path(path.canonicalize().unwrap_or(path))) } } diff --git a/crates/erg_compiler/module/global.rs b/crates/erg_compiler/module/global.rs index e9d49dcfe..76c80027a 100644 --- a/crates/erg_compiler/module/global.rs +++ b/crates/erg_compiler/module/global.rs @@ -52,7 +52,7 @@ impl SharedCompilerResource { pub fn inherit(&self, path: PathBuf) -> Self { let mut _self = self.clone(); - _self.promises.path = path; + _self.promises.path = path.into(); _self } diff --git a/crates/erg_compiler/module/graph.rs b/crates/erg_compiler/module/graph.rs index ac4c37b87..0074855d5 100644 --- a/crates/erg_compiler/module/graph.rs +++ b/crates/erg_compiler/module/graph.rs @@ -1,10 +1,11 @@ use std::fmt; use std::path::{Path, PathBuf}; +use erg_common::pathutil::NormalizedPathBuf; +use erg_common::set; use erg_common::set::Set; use erg_common::shared::{MappedRwLockReadGuard, RwLockReadGuard, Shared}; use erg_common::tsort::{tsort, Graph, Node, TopoSortError}; -use erg_common::{normalize_path, set}; #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum IncRefError { @@ -18,7 +19,7 @@ impl IncRefError { } #[derive(Debug, Clone, Default)] -pub struct ModuleGraph(Graph); +pub struct ModuleGraph(Graph); impl fmt::Display for ModuleGraph { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -35,7 +36,7 @@ impl fmt::Display for ModuleGraph { } impl IntoIterator for ModuleGraph { - type Item = Node; + type Item = Node; type IntoIter = std::vec::IntoIter; fn into_iter(self) -> Self::IntoIter { @@ -48,20 +49,45 @@ impl ModuleGraph { Self(Graph::new()) } - pub fn get_node(&self, path: &Path) -> Option<&Node> { - let path = normalize_path(path.to_path_buf()); + pub fn get_node(&self, path: &Path) -> Option<&Node> { + let path = NormalizedPathBuf::new(path.to_path_buf()); self.0.iter().find(|n| n.id == path) } - fn parents(&self, path: &Path) -> Option<&Set> { - let path = normalize_path(path.to_path_buf()); + /// if `path` depends on `target`, returns `true`, else `false`. + /// if `path` not found, returns `false` + pub fn depends_on(&self, path: &Path, target: &Path) -> bool { + let path = NormalizedPathBuf::new(path.to_path_buf()); + let target = NormalizedPathBuf::new(target.to_path_buf()); + self.0 + .iter() + .find(|n| n.id == path) + .map(|n| n.depends_on.contains(&target)) + .unwrap_or(false) + } + + pub fn children(&self, path: &Path) -> Set { + let path = NormalizedPathBuf::new(path.to_path_buf()); + self.0 + .iter() + .filter(|n| n.depends_on.contains(&path)) + .map(|n| n.id.clone()) + .collect() + } + + fn parents(&self, path: &Path) -> Option<&Set> { + let path = NormalizedPathBuf::new(path.to_path_buf()); self.0.iter().find(|n| n.id == path).map(|n| &n.depends_on) } - pub fn ancestors(&self, path: &Path) -> Set { - let path = normalize_path(path.to_path_buf()); + /// ```erg + /// # a.er + /// b = import "b" + /// ``` + /// -> a: child, b: parent + pub fn ancestors(&self, path: &Path) -> Set { let mut ancestors = set! {}; - if let Some(parents) = self.parents(&path) { + if let Some(parents) = self.parents(path) { for parent in parents.iter() { ancestors.insert(parent.clone()); ancestors.extend(self.ancestors(parent)); @@ -71,7 +97,7 @@ impl ModuleGraph { } pub fn add_node_if_none(&mut self, path: &Path) { - let path = normalize_path(path.to_path_buf()); + let path = NormalizedPathBuf::new(path.to_path_buf()); if self.0.iter().all(|n| n.id != path) { let node = Node::new(path, (), set! {}); self.0.push(node); @@ -80,8 +106,8 @@ impl ModuleGraph { /// returns Err (and do nothing) if this operation makes a cycle pub fn inc_ref(&mut self, referrer: &Path, depends_on: PathBuf) -> Result<(), IncRefError> { - let referrer = normalize_path(referrer.to_path_buf()); - let depends_on = normalize_path(depends_on); + let referrer = NormalizedPathBuf::new(referrer.to_path_buf()); + let depends_on = NormalizedPathBuf::new(depends_on); if self.ancestors(&depends_on).contains(&referrer) && referrer != depends_on { return Err(IncRefError::CycleDetected); } @@ -96,7 +122,7 @@ impl ModuleGraph { Ok(()) } - pub fn iter(&self) -> impl Iterator> { + pub fn iter(&self) -> impl Iterator> { self.0.iter() } @@ -112,13 +138,13 @@ impl ModuleGraph { } pub fn remove(&mut self, path: &Path) { - let path = normalize_path(path.to_path_buf()); + let path = NormalizedPathBuf::new(path.to_path_buf()); self.0.retain(|n| n.id != path); } pub fn rename_path(&mut self, old: &Path, new: PathBuf) { - let old = normalize_path(old.to_path_buf()); - let new = normalize_path(new); + let old = NormalizedPathBuf::new(old.to_path_buf()); + let new = NormalizedPathBuf::new(new); for node in self.0.iter_mut() { if node.id == old { node.id = new.clone(); @@ -145,7 +171,7 @@ impl fmt::Display for SharedModuleGraph { } impl IntoIterator for SharedModuleGraph { - type Item = Node; + type Item = Node; type IntoIter = std::vec::IntoIter; fn into_iter(self) -> Self::IntoIter { @@ -158,7 +184,10 @@ impl SharedModuleGraph { Self(Shared::new(ModuleGraph::new())) } - pub fn get_node(&self, path: &Path) -> Option>> { + pub fn get_node( + &self, + path: &Path, + ) -> Option>> { if self.0.borrow().get_node(path).is_some() { Some(RwLockReadGuard::map(self.0.borrow(), |graph| { graph.get_node(path).unwrap() @@ -168,7 +197,15 @@ impl SharedModuleGraph { } } - pub fn ancestors(&self, path: &Path) -> Set { + pub fn depends_on(&self, path: &Path, target: &Path) -> bool { + self.0.borrow().depends_on(path, target) + } + + pub fn children(&self, path: &Path) -> Set { + self.0.borrow().children(path) + } + + pub fn ancestors(&self, path: &Path) -> Set { self.0.borrow().ancestors(path) } diff --git a/crates/erg_compiler/module/promise.rs b/crates/erg_compiler/module/promise.rs index f80462eb4..047d23959 100644 --- a/crates/erg_compiler/module/promise.rs +++ b/crates/erg_compiler/module/promise.rs @@ -68,7 +68,7 @@ impl Promise { #[derive(Debug, Clone, Default)] pub struct SharedPromises { graph: SharedModuleGraph, - pub(crate) path: PathBuf, + pub(crate) path: NormalizedPathBuf, promises: Shared>, } @@ -86,7 +86,7 @@ impl SharedPromises { pub fn new(graph: SharedModuleGraph, path: PathBuf) -> Self { Self { graph, - path, + path: NormalizedPathBuf::new(path), promises: Shared::new(Dict::new()), } } @@ -125,6 +125,26 @@ impl SharedPromises { Promise::Running { parent, handle }; return Ok(()); } + // Suppose A depends on B and C, and B depends on C. + // In this case, B must join C before A joins C. Otherwise, a deadlock will occur. + let children = self.graph.children(path); + for child in children.iter() { + if child == &self.path { + continue; + } else if self.graph.depends_on(&self.path, child) { + *self.promises.borrow_mut().get_mut(path).unwrap() = + Promise::Running { parent, handle }; + while self + .promises + .borrow() + .get(path) + .is_some_and(|p| !p.is_finished()) + { + std::thread::yield_now(); + } + return Ok(()); + } + } let res = handle.join(); *self.promises.borrow_mut().get_mut(path).unwrap() = Promise::Finished; res