Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: introduce diagnostic for private types referenced by public types #362

Merged
merged 3 commits into from
Oct 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benches/doc_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ async fn parse_with_reexports() -> Vec<DocNode> {
},
)
.await;
DocParser::new(graph, false, analyzer.as_capturing_parser())
DocParser::new(&graph, false, analyzer.as_capturing_parser())
.unwrap()
.parse_with_reexports(&root)
.unwrap()
Expand Down
2 changes: 1 addition & 1 deletion examples/ddoc/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ async fn run() -> anyhow::Result<()> {
},
)
.await;
let parser = DocParser::new(graph, private, analyzer.as_capturing_parser())?;
let parser = DocParser::new(&graph, private, analyzer.as_capturing_parser())?;
let mut doc_nodes = parser.parse_with_reexports(&source_file)?;

doc_nodes.retain(|doc_node| doc_node.kind != DocNodeKind::Import);
Expand Down
2 changes: 1 addition & 1 deletion src/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ async fn inner_doc(
)
.await;
let entries =
DocParser::new(graph, include_all, analyzer.as_capturing_parser())?
DocParser::new(&graph, include_all, analyzer.as_capturing_parser())?
.parse_with_reexports(&root_specifier)?;
let serializer =
serde_wasm_bindgen::Serializer::new().serialize_maps_as_objects(true);
Expand Down
20 changes: 19 additions & 1 deletion src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub enum DocNodeKind {
Import,
}

#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, Hash)]
pub struct Location {
pub filename: String,
/// The 1-indexed display line.
Expand All @@ -34,6 +34,24 @@ pub struct Location {
pub col: usize,
}

impl Ord for Location {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
match self.filename.cmp(&other.filename) {
core::cmp::Ordering::Equal => match self.line.cmp(&other.line) {
core::cmp::Ordering::Equal => self.col.cmp(&other.col),
ord => ord,
},
ord => ord,
}
}
}

impl PartialOrd for Location {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

#[derive(Debug, Serialize, Deserialize, Clone)]
#[serde(rename_all = "camelCase")]
pub enum ReexportKind {
Expand Down
84 changes: 67 additions & 17 deletions src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,33 @@ use deno_graph::ModuleGraph;
use deno_graph::ModuleSpecifier;

use std::borrow::Cow;
use std::cell::RefCell;
use std::collections::HashMap;
use std::collections::HashSet;
use std::error::Error;
use std::fmt;

#[derive(Debug, Clone)]
pub enum DocDiagnosticKind {
PrivateTypeRef,
}

impl std::fmt::Display for DocDiagnosticKind {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
DocDiagnosticKind::PrivateTypeRef => {
f.write_str("Type is not exported, but referenced by an exported type.")
}
}
}
}

#[derive(Debug, Clone)]
pub struct DocDiagnostic {
pub location: Location,
pub kind: DocDiagnosticKind,
}

#[derive(Debug)]
pub enum DocError {
Resolve(String),
Expand Down Expand Up @@ -100,17 +122,17 @@ struct Import {
}

pub struct DocParser<'a> {
pub graph: ModuleGraph,
pub private: bool,
pub parser: CapturingModuleParser<'a>,
pub root_symbol: deno_graph::type_tracer::RootSymbol,
graph: &'a ModuleGraph,
private: bool,
root_symbol: deno_graph::type_tracer::RootSymbol,
private_types_in_public: RefCell<HashSet<Location>>,
}

