Skip to content

Refactor/json source info #74

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 23, 2025
Merged

Refactor/json source info #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a688633
perf: increase parsing speed by 2x by being careful around parsing tr…
cscheid Oct 22, 2025
eff85d4
updated source info representation in JSON format
cscheid Oct 23, 2025
13d2e9e
merge
cscheid Oct 23, 2025
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
16 changes: 15 additions & 1 deletion crates/qmd-syntax-helper/src/conversions/div_whitespace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,21 @@ impl DivWhitespaceConverter {
let error_row = error
.location
.as_ref()
.map(|loc| loc.range.start.row)
.and_then(|loc| {
// Get the start offset from SourceInfo
let offset = loc.start_offset();
// Binary search to find which line this offset is on
match line_starts.binary_search(&offset) {
Ok(idx) => Some(idx),
Err(idx) => {
if idx > 0 {
Some(idx - 1)
} else {
Some(0)
}
}
}
})
.unwrap_or(0);

// The error might be on the line itself or the line before (for div fences)
Expand Down
109 changes: 32 additions & 77 deletions crates/quarto-error-reporting/src/diagnostic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ impl DiagnosticMessage {
if let Some(loc) = &self.location {
// Try to map with context if available
if let Some(ctx) = ctx {
if let Some(mapped) = loc.map_offset(loc.range.start.offset, ctx) {
if let Some(mapped) = loc.map_offset(loc.start_offset(), ctx) {
if let Some(file) = ctx.get_file(mapped.file_id) {
write!(
result,
Expand All @@ -359,13 +359,9 @@ impl DiagnosticMessage {
}
} else {
// No context: show immediate location (1-indexed for display)
write!(
result,
" at {}:{}\n",
loc.range.start.row + 1,
loc.range.start.column + 1
)
.unwrap();
// Note: Without context, we can't get row/column from offsets
// We could map_offset with ctx to get Location, but ctx is None here
write!(result, " at offset {}\n", loc.start_offset()).unwrap();
}
}

Expand Down Expand Up @@ -500,15 +496,12 @@ impl DiagnosticMessage {
fn extract_file_id(
source_info: &quarto_source_map::SourceInfo,
) -> Option<quarto_source_map::FileId> {
match &source_info.mapping {
quarto_source_map::SourceMapping::Original { file_id } => Some(*file_id),
quarto_source_map::SourceMapping::Substring { parent, .. } => {
match source_info {
quarto_source_map::SourceInfo::Original { file_id, .. } => Some(*file_id),
quarto_source_map::SourceInfo::Substring { parent, .. } => {
Self::extract_file_id(parent)
}
quarto_source_map::SourceMapping::Transformed { parent, .. } => {
Self::extract_file_id(parent)
}
quarto_source_map::SourceMapping::Concat { pieces } => {
quarto_source_map::SourceInfo::Concat { pieces } => {
// For concatenated sources, use the first piece's file_id
pieces
.first()
Expand Down Expand Up @@ -544,8 +537,9 @@ impl DiagnosticMessage {
};

// Map the location offsets back to original file positions
let start_mapped = main_location.map_offset(main_location.range.start.offset, ctx)?;
let end_mapped = main_location.map_offset(main_location.range.end.offset, ctx)?;
// map_offset expects relative offsets (0 = start of this SourceInfo's range)
let start_mapped = main_location.map_offset(0, ctx)?;
let end_mapped = main_location.map_offset(main_location.length(), ctx)?;

// Determine report kind and color
let (report_kind, main_color) = match self.kind {
Expand Down Expand Up @@ -591,9 +585,10 @@ impl DiagnosticMessage {

if detail_file_id == file_id {
// Map detail offsets to original file positions
// map_offset expects relative offsets (0 = start of SourceInfo's range)
if let (Some(detail_start), Some(detail_end)) = (
detail_loc.map_offset(detail_loc.range.start.offset, ctx),
detail_loc.map_offset(detail_loc.range.end.offset, ctx),
detail_loc.map_offset(0, ctx),
detail_loc.map_offset(detail_loc.length(), ctx),
) {
let detail_span = detail_start.location.offset..detail_end.location.offset;
let detail_color = match detail.kind {
Expand Down Expand Up @@ -787,32 +782,19 @@ mod tests {
fn test_location_in_to_text_without_context() {
use crate::builder::DiagnosticMessageBuilder;

// Create a location at row 10, column 5
let location = quarto_source_map::SourceInfo::original(
quarto_source_map::FileId(0),
quarto_source_map::Range {
start: quarto_source_map::Location {
offset: 100,
row: 10,
column: 5,
},
end: quarto_source_map::Location {
offset: 110,
row: 10,
column: 15,
},
},
);
// Create a location at offsets 100-110
let location =
quarto_source_map::SourceInfo::original(quarto_source_map::FileId(0), 100, 110);

let msg = DiagnosticMessageBuilder::error("Invalid syntax")
.with_location(location)
.build();

let text = msg.to_text(None);

// Without context, should show immediate location (1-indexed)
// Without context, should show offset (we can't get row/column without context)
assert!(text.contains("Invalid syntax"));
assert!(text.contains("at 11:6")); // row 10 + 1, column 5 + 1
assert!(text.contains("at offset 100"));
}

#[test]
Expand All @@ -826,21 +808,10 @@ mod tests {
Some("line 1\nline 2\nline 3\nline 4".to_string()),
);

// Create a location in that file
// Create a location in that file (offset 7 is start of "line 2")
let location = quarto_source_map::SourceInfo::original(
file_id,
quarto_source_map::Range {
start: quarto_source_map::Location {
offset: 7, // Start of "line 2"
row: 1,
column: 0,
},
end: quarto_source_map::Location {
offset: 13,
row: 1,
column: 6,
},
},
file_id, 7, // Start of "line 2"
13, // End of "line 2"
);

let msg = DiagnosticMessageBuilder::error("Invalid syntax")
Expand All @@ -859,41 +830,25 @@ mod tests {
fn test_location_in_to_json() {
use crate::builder::DiagnosticMessageBuilder;

let location = quarto_source_map::SourceInfo::original(
quarto_source_map::FileId(0),
quarto_source_map::Range {
start: quarto_source_map::Location {
offset: 100,
row: 10,
column: 5,
},
end: quarto_source_map::Location {
offset: 110,
row: 10,
column: 15,
},
},
);
let location =
quarto_source_map::SourceInfo::original(quarto_source_map::FileId(0), 100, 110);

let msg = DiagnosticMessageBuilder::error("Invalid syntax")
.with_location(location)
.build();

let json = msg.to_json();

// Should have location field with range info
// Should have location field with Original variant
assert!(json.get("location").is_some());
let loc = &json["location"];
assert!(loc.get("range").is_some());

// Verify the range is serialized correctly
let range = &loc["range"];
assert_eq!(range["start"]["row"], 10);
assert_eq!(range["start"]["column"], 5);
assert_eq!(range["start"]["offset"], 100);
assert_eq!(range["end"]["row"], 10);
assert_eq!(range["end"]["column"], 15);
assert_eq!(range["end"]["offset"], 110);

// Verify the SourceInfo is serialized correctly (as Original enum variant)
assert!(loc.get("Original").is_some());
let original = &loc["Original"];
assert_eq!(original["file_id"], 0);
assert_eq!(original["start_offset"], 100);
assert_eq!(original["end_offset"], 110);
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion crates/quarto-markdown-pandoc/src/pandoc/inline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ mod tests {
use super::*;

fn dummy_source_info() -> quarto_source_map::SourceInfo {
quarto_source_map::SourceInfo::original(
quarto_source_map::SourceInfo::from_range(
quarto_source_map::FileId(0),
quarto_source_map::Range {
start: quarto_source_map::Location {
Expand Down
33 changes: 19 additions & 14 deletions crates/quarto-markdown-pandoc/src/pandoc/location.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ impl SourceInfo {
}
}

/// Get the start offset
pub fn start_offset(&self) -> usize {
self.range.start.offset
}

/// Get the end offset
pub fn end_offset(&self) -> usize {
self.range.end.offset
}

/// Convert to quarto-source-map::SourceInfo (temporary conversion helper)
///
/// This helper bridges between pandoc::location types and quarto-source-map types.
Expand All @@ -71,7 +81,7 @@ impl SourceInfo {
/// Creates an Original mapping with a dummy FileId(0).
/// For proper filename support, use to_source_map_info_with_mapping with a real FileId.
pub fn to_source_map_info(&self) -> quarto_source_map::SourceInfo {
quarto_source_map::SourceInfo::original(
quarto_source_map::SourceInfo::from_range(
quarto_source_map::FileId(0),
quarto_source_map::Range {
start: quarto_source_map::Location {
Expand All @@ -96,7 +106,7 @@ impl SourceInfo {
&self,
file_id: quarto_source_map::FileId,
) -> quarto_source_map::SourceInfo {
quarto_source_map::SourceInfo::original(
quarto_source_map::SourceInfo::from_range(
file_id,
quarto_source_map::Range {
start: quarto_source_map::Location {
Expand Down Expand Up @@ -132,14 +142,14 @@ pub fn node_location(node: &tree_sitter::Node) -> quarto_source_map::Range {
}

pub fn node_source_info(node: &tree_sitter::Node) -> quarto_source_map::SourceInfo {
quarto_source_map::SourceInfo::original(quarto_source_map::FileId(0), node_location(node))
quarto_source_map::SourceInfo::from_range(quarto_source_map::FileId(0), node_location(node))
}

pub fn node_source_info_with_context(
node: &tree_sitter::Node,
context: &ASTContext,
) -> quarto_source_map::SourceInfo {
quarto_source_map::SourceInfo::original(context.current_file_id(), node_location(node))
quarto_source_map::SourceInfo::from_range(context.current_file_id(), node_location(node))
}

pub fn empty_range() -> Range {
Expand All @@ -158,7 +168,7 @@ pub fn empty_range() -> Range {
}

pub fn empty_source_info() -> quarto_source_map::SourceInfo {
quarto_source_map::SourceInfo::original(
quarto_source_map::SourceInfo::from_range(
quarto_source_map::FileId(0),
quarto_source_map::Range {
start: quarto_source_map::Location {
Expand All @@ -177,15 +187,10 @@ pub fn empty_source_info() -> quarto_source_map::SourceInfo {

/// Extract filename index from quarto_source_map::SourceInfo by walking to Original mapping
pub fn extract_filename_index(info: &quarto_source_map::SourceInfo) -> Option<usize> {
match &info.mapping {
quarto_source_map::SourceMapping::Original { file_id } => Some(file_id.0),
quarto_source_map::SourceMapping::Substring { parent, .. } => {
extract_filename_index(parent)
}
quarto_source_map::SourceMapping::Transformed { parent, .. } => {
extract_filename_index(parent)
}
quarto_source_map::SourceMapping::Concat { pieces } => {
match info {
quarto_source_map::SourceInfo::Original { file_id, .. } => Some(file_id.0),
quarto_source_map::SourceInfo::Substring { parent, .. } => extract_filename_index(parent),
quarto_source_map::SourceInfo::Concat { pieces } => {
// Return first non-None filename_index from pieces
pieces
.iter()
Expand Down
46 changes: 42 additions & 4 deletions crates/quarto-markdown-pandoc/src/pandoc/source_map_compat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub fn node_to_source_info(node: &Node, file_id: FileId) -> SourceInfo {
let start_pos = node.start_position();
let end_pos = node.end_position();

SourceInfo::original(
SourceInfo::from_range(
file_id,
Range {
start: Location {
Expand Down Expand Up @@ -91,23 +91,61 @@ pub fn old_to_new_source_info(
};

// Convert the Range (both use the same Location structure)
SourceInfo::original(
SourceInfo::from_range(
file_id,
Range {
start: Location {
offset: old_info.range.start.offset,
offset: old_info.start_offset(),
row: old_info.range.start.row,
column: old_info.range.start.column,
},
end: Location {
offset: old_info.range.end.offset,
offset: old_info.end_offset(),
row: old_info.range.end.row,
column: old_info.range.end.column,
},
},
)
}

/// Convert quarto-source-map::SourceInfo to a quarto_source_map::Range, with a fallback if mapping fails.
///
/// This is for use with PandocNativeIntermediate which uses quarto_source_map::Range.
/// Provides a fallback Range with zero row/column values if the mapping fails.
///
/// # Arguments
/// * `source_info` - The SourceInfo to convert
/// * `ctx` - The ASTContext containing the source context
///
/// # Returns
/// A quarto_source_map::Range with row/column information if available, or a Range with offsets only
pub fn source_info_to_qsm_range_or_fallback(
source_info: &SourceInfo,
ctx: &ASTContext,
) -> quarto_source_map::Range {
let start_mapped = source_info.map_offset(0, &ctx.source_context);
let end_mapped = source_info.map_offset(source_info.length(), &ctx.source_context);

match (start_mapped, end_mapped) {
(Some(start), Some(end)) => quarto_source_map::Range {
start: start.location,
end: end.location,
},
_ => quarto_source_map::Range {
start: quarto_source_map::Location {
offset: source_info.start_offset(),
row: 0,
column: 0,
},
end: quarto_source_map::Location {
offset: source_info.end_offset(),
row: 0,
column: 0,
},
},
}
}

// Note: Tests for these functions will be validated through integration tests
// when they're used in actual parsing modules. The tree-sitter-qmd parser
// setup is too complex to mock in unit tests here.
Loading