From ce77da23a350d6ca47479b267910836edaa254d4 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 1 Nov 2024 11:27:58 -0400 Subject: [PATCH 1/6] Print errors by default and pretty print them --- Cargo.lock | 1 + crates/turbo-trace/Cargo.toml | 2 +- crates/turbo-trace/src/tracer.rs | 63 ++++++++++++++++++++------ crates/turborepo-lib/src/query/file.rs | 13 ++++-- 4 files changed, 61 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bba34a03ca13b..6eaad0a580924 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5223,6 +5223,7 @@ dependencies = [ "swc_atoms", "swc_eq_ignore_macros", "swc_visit", + "termcolor", "tracing", "unicode-width", "url", diff --git a/crates/turbo-trace/Cargo.toml b/crates/turbo-trace/Cargo.toml index 99f7fd50c3357..aa31a1a209b24 100644 --- a/crates/turbo-trace/Cargo.toml +++ b/crates/turbo-trace/Cargo.toml @@ -11,7 +11,7 @@ futures = { workspace = true } globwalk = { version = "0.1.0", path = "../turborepo-globwalk" } miette = { workspace = true, features = ["fancy"] } oxc_resolver = { version = "2.0.0" } -swc_common = { workspace = true, features = ["concurrent"] } +swc_common = { workspace = true, features = ["concurrent", "tty-emitter"] } swc_ecma_ast = { workspace = true } swc_ecma_parser = { workspace = true } swc_ecma_visit = { workspace = true } diff --git a/crates/turbo-trace/src/tracer.rs b/crates/turbo-trace/src/tracer.rs index 351c6ff0b0e76..8b90afddbf78a 100644 --- a/crates/turbo-trace/src/tracer.rs +++ b/crates/turbo-trace/src/tracer.rs @@ -2,17 +2,22 @@ use std::{collections::HashMap, sync::Arc}; use camino::Utf8PathBuf; use globwalk::WalkType; -use miette::{Diagnostic, NamedSource, SourceSpan}; +use miette::{Diagnostic, Report, SourceSpan}; use oxc_resolver::{ EnforceExtension, ResolveError, ResolveOptions, Resolver, TsconfigOptions, TsconfigReferences, }; -use swc_common::{comments::SingleThreadedComments, input::StringInput, FileName, SourceMap}; +use swc_common::{ + comments::SingleThreadedComments, + errors::{ColorConfig, Handler}, + input::StringInput, + FileName, SourceMap, +}; use swc_ecma_ast::EsVersion; use swc_ecma_parser::{lexer::Lexer, Capturing, EsSyntax, Parser, Syntax, TsSyntax}; use swc_ecma_visit::VisitWith; use thiserror::Error; use tokio::task::JoinSet; -use tracing::debug; +use tracing::{debug, error}; use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, PathError}; use crate::import_finder::ImportFinder; @@ -31,29 +36,52 @@ pub struct Tracer { import_type: ImportType, } -#[derive(Debug, Error, Diagnostic)] +#[derive(Clone, Debug, Error, Diagnostic)] pub enum TraceError { #[error("failed to parse file {}: {:?}", .0, .1)] ParseError(AbsoluteSystemPathBuf, swc_ecma_parser::error::Error), #[error("failed to read file: {0}")] FileNotFound(AbsoluteSystemPathBuf), #[error(transparent)] - PathEncoding(PathError), + PathEncoding(Arc), #[error("tracing a root file `{0}`, no parent found")] RootFile(AbsoluteSystemPathBuf), - #[error("failed to resolve import to `{path}`")] + #[error("failed to resolve import to `{import}` in `{file_path}`")] Resolve { - path: String, + import: String, + file_path: String, #[label("import here")] span: SourceSpan, #[source_code] - text: NamedSource, + text: String, }, #[error("failed to walk files")] - GlobError(#[from] globwalk::WalkError), + GlobError(Arc), +} + +impl TraceResult { + pub fn emit_errors(&self) { + let handler = Handler::with_tty_emitter( + ColorConfig::Auto, + true, + false, + Some(self.source_map.clone()), + ); + for error in &self.errors { + match error { + TraceError::ParseError(e) => { + e.clone().into_diagnostic(&handler).emit(); + } + e => { + eprintln!("{:?}", Report::new(e.clone())); + } + } + } + } } pub struct TraceResult { + source_map: Arc, pub errors: Vec, pub files: HashMap, } @@ -162,7 +190,7 @@ impl Tracer { match resolver.resolve(file_dir, import) { Ok(resolved) => { debug!("resolved {:?}", resolved); - match resolved.into_path_buf().try_into() { + match resolved.into_path_buf().try_into().map_err(Arc::new) { Ok(path) => files.push(path), Err(err) => { errors.push(TraceError::PathEncoding(err)); @@ -176,11 +204,14 @@ impl Tracer { Err(err) => { debug!("failed to resolve: {:?}", err); let (start, end) = source_map.span_to_char_offset(&source_file, *span); + let start = start as usize; + let end = end as usize; errors.push(TraceError::Resolve { - path: import.to_string(), - span: (start as usize, end as usize).into(), - text: NamedSource::new(file_path.to_string(), file_content.clone()), + import: import.to_string(), + file_path: file_path.to_string(), + span: SourceSpan::new(start.into(), (end - start).into()), + text: file_content.clone(), }); continue; } @@ -299,6 +330,7 @@ impl Tracer { } TraceResult { + source_map: self.source_map.clone(), files: seen, errors: self.errors, } @@ -322,8 +354,9 @@ impl Tracer { Ok(files) => files, Err(e) => { return TraceResult { + source_map: self.source_map.clone(), files: HashMap::new(), - errors: vec![e.into()], + errors: vec![TraceError::GlobError(Arc::new(e))], } } }; @@ -331,6 +364,7 @@ impl Tracer { let mut futures = JoinSet::new(); let resolver = Arc::new(self.create_resolver()); + let source_map = self.source_map.clone(); let shared_self = Arc::new(self); for file in files { @@ -380,6 +414,7 @@ impl Tracer { } TraceResult { + source_map, files: usages, errors, } diff --git a/crates/turborepo-lib/src/query/file.rs b/crates/turborepo-lib/src/query/file.rs index 2b1d06112122c..200a2c9ffc2d7 100644 --- a/crates/turborepo-lib/src/query/file.rs +++ b/crates/turborepo-lib/src/query/file.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use async_graphql::{Enum, Object, SimpleObject}; use camino::Utf8PathBuf; use itertools::Itertools; +use miette::SourceCode; use swc_ecma_ast::EsVersion; use swc_ecma_parser::{EsSyntax, Syntax, TsSyntax}; use turbo_trace::Tracer; @@ -108,9 +109,13 @@ impl From for TraceError { message: format!("failed to glob files: {}", err), ..Default::default() }, - turbo_trace::TraceError::Resolve { span, text, .. } => { + turbo_trace::TraceError::Resolve { + span, + text, + file_path, + .. + } => { let import = text - .inner() .read_span(&span, 1, 1) .ok() .map(|s| String::from_utf8_lossy(s.data()).to_string()); @@ -118,7 +123,7 @@ impl From for TraceError { TraceError { message, import, - path: Some(text.name().to_string()), + path: Some(file_path), start: Some(span.offset()), end: Some(span.offset() + span.len()), } @@ -204,6 +209,7 @@ impl File { } let mut result = tracer.trace(depth).await; + result.emit_errors(); // Remove the file itself from the result result.files.remove(&self.path); TraceResult::new(result, self.run.clone()) @@ -225,6 +231,7 @@ impl File { } let mut result = tracer.reverse_trace().await; + result.emit_errors(); // Remove the file itself from the result result.files.remove(&self.path); TraceResult::new(result, self.run.clone()) From c6902034fde16aec4c792d6b89d5cbad84964e56 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 1 Nov 2024 11:31:56 -0400 Subject: [PATCH 2/6] Small tweak to allow query strings without query keyword --- crates/turborepo-lib/src/commands/query.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/turborepo-lib/src/commands/query.rs b/crates/turborepo-lib/src/commands/query.rs index c3d79df1f9fe0..c845faccb84c4 100644 --- a/crates/turborepo-lib/src/commands/query.rs +++ b/crates/turborepo-lib/src/commands/query.rs @@ -80,7 +80,8 @@ pub async fn run( // If the arg starts with "query" or "mutation", and ends in a bracket, it's // likely a direct query If it doesn't, it's a file path, so we need to // read it - let query = if (trimmed_query.starts_with("query") || trimmed_query.starts_with("mutation")) + let query = if (trimmed_query.starts_with("query") + || trimmed_query.starts_with("mutation") | trimmed_query.starts_with('{')) && trimmed_query.ends_with('}') { query From 26f1e179d90dc67987d2a08015920f61f71f0392 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 1 Nov 2024 11:42:00 -0400 Subject: [PATCH 3/6] Update test --- crates/turborepo/tests/query.rs | 2 +- ...o_trace_get_`invalid.ts`_with_dependencies_(npm@10.5.0).snap | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/turborepo/tests/query.rs b/crates/turborepo/tests/query.rs index 4ea85f7df220e..46818a5775092 100644 --- a/crates/turborepo/tests/query.rs +++ b/crates/turborepo/tests/query.rs @@ -56,7 +56,7 @@ fn test_trace() -> Result<(), anyhow::Error> { "get `main.ts` with dependencies" => "query { file(path: \"main.ts\") { path, dependencies { files { items { path } } } } }", "get `button.tsx` with dependencies" => "query { file(path: \"button.tsx\") { path, dependencies { files { items { path } } } } }", "get `circular.ts` with dependencies" => "query { file(path: \"circular.ts\") { path dependencies { files { items { path } } } } }", - "get `invalid.ts` with dependencies" => "query { file(path: \"invalid.ts\") { path dependencies { files { items { path } } errors { items { message } } } } }", + "get `invalid.ts` with dependencies" => "query { file(path: \"invalid.ts\") { path dependencies { files { items { path } } errors { items { import } } } } }", "get `main.ts` with depth = 0" => "query { file(path: \"main.ts\") { path dependencies(depth: 1) { files { items { path } } } } }", "get `with_prefix.ts` with dependencies" => "query { file(path: \"with_prefix.ts\") { path dependencies { files { items { path } } } } }", "get `import_value_and_type.ts` with all dependencies" => "query { file(path: \"import_value_and_type.ts\") { path dependencies(importType: ALL) { files { items { path } } } } }", diff --git a/crates/turborepo/tests/snapshots/query__turbo_trace_get_`invalid.ts`_with_dependencies_(npm@10.5.0).snap b/crates/turborepo/tests/snapshots/query__turbo_trace_get_`invalid.ts`_with_dependencies_(npm@10.5.0).snap index c67ae3145901c..963d257ac7d21 100644 --- a/crates/turborepo/tests/snapshots/query__turbo_trace_get_`invalid.ts`_with_dependencies_(npm@10.5.0).snap +++ b/crates/turborepo/tests/snapshots/query__turbo_trace_get_`invalid.ts`_with_dependencies_(npm@10.5.0).snap @@ -23,7 +23,7 @@ expression: query_output "errors": { "items": [ { - "message": "failed to resolve import to `./non-existent-file.js`" + "import": "import foo from \"./non-existent-file.js\";\nimport { Button } from \"./button.tsx\";\n" } ] } From feffc005b2ab9a58719dc1f76aa27112dca7223c Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 1 Nov 2024 13:29:18 -0400 Subject: [PATCH 4/6] Fix up integration test --- turborepo-tests/integration/tests/turbo-trace.t | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/turborepo-tests/integration/tests/turbo-trace.t b/turborepo-tests/integration/tests/turbo-trace.t index 56a1f13dea51d..72356cc2e3a79 100644 --- a/turborepo-tests/integration/tests/turbo-trace.t +++ b/turborepo-tests/integration/tests/turbo-trace.t @@ -87,8 +87,7 @@ Setup } Trace file with invalid import - $ ${TURBO} query "query { file(path: \"invalid.ts\") { path dependencies { files { items { path } } errors { items { message } } } } }" - WARNING query command is experimental and may change in the future + $ ${TURBO} query "query { file(path: \"invalid.ts\") { path dependencies { files { items { path } } errors { items { message } } } } }" 2>/dev/null { "data": { "file": { @@ -110,7 +109,7 @@ Trace file with invalid import "errors": { "items": [ { - "message": "failed to resolve import to `./non-existent-file.js`" + "message": "failed to resolve import to `./non-existent-file.js` in `.*`" (re) } ] } From 9ee55151a46b0c3456be9b36f956bf2e922e4e90 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 1 Nov 2024 14:20:58 -0400 Subject: [PATCH 5/6] Fix compile error --- crates/turbo-trace/src/tracer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/turbo-trace/src/tracer.rs b/crates/turbo-trace/src/tracer.rs index 8b90afddbf78a..f8e3f2466262b 100644 --- a/crates/turbo-trace/src/tracer.rs +++ b/crates/turbo-trace/src/tracer.rs @@ -69,7 +69,7 @@ impl TraceResult { ); for error in &self.errors { match error { - TraceError::ParseError(e) => { + TraceError::ParseError(_, e) => { e.clone().into_diagnostic(&handler).emit(); } e => { From ca839d24946b8121f52333dcebfd6056b52bea53 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Mon, 4 Nov 2024 10:54:29 -0500 Subject: [PATCH 6/6] PR feedback --- crates/turbo-trace/Cargo.toml | 2 +- crates/turbo-trace/src/tracer.rs | 9 +++++++++ crates/turborepo-lib/src/commands/query.rs | 3 ++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/crates/turbo-trace/Cargo.toml b/crates/turbo-trace/Cargo.toml index aa31a1a209b24..8fb3ed3bd84c7 100644 --- a/crates/turbo-trace/Cargo.toml +++ b/crates/turbo-trace/Cargo.toml @@ -16,7 +16,7 @@ swc_ecma_ast = { workspace = true } swc_ecma_parser = { workspace = true } swc_ecma_visit = { workspace = true } thiserror = { workspace = true } -tokio = { workspace = true } +tokio = { workspace = true, features = ["full"] } tracing = { workspace = true } tracing-subscriber = { workspace = true } turbopath = { workspace = true } diff --git a/crates/turbo-trace/src/tracer.rs b/crates/turbo-trace/src/tracer.rs index f8e3f2466262b..5057c22befd1e 100644 --- a/crates/turbo-trace/src/tracer.rs +++ b/crates/turbo-trace/src/tracer.rs @@ -24,6 +24,11 @@ use crate::import_finder::ImportFinder; #[derive(Debug, Default)] pub struct SeenFile { + // We have to add these because of a Rust bug where dead code analysis + // doesn't work properly in multi-target crates + // (i.e. crates with both a binary and library) + // https://github.com/rust-lang/rust/issues/95513 + #[allow(dead_code)] pub ast: Option, } @@ -60,6 +65,7 @@ pub enum TraceError { } impl TraceResult { + #[allow(dead_code)] pub fn emit_errors(&self) { let handler = Handler::with_tty_emitter( ColorConfig::Auto, @@ -81,6 +87,7 @@ impl TraceResult { } pub struct TraceResult { + #[allow(dead_code)] source_map: Arc, pub errors: Vec, pub files: HashMap, @@ -88,6 +95,7 @@ pub struct TraceResult { /// The type of imports to trace. #[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[allow(dead_code)] pub enum ImportType { /// Trace all imports. All, @@ -118,6 +126,7 @@ impl Tracer { } } + #[allow(dead_code)] pub fn set_import_type(&mut self, import_type: ImportType) { self.import_type = import_type; } diff --git a/crates/turborepo-lib/src/commands/query.rs b/crates/turborepo-lib/src/commands/query.rs index c845faccb84c4..4a0a01363922c 100644 --- a/crates/turborepo-lib/src/commands/query.rs +++ b/crates/turborepo-lib/src/commands/query.rs @@ -81,7 +81,8 @@ pub async fn run( // likely a direct query If it doesn't, it's a file path, so we need to // read it let query = if (trimmed_query.starts_with("query") - || trimmed_query.starts_with("mutation") | trimmed_query.starts_with('{')) + || trimmed_query.starts_with("mutation") + || trimmed_query.starts_with('{')) && trimmed_query.ends_with('}') { query