Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_formatter): fix format range #4295

Merged
merged 3 commits into from
Mar 17, 2023
Merged

fix(rome_formatter): fix format range #4295

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
9909d4e
fix(rome_formatter): fix format range
denbezrukov Mar 11, 2023
b9eae76
fix(rome_formatter): fix format range
denbezrukov Mar 12, 2023
8407dd9
fix(rome_formatter): fix format range
denbezrukov Mar 12, 2023
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
95 changes: 64 additions & 31 deletions crates/rome_formatter/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ use crate::trivia::{format_skipped_token_trivia, format_trimmed_token};
pub use format_element::{normalize_newlines, FormatElement, LINE_TERMINATORS};
pub use group_id::GroupId;
use rome_rowan::{
Language, SyntaxElement, SyntaxNode, SyntaxResult, SyntaxToken, SyntaxTriviaPiece, TextLen,
TextRange, TextSize, TokenAtOffset,
Language, NodeOrToken, SyntaxElement, SyntaxNode, SyntaxResult, SyntaxToken, SyntaxTriviaPiece,
TextLen, TextRange, TextSize, TokenAtOffset,
};
pub use source_map::{TransformSourceMap, TransformSourceMapBuilder};
use std::marker::PhantomData;
Expand Down Expand Up @@ -922,20 +922,24 @@ pub fn format_node<L: FormatLanguage>(
let transformed_range = transformed.text_range();
let root_range = root.text_range();

let transformed = top_root
let transformed_root = top_root
.replace_child(root.clone().into(), transformed.into())
// SAFETY: Calling `unwrap` is safe because we know that `root` is part of the `top_root` subtree.
.unwrap()
// get replaced node
.covering_element(TextRange::new(
root_range.start(),
root_range.start() + transformed_range.len(),
))
.into_node()
// SAFETY: Calling `unwrap` is safe because we know that `transformed` is a node.
.unwrap();

(transformed, Some(source_map))
let transformed = transformed_root.covering_element(TextRange::new(
root_range.start(),
root_range.start() + transformed_range.len(),
));

let node = match transformed {
NodeOrToken::Node(node) => node,
NodeOrToken::Token(token) => {
// if we get a token we need to get the parent node
token.parent().unwrap_or(transformed_root)
}
};

(node, Some(source_map))
}
}
}
Expand Down Expand Up @@ -1173,36 +1177,65 @@ pub fn format_range<Language: FormatLanguage>(
// starting before or at said starting point, and the closest
// marker to the end of the source range starting after or at
// said ending point respectively
let mut range_start = None;
let mut range_end = None;
let mut range_start: Option<&SourceMarker> = None;
let mut range_end: Option<&SourceMarker> = None;

let sourcemap = printed.sourcemap();
for marker in sourcemap {
// marker.source <= range.start()
if let Some(start_dist) = range.start().checked_sub(marker.source) {
if marker.source <= range.start() {
range_start = match range_start {
Some((prev_marker, prev_dist)) => {
if start_dist < prev_dist {
Some((marker, start_dist))
Some(prev_marker) => {
if marker.source > prev_marker.source {
if prev_marker.dest == marker.dest {
// we found a marker that is closer to the start range than we have
// but we need to check if the marker has the same dest, otherwise we can get an incorrect substring in the source text
// e.g
// ...
// SourceMarker {
// source: 94, <----- but we need to use this source position to get correct substring in the source
// dest: 99,
// },
// SourceMarker {
// source: 96,
// dest: 99, <----- both markers have the same dest.
// },
// ...
Some(prev_marker)
} else {
Some(marker)
}
} else {
Some((prev_marker, prev_dist))
Some(prev_marker)
}
}
None => Some((marker, start_dist)),
None => Some(marker),
}
}

// marker.source >= range.end()
if let Some(end_dist) = marker.source.checked_sub(range.end()) {
if marker.source >= range.end() {
range_end = match range_end {
Some((prev_marker, prev_dist)) => {
if end_dist <= prev_dist {
Some((marker, end_dist))
Some(prev_marker) => {
if marker.source <= prev_marker.source || marker.dest == prev_marker.dest {
// 1. if we found a marker that is closer to the end we take it.
// 2. if we found a marker that is farther to the end range than we have
// but we need to check if the marker has the same dest, otherwise we can get an incorrect substring in the source text
// e.g
// ...
// SourceMarker {
// source: 94,
// dest: 99, <----- both markers have the same dest.
// },
// SourceMarker {
// source: 96, <----- but we need to use this source position to get correct substring in the source
// dest: 99,
// },
// ...
Some(marker)
} else {
Some((prev_marker, prev_dist))
Some(prev_marker)
}
}
None => Some((marker, end_dist)),
None => Some(marker),
}
}
}
Expand All @@ -1212,11 +1245,11 @@ pub fn format_range<Language: FormatLanguage>(
// the start (or after the end) of the formatting range: in this case
// the start/end marker default to the start/end of the input
let (start_source, start_dest) = match range_start {
Some((start_marker, _)) => (start_marker.source, start_marker.dest),
Some(start_marker) => (start_marker.source, start_marker.dest),
None => (common_root.text_range().start(), TextSize::from(0)),
};
let (end_source, end_dest) = match range_end {
Some((end_marker, _)) => (end_marker.source, end_marker.dest),
Some(end_marker) => (end_marker.source, end_marker.dest),
None => (
common_root.text_range().end(),
TextSize::try_from(printed.as_code().len()).expect("code length out of bounds"),
Expand Down
128 changes: 90 additions & 38 deletions crates/rome_formatter_test/src/spec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,26 @@
use crate::check_reformat::CheckReformat;
use crate::snapshot_builder::{SnapshotBuilder, SnapshotOutput};
use crate::utils::strip_rome_placeholders;
use crate::TestFormatLanguage;
use rome_console::EnvConsole;
use rome_formatter::FormatOptions;
use rome_formatter::{FormatOptions, Printed};
use rome_fs::RomePath;
use rome_parser::AnyParse;
use rome_rowan::{TextRange, TextSize};
use rome_service::workspace::{FeatureName, SupportsFeatureParams};
use rome_service::App;
use std::ops::Range;
use std::path::{Path, PathBuf};

#[derive(Debug)]
pub struct SpecTestFile<'a> {
input_file: RomePath,
root_path: &'a Path,

input_code: String,

range_start_index: Option<usize>,
range_end_index: Option<usize>,
}

impl<'a> SpecTestFile<'a> {
Expand All @@ -39,13 +47,19 @@ impl<'a> SpecTestFile<'a> {

match can_format.reason {
None => {
let input_code = input_file.get_buffer_from_file();
let mut input_code = input_file.get_buffer_from_file();

let (_, range_start_index, range_end_index) =
strip_rome_placeholders(&mut input_code);

Some(SpecTestFile {
input_file,
root_path,

input_code,

range_start_index,
range_end_index,
})
}
Some(_) => None,
Expand Down Expand Up @@ -76,6 +90,10 @@ impl<'a> SpecTestFile<'a> {
.to_str()
.expect("failed to get relative file name")
}

fn range(&self) -> (Option<usize>, Option<usize>) {
(self.range_start_index, self.range_end_index)
}
}

pub struct SpecSnapshot<'a, L>
Expand Down Expand Up @@ -108,6 +126,70 @@ where
}
}

fn formatted(&self, parsed: &AnyParse, options: L::Options) -> (String, Printed) {
let has_errors = parsed.has_errors();
let syntax = parsed.syntax();

let range = self.test_file.range();

let result = match range {
(Some(start), Some(end)) => self.language.format_range(
options.clone(),
&syntax,
TextRange::new(
TextSize::try_from(start).unwrap(),
TextSize::try_from(end).unwrap(),
),
),
_ => self
.language
.format_node(options.clone(), &syntax)
.map(|formatted| formatted.print().unwrap()),
};
let formatted = result.expect("formatting failed");

let output_code = match range {
(Some(_), Some(_)) => {
let range = formatted
.range()
.expect("the result of format_range should have a range");

let mut output_code = self.test_file.input_code.clone();
output_code.replace_range(Range::<usize>::from(range), formatted.as_code());

// Check if output code is a valid syntax
let parsed = self.language.parse(&output_code);

if parsed.has_errors() {
panic!(
"{:?} format range produced an invalid syntax tree: {:?}",
self.test_file.input_file, output_code
)
}

output_code
}
_ => {
let output_code = formatted.as_code();

if !has_errors {
let check_reformat = CheckReformat::new(
&syntax,
output_code,
self.test_file.file_name(),
&self.language,
options,
);
check_reformat.check_reformat();
}

output_code.to_string()
}
};

(output_code, formatted)
}

pub fn test(self) {
let input_file = self.test_file().input_file().as_path();

Expand All @@ -118,35 +200,17 @@ where

let parsed = self.language.parse(self.test_file.input_code());

let has_errors = parsed.has_errors();
let root = parsed.syntax();

let formatted = self
.language
.format_node(self.options.clone(), &root)
.unwrap();
let printed = formatted.print().unwrap();

if !has_errors {
let check_reformat = CheckReformat::new(
&root,
printed.as_code(),
self.test_file.file_name(),
&self.language,
self.options.clone(),
);
check_reformat.check_reformat();
}
let (output_code, printed) = self.formatted(&parsed, self.options.clone());

let max_width = self.options.line_width().value() as usize;

snapshot_builder = snapshot_builder
.with_output_and_options(
SnapshotOutput::new(printed.as_code()).with_index(1),
SnapshotOutput::new(&output_code).with_index(1),
self.options.clone(),
)
.with_unimplemented(&printed)
.with_lines_exceeding_max_width(printed.as_code(), max_width);
.with_lines_exceeding_max_width(&output_code, max_width);

let options_path = self.test_directory.join("options.json");
if options_path.exists() {
Expand All @@ -158,29 +222,17 @@ where
.deserialize_format_options(options_path.get_buffer_from_file().as_str());

for (index, options) in test_options.into_iter().enumerate() {
let formatted = self.language.format_node(options.clone(), &root).unwrap();
let printed = formatted.print().unwrap();

if !has_errors {
let check_reformat = CheckReformat::new(
&root,
printed.as_code(),
self.test_file.file_name(),
&self.language,
options.clone(),
);
check_reformat.check_reformat();
}
let (output_code, printed) = self.formatted(&parsed, options.clone());

let max_width = options.line_width().value() as usize;

snapshot_builder = snapshot_builder
.with_output_and_options(
SnapshotOutput::new(printed.as_code()).with_index(index + 2),
SnapshotOutput::new(&output_code).with_index(index + 2),
options,
)
.with_unimplemented(&printed)
.with_lines_exceeding_max_width(printed.as_code(), max_width);
.with_lines_exceeding_max_width(&output_code, max_width);
}
}

Expand Down
Loading