Skip to content

Commit

Permalink
rework content graph errors to make them work with manifest diagnostics
Browse files Browse the repository at this point in the history
  • Loading branch information
@SpontanCombust
SpontanCombust committed Feb 28, 2024
1 parent c46144b commit 680fd83
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 82 deletions.
18 changes: 15 additions & 3 deletions crates/lsp/src/reporting/diagnostics.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::path::PathBuf;
use tower_lsp::lsp_types::{self as lsp, Position};
use tower_lsp::lsp_types as lsp;
use witcherscript_analysis::diagnostics::{Diagnostic, DiagnosticBody};
use witcherscript_project::{manifest::ManifestParseError, FileError};
use crate::Backend;
Expand Down Expand Up @@ -28,7 +28,7 @@ impl IntoLspDiagnostic for Diagnostic {
impl IntoLspDiagnostic for FileError<std::io::Error> {
fn into_lsp_diagnostic(self) -> lsp::Diagnostic {
lsp::Diagnostic {
range: lsp::Range::new(Position::new(0, 0), Position::new(0, 1)),
range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 1)),
severity: Some(lsp::DiagnosticSeverity::ERROR),
source: Some(Backend::SERVER_NAME.to_string()),
message: self.error.to_string(),
Expand All @@ -42,7 +42,7 @@ impl IntoLspDiagnostic for FileError<ManifestParseError> {
let error = self.error.as_ref();

let range = match error {
ManifestParseError::Io(_) => lsp::Range::new(Position::new(0, 0), Position::new(0, 1)),
ManifestParseError::Io(_) => lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 1)),
ManifestParseError::Toml { range, msg: _ } => range.clone(),
};

Expand All @@ -61,6 +61,18 @@ impl IntoLspDiagnostic for FileError<ManifestParseError> {
}
}

impl IntoLspDiagnostic for (String, lsp::Range) {
fn into_lsp_diagnostic(self) -> lsp::Diagnostic {
lsp::Diagnostic {
range: self.1,
severity: Some(lsp::DiagnosticSeverity::ERROR),
source: Some(Backend::SERVER_NAME.to_string()),
message: self.0,
..Default::default()
}
}
}


pub trait TryIntoUrl {
fn try_into_url(self) -> Result<lsp::Url, ()>;
Expand Down
27 changes: 18 additions & 9 deletions crates/lsp/src/workspace.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use tower_lsp::lsp_types as lsp;
use witcherscript_project::content::{ContentScanError, ProjectDirectory, find_content_in_directory};
use witcherscript_project::{Content, ContentRepositories};
use witcherscript_project::content_graph::ContentGraphError;
Expand Down Expand Up @@ -77,27 +76,37 @@ impl Backend {
self.log_warning(format!("Content scanning issue at {}: {}", err.path.display(), err.error)).await;
},
ContentScanError::ManifestParse(err) => {
let url = lsp::Url::from_file_path(&err.path.canonicalize().unwrap()).unwrap();
self.client.publish_diagnostics(url, vec![err.into_lsp_diagnostic()], None).await;
self.publish_diagnostics(err.path.clone(), [err.into_lsp_diagnostic()]).await;
},
ContentScanError::NotContent => {},
}
}