impl<'a> DocParser<'a> {
pub fn new(
graph: ModuleGraph,
graph: &'a ModuleGraph,
private: bool,
parser: CapturingModuleParser<'a>,
parser: CapturingModuleParser,
) -> Result<Self, anyhow::Error> {
struct NullTypeTraceHandler;

Expand All @@ -123,7 +145,7 @@ impl<'a> DocParser<'a> {
}

let root_symbol = deno_graph::type_tracer::trace_public_types(
&graph,
graph,
&graph.roots,
&parser,
&NullTypeTraceHandler,
Expand All @@ -132,11 +154,25 @@ impl<'a> DocParser<'a> {
Ok(DocParser {
graph,
private,
parser,
root_symbol,
private_types_in_public: Default::default(),
})
}

/// Gets diagnostics found during any of the previous parses.
pub fn diagnostics(&self) -> Vec<DocDiagnostic> {
let private_types_in_public = self.private_types_in_public.borrow();
let mut diagnostics = Vec::with_capacity(private_types_in_public.len());
for location in private_types_in_public.iter() {
diagnostics.push(DocDiagnostic {
location: location.clone(),
kind: DocDiagnosticKind::PrivateTypeRef,
});
}
diagnostics.sort_by(|a, b| a.location.cmp(&b.location));
diagnostics
}

/// Parses a module into a list of exported items,
/// as well as a list of reexported items which need to be fetched from other modules.
pub fn parse_module(
Expand Down Expand Up @@ -232,11 +268,11 @@ impl<'a> DocParser<'a> {
let module_doc = self.parse_module(&module.specifier)?;
let mut flattened_docs = Vec::new();
let module_symbol = self.get_module_symbol(&module.specifier)?;
let exports = module_symbol.exports(&self.graph, &self.root_symbol);
let exports = module_symbol.exports(self.graph, &self.root_symbol);
for (export_name, (export_module, export_symbol_id)) in exports {
let export_symbol = export_module.symbol(export_symbol_id).unwrap();
let definitions = self.root_symbol.go_to_definitions(
&self.graph,
self.graph,
export_module,
export_symbol,
);
Expand Down Expand Up @@ -550,7 +586,7 @@ impl<'a> DocParser<'a> {
handled_symbols.insert(*export_symbol_id);
let export_symbol = module_symbol.symbol(*export_symbol_id).unwrap();
let definitions = self.root_symbol.go_to_definitions(
&self.graph,
self.graph,
ModuleSymbolRef::Esm(module_symbol),
export_symbol,
);
Expand Down Expand Up @@ -580,17 +616,24 @@ impl<'a> DocParser<'a> {
continue; // already handled
}
let child_symbol = module_symbol.symbol(child_id).unwrap();
if child_symbol.is_public() || is_ambient || self.private {
let is_public = child_symbol.is_public();
if is_public || is_ambient || self.private {
for decl in child_symbol.decls() {
if let Some(node) = decl.maybe_node() {
let is_declared =
is_ambient && self.get_declare_for_symbol_node(node);
if child_symbol.is_public() || is_declared || self.private {
if is_public || is_declared || self.private {
if let Some(mut doc_node) = self.get_doc_for_symbol_node_ref(
module_symbol,
child_symbol,
node,
) {
if is_public {
self
.private_types_in_public
.borrow_mut()
.insert(doc_node.location.clone());
}
doc_node.declaration_kind = if is_declared {
DeclarationKind::Declare
} else {
Expand Down Expand Up @@ -904,12 +947,12 @@ impl<'a> DocParser<'a> {
}

let mut handled_symbols = HashSet::new();
let exports = module_symbol.exports(&self.graph, &self.root_symbol);
let exports = module_symbol.exports(self.graph, &self.root_symbol);
for (export_name, (export_module, export_symbol_id)) in &exports {
handled_symbols.insert(*export_symbol_id);
let export_symbol = export_module.symbol(*export_symbol_id).unwrap();
let definitions = self.root_symbol.go_to_definitions(
&self.graph,
self.graph,
*export_module,
export_symbol,
);
Expand Down Expand Up @@ -938,17 +981,24 @@ impl<'a> DocParser<'a> {
continue; // already handled
}
let child_symbol = module_symbol.symbol(child_id).unwrap();
if child_symbol.is_public() || is_ambient || self.private {
let is_public = child_symbol.is_public();
if is_public || is_ambient || self.private {
for decl in child_symbol.decls() {
if let Some(node) = decl.maybe_node() {
let is_declared =
is_ambient && self.get_declare_for_symbol_node(node);
if child_symbol.is_public() || is_declared || self.private {
if is_public || is_declared || self.private {
if let Some(mut doc_node) = self.get_doc_for_symbol_node_ref(
module_symbol,
child_symbol,
node,
) {
if is_public {
self
.private_types_in_public
.borrow_mut()
.insert(doc_node.location.clone());
}
doc_node.declaration_kind = if is_declared {
DeclarationKind::Declare
} else {
Expand Down
Loading