Skip to content

Commit

Permalink
fix(fmt): redundant newline insertions in commented code (FuelLabs#4472)
Browse files Browse the repository at this point in the history
## Description

closes FuelLabs#4473

This contains quite a big change for how we insert newlines into the
formatted code - currently we would get a `Vec<NewlineSequence>` to
insert into a span, and this PR changes this to just a
`NewlineSequence`.

The bug was a result of collecting this `Vec` stated above when
**checking between the last unformatted leaf span and the end of code
leaf span that was manually inserted**. We then end up inserting it all
in one place - resulting in a bunch of newlines rather than just 1.

The reason for changing this to just a single `NewlineSequence` is
because IMO there isn't really a situation where we might have multiple
newline sequences between 2 leaf spans such that we want to insert a
`Vec<NewlineSequence>`, but it's causing problems when it comes to the
situation described above.

To get a look at how this situation looks like - the test case added
shows an example of this.

## Checklist

- [x] I have linked to any relevant issues.
- [ ] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

Co-authored-by: Kaya Gökalp <[email protected]>
Co-authored-by: Joshua Batty <[email protected]>
  • Loading branch information
3 people authored Apr 21, 2023
1 parent 3334843 commit 65bdd0e
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 40 deletions.
59 changes: 19 additions & 40 deletions swayfmt/src/utils/map/newline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,14 @@ fn add_newlines(
.skip(1)
.zip(formatted_newline_spans.iter().skip(1))
{
let newline_sequences = get_newline_sequences_between_spans(
if let Some(newline_sequence) = get_newline_sequence_between_spans(
previous_unformatted_newline_span,
unformatted_newline_span,
&newline_map,
);
if !newline_sequences.is_empty() {
) {
offset += insert_after_span(
previous_formatted_newline_span,
newline_sequences,
newline_sequence,
offset,
formatted_code,
newline_threshold,
Expand All @@ -169,70 +168,50 @@ fn add_newlines(
Ok(())
}

fn format_newline_sequnce(newline_sequence: &NewlineSequence, threshold: usize) -> String {
fn format_newline_sequence(newline_sequence: &NewlineSequence, threshold: usize) -> String {
if newline_sequence.sequence_length > threshold {
(0..threshold).map(|_| "\n").collect::<String>()
} else {
newline_sequence.to_string()
}
}

/// Checks for newlines that are already in the source code.
fn find_already_present_extra_newlines(from: usize, src: String) -> usize {
let mut number_of_newlines_present = 0;
for char in src.chars().skip(from) {
if char == '\n' {
number_of_newlines_present += 1;
} else {
break;
}
}
if number_of_newlines_present == 0 {
0
} else {
number_of_newlines_present - 1
}
}

/// Inserts after given span and returns the offset.
fn insert_after_span(
from: &ByteSpan,
newline_sequences_to_insert: Vec<NewlineSequence>,
newline_sequence: NewlineSequence,
offset: usize,
formatted_code: &mut FormattedCode,
threshold: usize,
) -> Result<usize, FormatterError> {
let iter = newline_sequences_to_insert.iter();
let mut sequence_string = String::new();
let newlines_to_skip =
find_already_present_extra_newlines(from.end, formatted_code.to_string());
for newline_sequence in iter.skip(newlines_to_skip) {
write!(
sequence_string,
"{}",
format_newline_sequnce(newline_sequence, threshold)
)?;
}

write!(
sequence_string,
"{}",
format_newline_sequence(&newline_sequence, threshold)
)?;
let mut src_rope = Rope::from_str(formatted_code);
src_rope.insert(from.end + offset, &sequence_string);
formatted_code.clear();
formatted_code.push_str(&src_rope.to_string());
Ok(sequence_string.len())
}

/// Returns a list of newline sequence between given spans.
fn get_newline_sequences_between_spans(
/// Returns a newline sequence between given spans, if found.
fn get_newline_sequence_between_spans(
from: &ByteSpan,
to: &ByteSpan,
newline_map: &NewlineMap,
) -> Vec<NewlineSequence> {
let mut newline_sequences: Vec<NewlineSequence> = Vec::new();
) -> Option<NewlineSequence> {
if from < to {
for (_, newline_sequence) in newline_map.range((Included(from), Excluded(to))) {
newline_sequences.push(newline_sequence.clone());
if let Some((_, newline_sequence)) =
newline_map.range((Included(from), Excluded(to))).next()
{
return Some(newline_sequence.clone());
}
}
newline_sequences
None
}

#[cfg(test)]
Expand Down
45 changes: 45 additions & 0 deletions swayfmt/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1486,3 +1486,48 @@ impl MyAbi for Contract {
"#,
);
}

#[test]
fn test_comments_after_deps() {
check(
r#"library;
use std::{u256::U256, vec::*};
use ::utils::vec::sort;
use ::utils::numbers::*;
// pub fn aggregate_results(results: Vec<Vec<U256>>) -> Vec<U256> {
// let mut aggregated = Vec::new();
// let mut i = 0;
// while (i < results.len) {
// let values = results.get(i).unwrap();
// aggregated.push(aggregate_values(values));
// i += 1;
// }
// return aggregated;
// }"#,
r#"library;
use std::{u256::U256, vec::*};
use ::utils::vec::sort;
use ::utils::numbers::*;
// pub fn aggregate_results(results: Vec<Vec<U256>>) -> Vec<U256> {
// let mut aggregated = Vec::new();
// let mut i = 0;
// while (i < results.len) {
// let values = results.get(i).unwrap();
// aggregated.push(aggregate_values(values));
// i += 1;
// }
// return aggregated;
// }
"#,
);
}

0 comments on commit 65bdd0e

Please sign in to comment.