async fn report_content_graph_errors(&self, errors: Vec<ContentGraphError>) {
for err in errors {
let err_str = err.to_string();
match err {
ContentGraphError::Io(err) => {
self.log_warning(format!("Content scanning issue at {}: {}", err.path.display(), err.error)).await;
},
ContentGraphError::ManifestParse(err) => {
let url = lsp::Url::from_file_path(&err.path.canonicalize().unwrap()).unwrap();
self.client.publish_diagnostics(url, vec![err.into_lsp_diagnostic()], None).await;
self.publish_diagnostics(err.path.clone(), [err.into_lsp_diagnostic()]).await;
},
ContentGraphError::DependencyPathNotFound { content_path: _, manifest_path, manifest_range } => {
self.publish_diagnostics(manifest_path, [(err_str, manifest_range).into_lsp_diagnostic()]).await;
},
ContentGraphError::DependencyNameNotFound { content_name: _, manifest_path, manifest_range } => {
self.publish_diagnostics(manifest_path, [(err_str, manifest_range).into_lsp_diagnostic()]).await;
},
ContentGraphError::MultipleMatchingDependencies { content_name: _, manifest_path, manifest_range } => {
self.publish_diagnostics(manifest_path, [(err_str, manifest_range).into_lsp_diagnostic()]).await;
},
ContentGraphError::Content0NotFound => {
self.show_error_notification(err_str).await;
},
ContentGraphError::MultipleContent0Found => {
self.show_error_notification(err_str).await;
},
//TODO parsing TOML in such a way that later on you know where fragments are located and can publish diagnostics
ContentGraphError::ContentPathNotFound { path, origin } => todo!(),
ContentGraphError::ContentNameNotFound { name, origin } => todo!(),
ContentGraphError::MultipleMatchingContents { name, origin } => todo!(),
}
}
}
Expand Down
156 changes: 89 additions & 67 deletions crates/project/src/content_graph.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::collections::HashSet;
use std::path::{Path, PathBuf};
use thiserror::Error;
use lsp_types as lsp;
use crate::content::{make_content, ContentScanError, ProjectDirectory};
use crate::{Content, ContentRepositories, FileError};
use crate::manifest::{DependencyValue, ManifestParseError};
Expand All @@ -12,22 +13,34 @@ pub enum ContentGraphError {
Io(#[from] FileError<std::io::Error>),
#[error(transparent)]
ManifestParse(#[from] FileError<ManifestParseError>),
//TODO introduce error ranges
#[error("content could not be found in this path")]
ContentPathNotFound {
path: PathBuf,
origin: Option<PathBuf>
#[error("project dependency could not be found in this path")]
DependencyPathNotFound {
content_path: PathBuf,
/// Manifest from which this error originated
manifest_path: PathBuf,
// Location in the manifest where the path is present
manifest_range: lsp::Range
},
#[error("content could not be found with this name")]
ContentNameNotFound {
name: String,
origin: Option<PathBuf>
#[error("project dependency could not be found with this name")]
DependencyNameNotFound {
content_name: String,
/// Manifest from which this error originated
manifest_path: PathBuf,
// Location in the manifest where the name is present
manifest_range: lsp::Range
},
#[error("there are multiple matching contents for this content name")]
MultipleMatchingContents {
name: String,
origin: Option<PathBuf>
}
#[error("there are multiple matching dependencies with this name for this project")]
MultipleMatchingDependencies {
content_name: String,
/// Manifest from which this error originated
manifest_path: PathBuf,
// Location in the manifest where the name is present
manifest_range: lsp::Range
},
#[error("content0 scripts could not be found")]
Content0NotFound,
#[error("found more than one instance of content0 scripts")]
MultipleContent0Found
}


Expand Down Expand Up @@ -113,7 +126,7 @@ impl ContentGraph {
}

// Proceed with dependency building only if content0 is available
match self.get_node_index_by_name("content0", None) {
match self.get_node_index_by_name("content0") {
Ok(content0_idx) => {
// Now visit each of workspace content nodes to check for their dependencies.
let mut visited = HashSet::new();
Expand All @@ -123,8 +136,12 @@ impl ContentGraph {
}
}
},
Err(err) => {
self.errors.push(err);
Err(content0_count) => {
if content0_count == 0 {
self.errors.push(ContentGraphError::Content0NotFound);
} else {
self.errors.push(ContentGraphError::MultipleContent0Found);
}
},
}

Expand All @@ -144,14 +161,14 @@ impl ContentGraph {

/// Visits all content nodes that are dependencies to the specified content.
pub fn walk_dependencies(&self, content: &impl Content, visitor: impl FnMut(usize)) {
if let Ok(idx) = self.get_node_index_by_path(content.path(), None) {
if let Some(idx) = self.get_node_index_by_path(content.path()) {
self.walk_by_index(idx, GraphEdgeDirection::Dependencies, visitor);
}
}

/// Visits all content nodes that are dependants to the specified content.
pub fn walk_dependants(&self, content: &impl Content, visitor: impl FnMut(usize)) {
if let Ok(idx) = self.get_node_index_by_path(content.path(), None) {
if let Some(idx) = self.get_node_index_by_path(content.path()) {
self.walk_by_index(idx, GraphEdgeDirection::Dependants, visitor);
}
}
Expand All @@ -160,23 +177,16 @@ impl ContentGraph {

/// Returns index of the node if it was inserted successfully
fn create_node_for_content(&mut self, content: Box<dyn Content>, in_repository: bool, in_workspace: bool) {
if content.path().exists() {
if self.get_node_index_by_path(content.path(), None).is_ok() {
// node has already been made for this content
return;
}

self.insert_node(GraphNode {
content,
in_workspace,
in_repository,
});
} else {
self.errors.push(ContentGraphError::ContentPathNotFound {
path: content.path().to_path_buf(),
origin: None
});
if self.get_node_index_by_path(content.path()).is_some() {
// node has already been made for this content
return;
}

self.insert_node(GraphNode {
content,
in_workspace,
in_repository,
});
}

fn link_dependencies(&mut self, node_idx: usize, content0_idx: usize, visited: &mut HashSet<usize>) {
Expand All @@ -190,10 +200,10 @@ impl ContentGraph {
for entry in dependencies.into_iter() {
match entry.value.inner() {
DependencyValue::FromRepo(active) => {
self.link_dependencies_value_from_repo(node_idx, content0_idx, visited, &entry.name, *active);
self.link_dependencies_value_from_repo(node_idx, content0_idx, visited, &entry.name, entry.name.range(), *active);
},
DependencyValue::FromPath { path } => {
self.link_dependencies_value_from_path(node_idx, content0_idx, visited, path);
self.link_dependencies_value_from_path(node_idx, content0_idx, visited, path, entry.value.range());
},
}
}
Expand All @@ -208,17 +218,30 @@ impl ContentGraph {
node_idx: usize,
content0_idx: usize,
visited: &mut HashSet<usize>,
dependency_name: &str,
dependency_name: &str,
dependency_name_range: &lsp::Range,
active: bool
) {
if active {
match self.get_node_index_by_name(&dependency_name, Some(self.nodes[node_idx].content.path())) {
match self.get_node_index_by_name(&dependency_name) {
Ok(dep_idx) => {
self.insert_edge(GraphEdge { dependant_idx: node_idx, dependency_idx: dep_idx });
self.link_dependencies(dep_idx, content0_idx, visited);
},
Err(err) => {
self.errors.push(err);
Err(dep_count) => {
if dep_count == 0 {
self.errors.push(ContentGraphError::DependencyNameNotFound {
content_name: dependency_name.to_string(),
manifest_path: self.nodes[node_idx].content.path().to_path_buf(),
manifest_range: dependency_name_range.clone()
});
} else {
self.errors.push(ContentGraphError::MultipleMatchingDependencies {
content_name: dependency_name.to_string(),
manifest_path: self.nodes[node_idx].content.path().to_path_buf(),
manifest_range: dependency_name_range.clone()
});
}
}
}
}
Expand All @@ -228,7 +251,8 @@ impl ContentGraph {
node_idx: usize,
content0_idx: usize,
visited: &mut HashSet<usize>,
dependency_path: &Path
dependency_path: &Path,
dependency_path_range: &lsp::Range
) {
let dependant_path = self.nodes[node_idx].content.path().to_path_buf();
let final_dependency_path = if dependency_path.is_absolute() {
Expand All @@ -239,7 +263,7 @@ impl ContentGraph {

match final_dependency_path {
Ok(dep_path) => {
if let Ok(dep_idx) = self.get_node_index_by_path(&dep_path, None) {
if let Some(dep_idx) = self.get_node_index_by_path(&dep_path) {
self.insert_edge(GraphEdge { dependant_idx: node_idx, dependency_idx: dep_idx });
self.link_dependencies(dep_idx, content0_idx, visited);
} else {
Expand All @@ -263,9 +287,10 @@ impl ContentGraph {
self.errors.push(ContentGraphError::ManifestParse(err));
},
ContentScanError::NotContent => {
self.errors.push(ContentGraphError::ContentPathNotFound {
path: dep_path,
origin: Some(dependant_path)
self.errors.push(ContentGraphError::DependencyPathNotFound {
content_path: dep_path,
manifest_path: dependant_path.to_path_buf(),
manifest_range: dependency_path_range.clone()
})
},
}
Expand All @@ -274,16 +299,18 @@ impl ContentGraph {
}
},
Err(_) => {
self.errors.push(ContentGraphError::ContentPathNotFound {
path: dependency_path.to_path_buf(),
origin: Some(dependant_path)
self.errors.push(ContentGraphError::DependencyPathNotFound {
content_path: dependency_path.to_path_buf(),
manifest_path: dependant_path.to_path_buf(),
manifest_range: dependency_path_range.clone()
})
}
}
}




/// Returns the index of this node
fn insert_node(&mut self, node: GraphNode) -> usize {
self.nodes.push(node);
Expand All @@ -292,7 +319,7 @@ impl ContentGraph {

/// Changes node indices. Be aware!
fn remove_node_by_path(&mut self, content_path: &Path) {
if let Ok(target_idx) = self.get_node_index_by_path(content_path, None) {
if let Some(target_idx) = self.get_node_index_by_path(content_path) {
// first remove all edges that mention this node
self.edges.retain(|edge| edge.dependant_idx != target_idx && edge.dependency_idx != target_idx);

Expand Down Expand Up @@ -321,39 +348,34 @@ impl ContentGraph {
}
}

fn get_node_index_by_path(&self, path: &Path, dependant: Option<&Path>) -> Result<usize, ContentGraphError> {
fn get_node_index_by_path(&self, path: &Path) -> Option<usize> {
for (i, n) in self.nodes.iter().enumerate() {
if n.content.path() == path {
return Ok(i)
return Some(i)
}
}

Err(ContentGraphError::ContentPathNotFound {
path: path.to_path_buf(),
origin: dependant.map(|p| p.to_path_buf())
})
None
}

fn get_node_index_by_name(&self, name: &str, dependant: Option<&Path>) -> Result<usize, ContentGraphError> {
/// If there is just one content with the name returns Ok with the index.
/// Otherwise returns Err with the number of contents encountered with that name.
/// So if it wasn't found returns Err(0) or if more than one with than name was found returns Err(2) for example.
fn get_node_index_by_name(&self, name: &str) -> Result<usize, usize> {
let mut candidates = Vec::new();
for (i, n) in self.nodes.iter().enumerate() {
if n.content.content_name() == name {
candidates.push(i);
}
}

if candidates.len() == 0 {
Err(ContentGraphError::ContentNameNotFound {
name: name.to_string(),
origin: dependant.map(|p| p.to_path_buf())
})
} else if candidates.len() == 1 {
let candidates_len = candidates.len();
if candidates_len == 0 {
Err(0)
} else if candidates_len == 1 {
Ok(candidates[0])
} else {
Err(ContentGraphError::MultipleMatchingContents {
name: name.to_string(),
origin: dependant.map(|p| p.to_path_buf())
})
Err(candidates_len)
}
}

Expand Down
Loading

0 comments on commit 680fd83

Please sign in to comment.