Turbopack: Attribute missing module issues to the import site (vercel…

…/turborepo#6351) This attaches source information to `ResolvingIssues` created from esm asset references. Test Plan: Updated snapshot tests, which now include source location code frames. --------- Co-authored-by: Donny/강동윤 <[email protected]> Co-authored-by: Tobias Koppers <[email protected]>
vercel · Nov 9, 2023 · a21180f · a21180f
1 parent 8b93b31
commit a21180f
Show file tree

Hide file tree

Showing 19 changed files with 208 additions and 104 deletions.
diff --git a/crates/turbopack-cli-utils/src/issue.rs b/crates/turbopack-cli-utils/src/issue.rs
@@ -79,13 +79,11 @@ fn severity_to_style(severity: IssueSeverity) -> Style {
 
 fn format_source_content(source: &PlainIssueSource, formatted_issue: &mut String) {
     if let FileLinesContent::Lines(lines) = source.asset.content.lines_ref() {
-        let start_line = source.start.line;
-        let end_line = source.end.line;
-        let start_column = source.start.column;
-        let end_column = source.end.column;
-        let lines = lines.iter().map(|l| l.content.as_str());
-        let ctx = get_source_context(lines, start_line, start_column, end_line, end_column);
-        format_source_context_lines(&ctx, formatted_issue);
+        if let Some((start, end)) = source.range {
+            let lines = lines.iter().map(|l| l.content.as_str());
+            let ctx = get_source_context(lines, start.line, start.column, end.line, end.column);
+            format_source_context_lines(&ctx, formatted_issue);
+        }
     }
 }
 
@@ -143,23 +141,8 @@ pub fn format_issue(
         .replace("/./", "/")
         .replace("\\\\?\\", "");
     let category = &plain_issue.category;
-    let title = &plain_issue.title;
-
-    let mut styled_issue = if let Some(source) = &plain_issue.source {
-        let mut styled_issue = format!(
-            "{}:{}:{}  {}",
-            context_path,
-            source.start.line + 1,
-            source.start.column,
-            title.bold()
-        );
-        styled_issue.push('\n');
-        format_source_content(source, &mut styled_issue);
-        styled_issue
-    } else {
-        format!("{}", title.bold())
-    };
 
+    let mut styled_issue = style_issue_source(plain_issue, &context_path);
     let description = &plain_issue.description;
     if !description.is_empty() {
         writeln!(styled_issue, "\n{description}").unwrap();
@@ -395,27 +378,12 @@ impl IssueReporter for ConsoleUi {
             let context_path =
                 make_relative_to_cwd(&plain_issue.file_path, project_dir, current_dir);
             let category = &plain_issue.category;
-            let title = &plain_issue.title;
             let processing_path = &*plain_issue.processing_path;
             let severity_map = grouped_issues.entry(severity).or_default();
             let category_map = severity_map.entry(category.clone()).or_default();
             let issues = category_map.entry(context_path.to_string()).or_default();
 
-            let mut styled_issue = if let Some(source) = &plain_issue.source {
-                let mut styled_issue = format!(
-                    "{}:{}:{}  {}",
-                    context_path,
-                    source.start.line + 1,
-                    source.start.column,
-                    title.bold()
-                );
-                styled_issue.push('\n');
-                format_source_content(source, &mut styled_issue);
-                styled_issue
-            } else {
-                format!("{}", title.bold())
-            };
-
+            let mut styled_issue = style_issue_source(&plain_issue, &context_path);
             let description = &plain_issue.description;
             if !description.is_empty() {
                 writeln!(&mut styled_issue, "\n{description}")?;
@@ -570,3 +538,25 @@ fn show_all_message_with_shown_count(
         .bold()
     }
 }
+
+fn style_issue_source(plain_issue: &PlainIssue, context_path: &str) -> String {
+    let title = &plain_issue.title;
+
+    if let Some(source) = &plain_issue.source {
+        let mut styled_issue = match source.range {
+            Some((start, _)) => format!(
+                "{}:{}:{}  {}",
+                context_path,
+                start.line + 1,
+                start.column,
+                title.bold()
+            ),
+            None => format!("{}  {}", context_path, title.bold()),
+        };
+        styled_issue.push('\n');
+        format_source_content(source, &mut styled_issue);
+        styled_issue
+    } else {
+        format!("{}", title.bold())
+    }
+}
diff --git a/crates/turbopack-core/src/issue/mod.rs b/crates/turbopack-core/src/issue/mod.rs
@@ -4,7 +4,7 @@ pub mod resolve;
 pub mod unsupported_module;
 
 use std::{
-    cmp::Ordering,
+    cmp::{min, Ordering},
     fmt::{Display, Formatter},
     sync::Arc,
 };
@@ -404,39 +404,83 @@ impl CapturedIssues {
 #[derive(Clone, Debug)]
 pub struct LazyIssueSource {
     pub source: Vc<Box<dyn Source>>,
-    pub start: usize,
-    pub end: usize,
+    pub start_end: Option<(usize, usize)>,
 }
 
 #[turbo_tasks::value_impl]
 impl LazyIssueSource {
+    /// Create a [`LazyIssueSource`] from byte offsets given by an swc ast node
+    /// span.
+    ///
+    /// Arguments:
+    ///
+    /// * `source`: The source code in which to look up the byte offsets.
+    /// * `start`: The start index of the span. Must use **1-based** indexing.
+    /// * `end`: The end index of the span. Must use **1-based** indexing.
     #[turbo_tasks::function]
-    pub fn new(source: Vc<Box<dyn Source>>, start: usize, end: usize) -> Vc<Self> {
-        Self::cell(LazyIssueSource { source, start, end })
+    pub fn from_swc_offsets(source: Vc<Box<dyn Source>>, start: usize, end: usize) -> Vc<Self> {
+        match (start == 0, end == 0) {
+            (true, true) => Self::cell(LazyIssueSource {
+                source,
+                start_end: None,
+            }),
+            (false, false) => Self::cell(LazyIssueSource {
+                source,
+                start_end: Some((start - 1, end - 1)),
+            }),
+            (false, true) => Self::cell(LazyIssueSource {
+                source,
+                start_end: Some((start - 1, start - 1)),
+            }),
+            (true, false) => Self::cell(LazyIssueSource {
+                source,
+                start_end: Some((end - 1, end - 1)),
+            }),
+        }
     }
 
     #[turbo_tasks::function]
     pub async fn to_issue_source(self: Vc<Self>) -> Result<Vc<IssueSource>> {
         let this = &*self.await?;
-        Ok(IssueSource::from_byte_offset(
-            this.source,
-            this.start,
-            this.end,
-        ))
+        Ok(if let Some((start, end)) = this.start_end {
+            IssueSource::from_byte_offset(this.source, start, end)
+        } else {
+            IssueSource::from_source_only(this.source)
+        })
     }
 }
 
 #[turbo_tasks::value]
 #[derive(Clone, Debug)]
 pub struct IssueSource {
     source: Vc<Box<dyn Source>>,
-    start: SourcePos,
-    end: SourcePos,
+    range: Option<(SourcePos, SourcePos)>,
 }
 
 #[turbo_tasks::value_impl]
 impl IssueSource {
+    // Sometimes we only have the source file that causes an issue, not the
+    // exact location, such as as in some generated code.
+    #[turbo_tasks::function]
+    pub fn from_source_only(source: Vc<Box<dyn Source>>) -> Vc<Self> {
+        Self::cell(IssueSource {
+            source,
+            range: None,
+        })
+    }
+
     #[turbo_tasks::function]
+    /// Returns an `IssueSource` representing a span of code in the `source`.
+    /// Positions are derived from byte offsets and stored as lines and columns.
+    /// Requires a binary search of the source text to perform this.
+    ///
+    /// Arguments:
+    ///
+    /// * `source`: The source code in which to look up the byte offsets.
+    /// * `start`: Byte offset into the source that the text begins. 0-based
+    ///   index and inclusive.
+    /// * `end`: Byte offset into the source that the text ends. 0-based index
+    ///   and exclusive.
     pub async fn from_byte_offset(
         source: Vc<Box<dyn Source>>,
         start: usize,
@@ -452,27 +496,25 @@ impl IssueSource {
                             column: offset,
                         }
                     } else {
+                        let line = &lines[i - 1];
                         SourcePos {
                             line: i - 1,
-                            column: offset - lines[i - 1].bytes_offset,
+                            column: min(line.content.len(), offset - line.bytes_offset),
                         }
                     }
                 }
             }
         }
-        Ok(Self::cell(
-            if let FileLinesContent::Lines(lines) = &*source.content().lines().await? {
+        Ok(Self::cell(IssueSource {
+            source,
+            range: if let FileLinesContent::Lines(lines) = &*source.content().lines().await? {
                 let start = find_line_and_column(lines.as_ref(), start);
                 let end = find_line_and_column(lines.as_ref(), end);
-                IssueSource { source, start, end }
+                Some((start, end))
             } else {
-                IssueSource {
-                    source,
-                    start: SourcePos::default(),
-                    end: SourcePos::max(),
-                }
+                None
             },
-        ))
+        }))
     }
 }
 
@@ -512,8 +554,7 @@ fn hash_plain_issue(issue: &PlainIssue, hasher: &mut Xxh3Hash64Hasher, full: boo
         hasher.write_value(1_u8);
         // I'm assuming we don't need to hash the contents. Not 100% correct, but
         // probably 99%.
-        hasher.write_ref(&source.start);
-        hasher.write_ref(&source.end);
+        hasher.write_ref(&source.range);
     } else {
         hasher.write_value(0_u8);
     }
@@ -564,8 +605,7 @@ impl PlainIssue {
 #[derive(Clone, Debug)]
 pub struct PlainIssueSource {
     pub asset: ReadRef<PlainSource>,
-    pub start: SourcePos,
-    pub end: SourcePos,
+    pub range: Option<(SourcePos, SourcePos)>,
 }
 
 #[turbo_tasks::value_impl]
@@ -575,8 +615,7 @@ impl IssueSource {
         let this = self.await?;
         Ok(PlainIssueSource {
             asset: PlainSource::from_source(this.source).await?,
-            start: this.start,
-            end: this.end,
+            range: this.range,
         }
         .cell())
     }

diff --git a/crates/turbopack-core/src/source_pos.rs b/crates/turbopack-core/src/source_pos.rs
@@ -22,7 +22,9 @@ const U8_CR: u8 = 0x0D;
     DeterministicHash,
 )]
 pub struct SourcePos {
+    /// The line, 0-indexed.
     pub line: usize,
+    /// The byte index of the column, 0-indexed.
     pub column: usize,
 }
 

diff --git a/crates/turbopack-css/src/references/mod.rs b/crates/turbopack-css/src/references/mod.rs
@@ -133,7 +133,7 @@ impl<'a> VisitAstPath for ModuleReferencesVisitor<'a> {
             Request::parse(Value::new(src.to_string().into())),
             Vc::cell(as_parent_path(ast_path)),
             ImportAttributes::new_from_prelude(i).into(),
-            LazyIssueSource::new(
+            LazyIssueSource::from_swc_offsets(
                 Vc::upcast(self.source),
                 issue_span.lo.to_usize(),
                 issue_span.hi.to_usize(),
@@ -160,7 +160,7 @@ impl<'a> VisitAstPath for ModuleReferencesVisitor<'a> {
                 self.origin,
                 Request::parse(Value::new(src.to_string().into())),
                 Vc::cell(as_parent_path(ast_path)),
-                LazyIssueSource::new(
+                LazyIssueSource::from_swc_offsets(
                     Vc::upcast(self.source),
                     issue_span.lo.to_usize(),
                     issue_span.hi.to_usize(),

diff --git a/crates/turbopack-ecmascript-hmr-protocol/src/lib.rs b/crates/turbopack-ecmascript-hmr-protocol/src/lib.rs
@@ -124,6 +124,11 @@ pub struct Asset<'a> {
 #[derive(Serialize)]
 pub struct IssueSource<'a> {
     pub asset: Asset<'a>,
+    pub range: Option<IssueSourceRange>,
+}
+
+#[derive(Serialize)]
+pub struct IssueSourceRange {
     pub start: SourcePos,
     pub end: SourcePos,
 }
@@ -151,8 +156,9 @@ impl<'a> From<&'a PlainIssue> for Issue<'a> {
             asset: Asset {
                 path: &source.asset.ident,
             },
-            start: source.start,
-            end: source.end,
+            range: source
+                .range
+                .map(|(start, end)| IssueSourceRange { start, end }),
         });
 
         Issue {

diff --git a/crates/turbopack-ecmascript-runtime/js/src/dev/runtime/base/protocol.d.ts b/crates/turbopack-ecmascript-runtime/js/src/dev/runtime/base/protocol.d.ts
@@ -124,6 +124,10 @@ type SourcePos = {
 
 type IssueSource = {
   asset: IssueAsset;
+  range?: IssueSourceRange;
+};
+
+type IssueSourceRange = {
   start: SourcePos;
   end: SourcePos;
 };

diff --git a/crates/turbopack-ecmascript/benches/analyzer.rs b/crates/turbopack-ecmascript/benches/analyzer.rs
@@ -55,7 +55,7 @@ pub fn benchmark(c: &mut Criterion) {
                 let top_level_mark = Mark::new();
                 program.visit_mut_with(&mut resolver(unresolved_mark, top_level_mark, false));
 
-                let eval_context = EvalContext::new(&program, unresolved_mark);
+                let eval_context = EvalContext::new(&program, unresolved_mark, None);
                 let var_graph = create_graph(&program, &eval_context);
 
                 let input = BenchInput {

diff --git a/crates/turbopack-ecmascript/src/analyzer/graph.rs b/crates/turbopack-ecmascript/src/analyzer/graph.rs
@@ -12,6 +12,8 @@ use swc_core::{
         visit::{fields::*, VisitAstPath, VisitWithPath, *},
     },
 };
+use turbo_tasks::Vc;
+use turbopack_core::source::Source;
 
 use super::{ConstantNumber, ConstantValue, ImportMap, JsValue, ObjectPart, WellKnownFunctionKind};
 use crate::{analyzer::is_unresolved, utils::unparen};
@@ -263,10 +265,14 @@ pub struct EvalContext {
 }
 
 impl EvalContext {
-    pub fn new(module: &Program, unresolved_mark: Mark) -> Self {
+    pub fn new(
+        module: &Program,
+        unresolved_mark: Mark,
+        source: Option<Vc<Box<dyn Source>>>,
+    ) -> Self {
         Self {
             unresolved_mark,
-            imports: ImportMap::analyze(module),
+            imports: ImportMap::analyze(module, source),
         }
     }