Skip to content
This repository has been archived by the owner on Jan 25, 2024. It is now read-only.

feat: improved reformat edits #87

Closed
Closed
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
42 changes: 29 additions & 13 deletions Cargo.lock

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

4 changes: 3 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ regex = "1.5.6"
rnix = "0.10.2"
serde = "1.0.138"
serde_json = "1.0.82"
nixpkgs-fmt-rnix = "1.2.0"
nixpkgs-fmt = { git = "https://github.com/nix-community/nixpkgs-fmt" }
smol_str = "0.1.17"
textedit-merge = "0.2.1"

[dev-dependencies]
stoppable_thread = "0.2.1"
Expand Down
25 changes: 19 additions & 6 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ use std::{
str::FromStr,
};

use crate::error::AppError;
use crate::{
error::AppError,
utils::{atom_edit_to_tuple, tuple_to_text_edit},
};

type Error = Box<dyn std::error::Error>;

Expand Down Expand Up @@ -210,11 +213,21 @@ impl App {
self.reply(Response::new_ok(id, document_links));
} else if let Some((id, params)) = cast::<Formatting>(&mut req) {
let changes = if let Some((ast, code, _)) = self.files.get(&params.text_document.uri) {
let fmt = nixpkgs_fmt::reformat_node(&ast.node());
vec![TextEdit {
range: utils::range(&code, TextRange::up_to(ast.node().text().len())),
new_text: fmt.text().to_string(),
}]
let (spacing_edits, indent_edits) = nixpkgs_fmt::reformat_edits(&ast.node());
let merged_edits = textedit_merge::merge(
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this is needed, but I guess this can be answered with nix-community/nixpkgs-fmt#296 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, like I mentioned in nix-community/nixpkgs-fmt#296 (comment), the delete ranges in the spacing edits apply to the original document, and the indent edits apply to the document after the spacing edits have been applied. Since the language server protocol expects a single set of edits in response to a reformat request, I believe merging the two sequential sets of edits into a single set of edits is necessary.

&spacing_edits
.into_iter()
.map(atom_edit_to_tuple)
.collect::<Vec<(std::ops::Range<usize>, _)>>(),
&indent_edits
.into_iter()
.map(atom_edit_to_tuple)
.collect::<Vec<(std::ops::Range<usize>, _)>>(),
);
merged_edits
.into_iter()
.map(|t| tuple_to_text_edit(t, &code))
.collect::<Vec<_>>()
} else {
Vec::new()
};
Expand Down
132 changes: 132 additions & 0 deletions src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,138 @@ fn test_rename() {
handle.stop().join().expect("Failed to gracefully terminate LSP worker thread!");
}

#[test]
fn test_reformat_integration() {
let urlpath = "file:///code/default.nix";
let input = r#"{
f = { x
, y
}: body;

testAllTrue = expr: {inherit expr;expected=map (x: true) expr; };
}
"#;
let (client, handle) = prepare_integration_test(input, urlpath);

let r = Request {
id: RequestId::from(23),
method: String::from("textDocument/formatting"),
params: json!({
"textDocument": {
"uri": "file:///code/default.nix",
},
"options": {
// Tab size isn't respected yet
"tabSize": 37,
"insertSpaces": true
}
})
};
client.sender.send(r.into()).expect("Cannot send reformat request!");

expect_diagnostics(&client);

let msg = recv_msg(&client);
let hover_json = coerce_response(msg).result.expect("Expected reformat response!");
let edits = hover_json;
let expected = json!([
{
"newText": "\n ",
"range": {
"start": {
"character": 5,
"line": 1
},
"end": {
"character": 6,
"line": 1
}
}
},
{
"newText": "\n ",
"range": {
"start": {
"character": 9,
"line": 1
},
"end": {
"character": 2,
"line": 2
}
}
},
{
"newText": "\n ",
"range": {
"start": {
"character": 5,
"line": 2
},
"end": {
"character": 6,
"line": 3
}
}
},
{
"newText": " ",
"range": {
"start": {
"character": 23,
"line": 5
},
"end": {
"character": 23,
"line": 5
}
}
},
{
"newText": " ",
"range": {
"start": {
"character": 36,
"line": 5
},
"end": {
"character": 36,
"line": 5
}
}
},
{
"newText": " ",
"range": {
"start": {
"character": 44,
"line": 5
},
"end": {
"character": 44,
"line": 5
}
}
},
{
"newText": " ",
"range": {
"start": {
"character": 45,
"line": 5
},
"end": {
"character": 45,
"line": 5
}
}
}
]);
assert_eq!(edits, expected);

handle.stop().join().expect("Failed to gracefully terminate LSP worker thread!");
}

#[test]
fn attrs_simple() {
let code = "{ x = 1; y = 2; }.x";
Expand Down
53 changes: 53 additions & 0 deletions src/utils.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use lsp_types::*;
use nixpkgs_fmt::AtomEdit;
use rnix::{types::*, SyntaxNode, TextRange, TextSize, TokenAtOffset};
use std::{
collections::HashMap,
convert::TryFrom,
fmt::{Debug, Display, Formatter, Result},
ops,
path::PathBuf,
rc::Rc,
};
Expand Down Expand Up @@ -323,9 +325,30 @@ pub fn selection_ranges(root: &SyntaxNode, content: &str, pos: Position) -> Opti
root.map(|b| *b)
}

/// Convert an AtomEdit to a tuple whose first element is `atom_edit`'s delete range, and whose
/// second element is a String with the same contents as `atom_edit`'s insert value.
pub fn atom_edit_to_tuple(atom_edit: AtomEdit) -> (ops::Range<usize>, String) {
let r: std::ops::Range<usize> = atom_edit.delete.start().into()..atom_edit.delete.end().into();
Copy link
Member

Choose a reason for hiding this comment

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

Are the two dots really needed here or is that a Rust feature I'm not aware of? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The .. operator creates a Range from two numbers. Since AtomEdit's delete property is of type TextRange, which belongs to the text-size crate, I'm converting that range type to the builtin std::ops::Range type to avoid adding dependencies to textedit-merge.

(r, atom_edit.insert.to_string())
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to document what this is doing and also to add a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done in 4cc254a.

}

pub fn tuple_to_text_edit(tup: (ops::Range<usize>, String), code: &str) -> TextEdit {
TextEdit {
range: range(
code,
TextRange::new(
TextSize::from(tup.0.start as u32),
TextSize::from(tup.0.end as u32),
),
),
new_text: tup.1,
}
}

#[cfg(test)]
mod tests {
use super::*;
use smol_str::SmolStr;

#[test]
fn test_get_offset_from_nix_expr() {
Expand Down Expand Up @@ -467,4 +490,34 @@ mod tests {
let ident_ = ident.unwrap();
assert_eq!(vec!["a"], ident_.path);
}

#[test]
fn test_atom_edit_to_tuple() {
let atom_edits = vec![
AtomEdit {
delete: TextRange::new(TextSize::from(5), TextSize::from(9)),
insert: SmolStr::from("hello world"),
},
AtomEdit {
delete: TextRange::new(TextSize::from(11), TextSize::from(11)),
insert: SmolStr::from("the quick brown fox"),
},
AtomEdit {
delete: TextRange::new(TextSize::from(115698), TextSize::from(126498)),
insert: SmolStr::from("a string"),
},
];
let expected = vec![
(5..9, String::from("hello world")),
(11..11, String::from("the quick brown fox")),
(115698..126498, String::from("a string")),
];
assert_eq!(
atom_edits
.into_iter()
.map(atom_edit_to_tuple)
.collect::<Vec<(ops::Range<usize>, String)>>(),
expected
);
}
}