From 15320788999a3b7650241532d1fd2f32809b45d4 Mon Sep 17 00:00:00 2001 From: bing Date: Tue, 9 May 2023 23:24:50 +0000 Subject: [PATCH] feat(fmt): improve newline insertion when comments are in between leaf spans (#4548) Closes #4185 ## Description The solution we adopted as a start would have us take the space between leaf spans, except a problem was that a doc comment would be a leaf span but a regular comment would not be a leaf span. Usually this would not be an issue, but in certain cases where we perhaps want to comment code out temporarily to test changes, the formatter would move comments around unintentionally. For example, a doc comment followed by a commented out function (as shown in the issue) would cause the commented out function to move away from the doc comment: ```rust contract; abi MyContract { fn test_function() -> bool; } impl MyContract for Contract { /// This is documentation for a commented out function <-- a newline is inserted after this comment // fn commented_out_function() { //} fn test_function() -> bool { true } } ``` Situations like these could ruin developer experience. The main problem is that the entire whitespace gap between 2 leaf spans could have multiple comments with multiple newline sequences, and all these would be squished to the end of the first leaf span when inserting the newlines. The fix is to take 'snippets' while we're scanning for newline sequences to insert and try to detect comments within these snippets. We then split insertion of newline sequences into two phases: one before the first comment (which is described above, which caused the issue above), one after the last comment. ## 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). - [ ] 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. --- swayfmt/src/utils/map/newline.rs | 180 +++++++++++++++++++++++++------ swayfmt/tests/mod.rs | 45 ++++++++ 2 files changed, 190 insertions(+), 35 deletions(-) diff --git a/swayfmt/src/utils/map/newline.rs b/swayfmt/src/utils/map/newline.rs index afed65d494d..8e8dbe9a6d4 100644 --- a/swayfmt/src/utils/map/newline.rs +++ b/swayfmt/src/utils/map/newline.rs @@ -1,12 +1,6 @@ use anyhow::Result; use ropey::Rope; -use std::{ - collections::BTreeMap, - fmt::Write, - ops::Bound::{Excluded, Included}, - path::PathBuf, - sync::Arc, -}; +use std::{collections::BTreeMap, fmt::Write, path::PathBuf, sync::Arc}; use sway_ast::Module; use crate::{ @@ -17,7 +11,7 @@ use crate::{ }; /// Represents a series of consecutive newlines -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq)] struct NewlineSequence { sequence_length: usize, } @@ -149,18 +143,109 @@ fn add_newlines( .skip(1) .zip(formatted_newline_spans.iter().skip(1)) { - if let Some(newline_sequence) = get_newline_sequence_between_spans( - previous_unformatted_newline_span, - unformatted_newline_span, - &newline_map, - ) { - offset += insert_after_span( - previous_formatted_newline_span, - newline_sequence, - offset, - formatted_code, - newline_threshold, - )?; + if previous_unformatted_newline_span.end < unformatted_newline_span.start { + // At its core, the spaces between leaf spans are nothing more than just whitespace characters, + // and sometimes comments, since they are not considered valid AST nodes. We are interested in + // these spaces (with comments, if any) + let whitespaces_with_comments = &unformatted_code + [previous_unformatted_newline_span.end..unformatted_newline_span.start]; + + let mut whitespaces_with_comments_it = + whitespaces_with_comments.char_indices().peekable(); + + let start = previous_unformatted_newline_span.end; + let mut comment_found = false; + + // Here, we will try to insert newlines that occur before comments. + while let Some((idx, character)) = whitespaces_with_comments_it.next() { + if character == '/' { + if let Some((_, '/') | (_, '*')) = whitespaces_with_comments_it.peek() { + comment_found = true; + + // Insert newlines that occur before the first comment here + if let Some(newline_sequence) = first_newline_sequence_in_span( + &ByteSpan { + start, + end: start + idx, + }, + &newline_map, + ) { + let at = previous_formatted_newline_span.end + offset; + offset += insert_after_span( + at, + newline_sequence, + formatted_code, + newline_threshold, + )?; + break; + } + } + } + + // If there are no comments found in the sequence of whitespaces, there is no point + // in trying to find newline sequences from the back. So we simply take the entire + // sequence, insert newlines at the start and we're done with this iteration of the for loop. + if idx == whitespaces_with_comments.len() - 1 && !comment_found { + if let Some(newline_sequence) = first_newline_sequence_in_span( + &ByteSpan { + start, + end: unformatted_newline_span.start, + }, + &newline_map, + ) { + let at = previous_formatted_newline_span.end + offset; + offset += insert_after_span( + at, + newline_sequence, + formatted_code, + newline_threshold, + )?; + } + } + } + + // If we found some comment(s), we are also interested in inserting + // newline sequences that happen after the last comment. + // + // This can be a single comment or multiple comments. + if comment_found { + let mut whitespaces_with_comments_rev_it = + whitespaces_with_comments.char_indices().rev().peekable(); + let mut end_of_last_comment = whitespaces_with_comments.len(); + + // Find point of insertion of newline sequences + for (idx, character) in whitespaces_with_comments_rev_it.by_ref() { + if !character.is_whitespace() { + end_of_last_comment = idx + 1; + break; + } + } + + while let Some((_, character)) = whitespaces_with_comments_rev_it.next() { + if character == '/' { + // Comments either start with '//' or end with '*/' + if let Some((_, '/') | (_, '*')) = whitespaces_with_comments_rev_it.peek() { + if let Some(newline_sequence) = first_newline_sequence_in_span( + &ByteSpan { + start: start + end_of_last_comment, + end: unformatted_newline_span.start, + }, + &newline_map, + ) { + offset += insert_after_span( + previous_formatted_newline_span.end + + end_of_last_comment + + offset, + newline_sequence, + formatted_code, + newline_threshold, + )?; + } + break; + } + } + } + } } previous_unformatted_newline_span = unformatted_newline_span; previous_formatted_newline_span = formatted_newline_span; @@ -176,39 +261,37 @@ fn format_newline_sequence(newline_sequence: &NewlineSequence, threshold: usize) } } -/// Inserts after given span and returns the offset. +/// Inserts a `NewlineSequence` at position `at` and returns the length of `NewlineSequence` inserted. +/// The return value is used to calculate the new `at` in a later point. fn insert_after_span( - from: &ByteSpan, + at: usize, newline_sequence: NewlineSequence, - offset: usize, formatted_code: &mut FormattedCode, threshold: usize, ) -> Result { let mut sequence_string = String::new(); - 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); + src_rope.insert(at, &sequence_string); formatted_code.clear(); formatted_code.push_str(&src_rope.to_string()); Ok(sequence_string.len()) } -/// Returns a newline sequence between given spans, if found. -fn get_newline_sequence_between_spans( - from: &ByteSpan, - to: &ByteSpan, +/// Returns the first newline sequence contained in a span. +/// This is inclusive at the start, and exclusive at the end, i.e. +/// the bounds are [span.start, span.end). +fn first_newline_sequence_in_span( + span: &ByteSpan, newline_map: &NewlineMap, ) -> Option { - if from < to { - if let Some((_, newline_sequence)) = - newline_map.range((Included(from), Excluded(to))).next() - { - return Some(newline_sequence.clone()); + for (range, sequence) in newline_map.iter() { + if span.start <= range.start && range.end < span.end { + return Some(sequence.clone()); } } None @@ -216,7 +299,9 @@ fn get_newline_sequence_between_spans( #[cfg(test)] mod tests { - use super::newline_map_from_src; + use crate::utils::map::{byte_span::ByteSpan, newline::first_newline_sequence_in_span}; + + use super::{newline_map_from_src, NewlineMap, NewlineSequence}; #[test] fn test_newline_map() { @@ -264,4 +349,29 @@ fn main() { assert_eq!(newline_sequence_lengths, correct_newline_sequence_lengths); } + + #[test] + fn test_newline_range_simple() { + let mut newline_map = NewlineMap::new(); + let newline_sequence = NewlineSequence { sequence_length: 2 }; + + newline_map.insert(ByteSpan { start: 9, end: 10 }, newline_sequence.clone()); + assert_eq!( + newline_sequence, + first_newline_sequence_in_span(&ByteSpan { start: 8, end: 11 }, &newline_map).unwrap() + ); + assert_eq!( + newline_sequence, + first_newline_sequence_in_span(&ByteSpan { start: 9, end: 11 }, &newline_map).unwrap() + ); + assert!( + first_newline_sequence_in_span(&ByteSpan { start: 9, end: 10 }, &newline_map).is_none() + ); + assert!( + first_newline_sequence_in_span(&ByteSpan { start: 9, end: 9 }, &newline_map).is_none() + ); + assert!( + first_newline_sequence_in_span(&ByteSpan { start: 8, end: 8 }, &newline_map).is_none() + ); + } } diff --git a/swayfmt/tests/mod.rs b/swayfmt/tests/mod.rs index 68623263718..d0f081fe5e7 100644 --- a/swayfmt/tests/mod.rs +++ b/swayfmt/tests/mod.rs @@ -1531,3 +1531,48 @@ use ::utils::numbers::*; "#, ); } + +#[test] +fn temporarily_commented_out_fn_with_doc_comments() { + check( + r#"contract; + +abi MyContract { + /// Doc comment + /* + Some comment + */ + fn test_function() -> bool; +} + +impl MyContract for Contract { + /// This is documentation for a commented out function + // fn commented_out_function() { + //} + + fn test_function() -> bool { + true + } +}"#, + r#"contract; + +abi MyContract { + /// Doc comment + /* + Some comment + */ + fn test_function() -> bool; +} + +impl MyContract for Contract { + /// This is documentation for a commented out function + // fn commented_out_function() { + //} + + fn test_function() -> bool { + true + } +} +"#, + ); +}