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

refactor: unify the logic of span_to_location into trait SourcePosition #8640

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/rspack_core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ once_cell = { workspace = true }
paste = { workspace = true }
rayon = { workspace = true }
regex = { workspace = true }
ropey = { workspace = true }
rspack_ast = { workspace = true }
rspack_cacheable = { workspace = true }
rspack_collections = { workspace = true }
Expand Down
86 changes: 63 additions & 23 deletions crates/rspack_core/src/dependency/dependency_location.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::{

use derivative::Derivative;
use rspack_cacheable::cacheable;
use rspack_util::itoa;

/// Represents a range in a dependency, typically used for tracking the span of code in a source file.
/// It stores the start and end positions (as offsets) of the range, typically using base-0 indexing.
Expand Down Expand Up @@ -41,25 +42,16 @@ impl DependencyRange {

/// Converts the `DependencyRange` into a `DependencyLocation`.
/// The `source` parameter is an optional source map used to resolve the exact position in the source file.
pub fn to_loc(&self, source: Option<&Arc<dyn SourceLocation>>) -> DependencyLocation {
DependencyLocation::Real(match source {
Some(source) => {
let (start, end) = source.look_up_range_pos(self.start, self.end);

if start.line == end.line && start.column == end.column {
pub fn to_loc<T: AsLoc>(&self, source: Option<T>) -> Option<DependencyLocation> {
source
.and_then(|s| s.as_loc().look_up_range_pos(self.start, self.end))
.map(|(start, end)| {
DependencyLocation::Real(if start.line == end.line && start.column == end.column {
RealDependencyLocation::new(start, None)
} else {
RealDependencyLocation::new(start, Some(end))
}
}
None => RealDependencyLocation::new(
SourcePosition {
line: self.start as usize,
column: self.end as usize,
},
None,
),
})
})
})
}
}

Expand All @@ -85,17 +77,22 @@ impl fmt::Display for RealDependencyLocation {
write!(
f,
"{}:{}-{}",
self.start.line, self.start.column, end.column
itoa!(self.start.line),
itoa!(self.start.column),
itoa!(end.column)
)
} else {
write!(
f,
"{}:{}-{}:{}",
self.start.line, self.start.column, end.line, end.column
itoa!(self.start.line),
itoa!(self.start.column),
itoa!(end.line),
itoa!(end.column)
)
}
} else {
write!(f, "{}:{}", self.start.line, self.start.column)
write!(f, "{}:{}", itoa!(self.start.line), itoa!(self.start.column))
}
}
}
Expand Down Expand Up @@ -157,7 +154,7 @@ impl From<(u32, u32)> for SourcePosition {

/// Trait representing a source map that can resolve the positions of code ranges to source file positions.
pub trait SourceLocation: Send + Sync {
fn look_up_range_pos(&self, start: u32, end: u32) -> (SourcePosition, SourcePosition);
fn look_up_range_pos(&self, start: u32, end: u32) -> Option<(SourcePosition, SourcePosition)>;
}

impl Debug for dyn SourceLocation {
Expand All @@ -167,11 +164,11 @@ impl Debug for dyn SourceLocation {
}

impl SourceLocation for swc_core::common::SourceMap {
fn look_up_range_pos(&self, start: u32, end: u32) -> (SourcePosition, SourcePosition) {
fn look_up_range_pos(&self, start: u32, end: u32) -> Option<(SourcePosition, SourcePosition)> {
let lo = self.lookup_char_pos(swc_core::common::BytePos(start + 1));
let hi = self.lookup_char_pos(swc_core::common::BytePos(end + 1));

(
Some((
SourcePosition {
line: lo.line,
column: lo.col_display,
Expand All @@ -180,9 +177,52 @@ impl SourceLocation for swc_core::common::SourceMap {
line: hi.line,
column: hi.col_display,
},
)
))
}
}

impl SourceLocation for &str {
fn look_up_range_pos(&self, start: u32, end: u32) -> Option<(SourcePosition, SourcePosition)> {
Comment on lines +184 to +185
Copy link
Collaborator Author

@shulaoda shulaoda Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't implement that method for dyn rspack_sources::Source, so impl it for &str.

let r = ropey::Rope::from_str(self);
let start_char_offset = r.try_byte_to_char(start as usize).ok()?;
let end_char_offset = r.try_byte_to_char(end as usize).ok()?;

let start_line = r.char_to_line(start_char_offset);
let start_column = start_char_offset - r.line_to_char(start_line);
let end_line = r.char_to_line(end_char_offset);
let end_column = end_char_offset - r.line_to_char(end_line);

Some((
SourcePosition {
line: start_line + 1,
column: start_column,
},
SourcePosition {
line: end_line + 1,
column: end_column,
},
))
}
}

/// Type alias for a shared reference to a `SourceLocation` trait object, typically used for source maps.
pub type SharedSourceMap = Arc<dyn SourceLocation>;

pub trait AsLoc {
fn as_loc(&self) -> &dyn SourceLocation;
}

impl AsLoc for &Arc<dyn SourceLocation> {
#[inline]
fn as_loc(&self) -> &dyn SourceLocation {
self.as_ref()
}
}

impl AsLoc for &str {
#[inline]
fn as_loc(&self) -> &dyn SourceLocation {
let loc: &dyn SourceLocation = self;
loc
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl Dependency for CommonJsFullRequireDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn range(&self) -> Option<&DependencyRange> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl Dependency for CommonJsRequireDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn category(&self) -> &DependencyCategory {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ impl Dependency for RequireHeaderDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn could_affect_referencing_module(&self) -> rspack_core::AffectType {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ impl Dependency for RequireResolveHeaderDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn could_affect_referencing_module(&self) -> AffectType {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ impl Dependency for ESMExportExpressionDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn get_exports(&self, _mg: &ModuleGraph) -> Option<ExportsSpec> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ impl Dependency for ESMExportHeaderDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn dependency_type(&self) -> &DependencyType {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1053,7 +1053,7 @@ impl Dependency for ESMExportImportedSpecifierDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn range(&self) -> Option<&DependencyRange> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Dependency for ESMExportSpecifierDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn category(&self) -> &DependencyCategory {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ impl Dependency for ESMImportSideEffectDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn range(&self) -> Option<&DependencyRange> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ impl Dependency for ESMImportSpecifierDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn range(&self) -> Option<&DependencyRange> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ impl Dependency for ProvideDependency {
}

fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}

fn category(&self) -> &DependencyCategory {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl ESMAcceptDependency {
}

pub fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ impl ModuleArgumentDependency {
}

pub fn loc(&self) -> Option<DependencyLocation> {
Some(self.range.to_loc(self.source_map.as_ref()))
self.range.to_loc(self.source_map.as_ref())
}
}

Expand Down
44 changes: 8 additions & 36 deletions crates/rspack_plugin_javascript/src/parser_and_generator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,15 @@ use rspack_core::diagnostics::map_box_diagnostics_to_module_parse_diagnostics;
use rspack_core::rspack_sources::{BoxSource, ReplaceSource, Source, SourceExt};
use rspack_core::{
render_init_fragments, AsyncDependenciesBlockIdentifier, BuildMetaExportsType, ChunkGraph,
Compilation, DependenciesBlock, DependencyId, GenerateContext, Module, ModuleGraph, ModuleType,
ParseContext, ParseResult, ParserAndGenerator, SideEffectsBailoutItem, SourceType, SpanExt,
TemplateContext, TemplateReplaceSource,
Compilation, DependenciesBlock, DependencyId, DependencyRange, GenerateContext, Module,
ModuleGraph, ModuleType, ParseContext, ParseResult, ParserAndGenerator, SideEffectsBailoutItem,
SourceType, TemplateContext, TemplateReplaceSource,
};
use rspack_error::miette::Diagnostic;
use rspack_error::{DiagnosticExt, IntoTWithDiagnosticArray, Result, TWithDiagnosticArray};
use rspack_util::itoa;
use swc_core::common::comments::Comments;
use swc_core::common::input::SourceFileInput;
use swc_core::common::{FileName, Span, SyntaxContext};
use swc_core::common::{FileName, SyntaxContext};
use swc_core::ecma::ast;
use swc_core::ecma::parser::{lexer::Lexer, EsSyntax, Syntax};
use swc_node_comments::SwcComments;
Expand Down Expand Up @@ -241,7 +240,10 @@ impl ParserAndGenerator for JavaScriptParserAndGenerator {
.side_effects_item
.take()
.and_then(|item| -> Option<_> {
let msg = span_to_location(item.span, &source.source())?;
let source = source.source();
let msg = Into::<DependencyRange>::into(item.span)
.to_loc(Some(source.as_ref()))?
.to_string();
Some(SideEffectsBailoutItem { msg, ty: item.ty })
})
});
Expand Down Expand Up @@ -376,36 +378,6 @@ impl ParserAndGenerator for JavaScriptParserAndGenerator {
}
}

// Todo(shulaoda): check if this can be removed
fn span_to_location(span: Span, source: &str) -> Option<String> {
let r = ropey::Rope::from_str(source);
let start = span.real_lo();
let end = span.real_hi();
let start_char_offset = r.try_byte_to_char(start as usize).ok()?;
let start_line = r.char_to_line(start_char_offset);
let start_column = start_char_offset - r.line_to_char(start_line);

let end_char_offset = r.try_byte_to_char(end as usize).ok()?;
let end_line = r.char_to_line(end_char_offset);
let end_column = end_char_offset - r.line_to_char(end_line);
if start_line == end_line {
Some(format!(
"{}:{}-{}",
itoa!(start_line + 1),
itoa!(start_column),
itoa!(end_column)
))
} else {
Some(format!(
"{}:{}-{}:{}",
itoa!(start_line + 1),
itoa!(start_column),
itoa!(end_line + 1),
itoa!(end_column)
))
}
}

fn remove_bom(s: Arc<dyn Source>) -> Arc<dyn Source> {
if s.source().starts_with('\u{feff}') {
let mut s = ReplaceSource::new(s);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,7 @@ impl AMDRequireDependenciesBlockParserPlugin {
));

let source_map: SharedSourceMap = parser.source_map.clone();
let block_loc =
Some(Into::<DependencyRange>::into(call_expr.span).to_loc(Some(source_map).as_ref()));
let block_loc = Into::<DependencyRange>::into(call_expr.span).to_loc(Some(&source_map));

if call_expr.args.len() == 1 {
let mut block_deps: Vec<BoxDependency> = vec![dep];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ impl JavascriptParserPlugin for ImportParserPlugin {
let source_map: SharedSourceMap = parser.source_map.clone();
let mut block = AsyncDependenciesBlock::new(
*parser.module_identifier,
Some(Into::<DependencyRange>::into(node.span).to_loc(Some(source_map).as_ref())),
Into::<DependencyRange>::into(node.span).to_loc(Some(&source_map)),
None,
vec![dep],
Some(param.string().clone()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ impl JavascriptParserPlugin for RequireEnsureDependenciesBlockParserPlugin {
let source_map: SharedSourceMap = parser.source_map.clone();
let mut block = AsyncDependenciesBlock::new(
*parser.module_identifier,
Some(Into::<DependencyRange>::into(expr.span).to_loc(Some(source_map).as_ref())),
Into::<DependencyRange>::into(expr.span).to_loc(Some(&source_map)),
None,
deps,
None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ fn add_dependencies(
let source_map: SharedSourceMap = parser.source_map.clone();
let mut block = AsyncDependenciesBlock::new(
*parser.module_identifier,
Some(Into::<DependencyRange>::into(span).to_loc(Some(source_map).as_ref())),
Into::<DependencyRange>::into(span).to_loc(Some(&source_map)),
None,
vec![dep],
None,
Expand Down
Loading