From 17d761252fbc63d9fbda46999b25607beae52f94 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Fri, 26 Apr 2024 16:31:20 -0400 Subject: [PATCH] Improve reporting for doc links problems --- Cargo.lock | 2 + crates/compiler/load_internal/src/file.rs | 1 + crates/compiler/load_internal/src/module.rs | 8 + crates/compiler/module/src/ident.rs | 18 ++- crates/docs/Cargo.toml | 3 + crates/docs/src/lib.rs | 157 +++++++++++++++++--- 6 files changed, 164 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f8728080e1..ecc5b2ee0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2502,12 +2502,14 @@ dependencies = [ "roc_module", "roc_packaging", "roc_parse", + "roc_problem", "roc_region", "roc_reporting", "roc_solve", "roc_target", "roc_types", "snafu", + "ven_pretty", ] [[package]] diff --git a/crates/compiler/load_internal/src/file.rs b/crates/compiler/load_internal/src/file.rs index 4977409590..76251358bf 100644 --- a/crates/compiler/load_internal/src/file.rs +++ b/crates/compiler/load_internal/src/file.rs @@ -3381,6 +3381,7 @@ fn finish( LoadedModule { module_id: state.root_id, + filename: state.root_path, interns, solved, can_problems: state.module_cache.can_problems, diff --git a/crates/compiler/load_internal/src/module.rs b/crates/compiler/load_internal/src/module.rs index a18e76f163..28ad33cf7b 100644 --- a/crates/compiler/load_internal/src/module.rs +++ b/crates/compiler/load_internal/src/module.rs @@ -30,6 +30,7 @@ use std::time::{Duration, Instant}; #[derive(Debug)] pub struct LoadedModule { pub module_id: ModuleId, + pub filename: PathBuf, pub interns: Interns, pub solved: Solved, pub can_problems: MutMap>, @@ -54,6 +55,13 @@ pub struct LoadedModule { } impl LoadedModule { + /// Infer the filename for the given ModuleId, based on this root module's filename. + pub fn filename(&self, module_id: ModuleId) -> PathBuf { + let module_name = self.interns.module_name(module_id); + + module_name.filename(&self.filename) + } + pub fn total_problems(&self) -> usize { let mut total = 0; diff --git a/crates/compiler/module/src/ident.rs b/crates/compiler/module/src/ident.rs index 17f9d79582..c82a737204 100644 --- a/crates/compiler/module/src/ident.rs +++ b/crates/compiler/module/src/ident.rs @@ -1,5 +1,8 @@ pub use roc_ident::IdentStr; -use std::fmt::{self, Debug}; +use std::{ + fmt::{self, Debug}, + path::{Path, PathBuf}, +}; use crate::symbol::PQModuleName; @@ -45,6 +48,19 @@ impl<'a> QualifiedModuleName<'a> { #[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct ModuleName(IdentStr); +impl ModuleName { + /// Given the root module's path, infer this module's path based on its name. + pub fn filename(&self, root_filename: impl AsRef) -> PathBuf { + let mut answer = root_filename.as_ref().with_file_name(""); + + for part in self.split(".") { + answer = answer.join(part); + } + + answer.with_extension("roc") + } +} + impl std::ops::Deref for ModuleName { type Target = str; diff --git a/crates/docs/Cargo.toml b/crates/docs/Cargo.toml index b9c1e7f0c8..e5337c5373 100644 --- a/crates/docs/Cargo.toml +++ b/crates/docs/Cargo.toml @@ -21,6 +21,9 @@ roc_reporting = { path = "../reporting" } roc_solve = { path = "../compiler/solve" } roc_target = { path = "../compiler/roc_target" } roc_types = { path = "../compiler/types" } +roc_problem = { path = "../compiler/problem" } + +ven_pretty = { path = "../vendor/pretty" } bumpalo.workspace = true pulldown-cmark.workspace = true diff --git a/crates/docs/src/lib.rs b/crates/docs/src/lib.rs index 2d9b5dd175..75e698e6f7 100644 --- a/crates/docs/src/lib.rs +++ b/crates/docs/src/lib.rs @@ -13,6 +13,7 @@ use roc_packaging::cache::{self, RocCacheDir}; use roc_parse::ident::{parse_ident, Accessor, Ident}; use roc_parse::keyword; use roc_parse::state::State; +use roc_problem::Severity; use roc_region::all::Region; use std::fs; use std::path::{Path, PathBuf}; @@ -146,7 +147,7 @@ pub fn generate_docs_html(root_file: PathBuf, build_dir: &Path) { } // Write each package module's index.html file - for (_, module_docs) in exposed_module_docs.iter() { + for (module_id, module_docs) in exposed_module_docs.iter() { let module_name = module_docs.name.as_str(); let module_dir = build_dir.join(module_name.replace('.', "/").as_str()); @@ -164,8 +165,13 @@ pub fn generate_docs_html(root_file: PathBuf, build_dir: &Path) { ) .replace( "", - render_module_documentation(module_docs, &loaded_module, &all_exposed_symbols) - .as_str(), + render_module_documentation( + *module_id, + module_docs, + &loaded_module, + &all_exposed_symbols, + ) + .as_str(), ); fs::write(module_dir.join("index.html"), rendered_module) @@ -235,6 +241,7 @@ fn render_package_index(docs_by_module: &[(ModuleId, ModuleDocumentation)]) -> S } fn render_module_documentation( + module_id: ModuleId, module: &ModuleDocumentation, root_module: &LoadedModule, all_exposed_symbols: &VecSet, @@ -292,6 +299,7 @@ fn render_module_documentation( if let Some(docs) = &doc_def.docs { markdown_to_html( &mut buf, + &root_module.filename(module_id), all_exposed_symbols, &module.scope, docs, @@ -305,6 +313,7 @@ fn render_module_documentation( DocEntry::ModuleDoc(docs) => { markdown_to_html( &mut buf, + &root_module.filename(module_id), all_exposed_symbols, &module.scope, docs, @@ -314,6 +323,7 @@ fn render_module_documentation( DocEntry::DetachedDoc(docs) => { markdown_to_html( &mut buf, + &root_module.filename, all_exposed_symbols, &module.scope, docs, @@ -899,13 +909,20 @@ struct DocUrl { title: String, } +enum LinkProblem { + MalformedAutoLink, + AutoLinkIdentNotInScope, + AutoLinkNotExposed, + AutoLinkModuleNotImported, +} + fn doc_url<'a>( all_exposed_symbols: &VecSet, scope: &Scope, interns: &'a Interns, mut module_name: &'a str, ident: &str, -) -> DocUrl { +) -> Result { if module_name.is_empty() { // This is an unqualified lookup, so look for the ident // in scope! @@ -918,10 +935,7 @@ fn doc_url<'a>( module_name = symbol.module_string(interns); } Err(_) => { - // TODO return Err here - panic!( - "Tried to generate an automatic link in docs for symbol `{ident}`, but that symbol was not in scope in this module." - ); + return Err((format!("[{ident}]"), LinkProblem::AutoLinkIdentNotInScope)); } } } else { @@ -940,9 +954,10 @@ fn doc_url<'a>( // Note: You can do qualified lookups on your own module, e.g. // if I'm in the Foo module, I can do a `Foo.bar` lookup. else if !all_exposed_symbols.contains(&symbol) { - // TODO return Err here - panic!( - "Tried to generate an automatic link in docs for `{module_name}.{ident}`, but `{module_name}` does not expose `{ident}`."); + return Err(( + format!("[{module_name}.{ident}]"), + LinkProblem::AutoLinkNotExposed, + )); } // This is a valid symbol for this dependency, @@ -952,8 +967,10 @@ fn doc_url<'a>( // incorporate the package name into the link. } None => { - // TODO return Err here - panic!("Tried to generate a doc link for `{module_name}.{ident}` but the `{module_name}` module was not imported!"); + return Err(( + format!("[{module_name}.{ident}]"), + LinkProblem::AutoLinkModuleNotImported, + )); } } } @@ -967,14 +984,15 @@ fn doc_url<'a>( url.push('#'); url.push_str(ident); - DocUrl { + Ok(DocUrl { url, title: format!("Docs for {module_name}.{ident}"), - } + }) } fn markdown_to_html( buf: &mut String, + filename: &Path, all_exposed_symbols: &VecSet, scope: &Scope, markdown: &str, @@ -1010,20 +1028,33 @@ fn markdown_to_html( match iter.next() { Some(Accessor::RecordField(symbol_name)) if iter.next().is_none() => { - let DocUrl { url, title } = doc_url( + match doc_url( all_exposed_symbols, scope, &loaded_module.interns, module_name, symbol_name, - ); + ) { + Ok(DocUrl { url, title }) => Some((url.into(), title.into())), + Err((link_markdown, problem)) => { + report_markdown_link_problem( + loaded_module.module_id, + filename.to_path_buf(), + &link_markdown, + problem, + ); - Some((url.into(), title.into())) + None + } + } } _ => { - // This had record field access, - // e.g. [foo.bar] - which we - // can't create a doc link to! + report_markdown_link_problem( + loaded_module.module_id, + filename.to_path_buf(), + &format!("[{}]", link.reference), + LinkProblem::MalformedAutoLink, + ); None } } @@ -1031,17 +1062,36 @@ fn markdown_to_html( Ok((_, Ident::Tag(type_name), _)) => { // This looks like a tag name, but it could // be a type alias that's in scope, e.g. [I64] - let DocUrl { url, title } = doc_url( + match doc_url( all_exposed_symbols, scope, &loaded_module.interns, "", type_name, + ) { + Ok(DocUrl { url, title }) => Some((url.into(), title.into())), + Err((link_markdown, problem)) => { + report_markdown_link_problem( + loaded_module.module_id, + filename.to_path_buf(), + &link_markdown, + problem, + ); + + None + } + } + } + _ => { + report_markdown_link_problem( + loaded_module.module_id, + filename.to_path_buf(), + &format!("[{}]", link.reference), + LinkProblem::MalformedAutoLink, ); - Some((url.into(), title.into())) + None } - _ => None, } } _ => None, @@ -1137,3 +1187,62 @@ fn markdown_to_html( pulldown_cmark::html::push_html(buf, docs_parser.into_iter()); } + +/// TODO: this should be moved into Reporting, and the markdown checking +/// for docs should be part of `roc check`. Problems like these should +/// be reported as `roc check` warnings and included in the total count +/// of warnings at the end. +fn report_markdown_link_problem( + module_id: ModuleId, + filename: PathBuf, + link_markdown: &str, + problem: LinkProblem, +) { + use roc_reporting::report::{Report, RocDocAllocator, DEFAULT_PALETTE}; + use ven_pretty::DocAllocator; + + // Report parsing and canonicalization problems + let interns = Interns::default(); + let alloc = RocDocAllocator::new(&[], module_id, &interns); + + let report = { + const AUTO_LINK_TIP: &str = "Tip: When a link in square brackets doesn't have a URL immediately after it in parentheses, the part in square brackets needs to be the name of either an uppercase type in scope, or a lowercase value in scope. Then Roc will generate a link to its docs, if available."; + + let link_problem = match problem { + LinkProblem::MalformedAutoLink => alloc.stack([ + alloc.reflow("The part in square brackets is not a Roc type or value name that can be automatically linked to."), + alloc.reflow(AUTO_LINK_TIP), + ]), + LinkProblem::AutoLinkIdentNotInScope => alloc.stack([ + alloc.reflow("The name in square brackets was not found in scope."), + alloc.reflow(AUTO_LINK_TIP), + ]), + LinkProblem::AutoLinkNotExposed => alloc.stack([ + alloc.reflow("The name in square brackets is not exposed by the module where it's defined."), + alloc.reflow(AUTO_LINK_TIP), + ]), + LinkProblem::AutoLinkModuleNotImported => alloc.stack([ + alloc.reflow("The name in square brackets is not in scope because its module is not imported."), + alloc.reflow(AUTO_LINK_TIP), + ]) + }; + + let doc = alloc.stack([ + alloc.reflow("This link in a doc comment is invalid:"), + alloc.reflow(link_markdown).indent(4), + link_problem, + ]); + + Report { + filename, + doc, + title: "INVALID DOCS LINK".to_string(), + severity: Severity::Warning, + } + }; + + let palette = DEFAULT_PALETTE; + let mut buf = String::new(); + + report.render_color_terminal(&mut buf, &alloc, &palette); +}