Repository: nix-community/nixpkgs-fmt (rust)

Issue:
Odd formatting decision with parenthesized function call with attrset
## Input
```nix
{
  nixpkgs-fmt = (buildCargoCrate {
    name = "nixpkgs-fmt";
    # TODO: probably want to filter .gitignore or something
    src = sources.nixpkgs-fmt;
  }).nixpkgs-fmt.build;
}
```

## Output
```nix
{
  nixpkgs-fmt = (buildCargoCrate {
    name = "nixpkgs-fmt";
    # TODO: probably want to filter .gitignore or something
    src = sources.nixpkgs-fmt;
  }
  ).nixpkgs-fmt.build;
}
```

## Desired output

I personally find the original less odd-looking here, and this seems to be one of the few cases where I disagree with nixpkgs-fmt's decision. Sorry if this has already been opened/discussed; I couldn't find anything from a brief search of the issue tracker.

Pull request:
Fix #178 and #179: comment indentation on top level node let and give…
This PR fix the indentation problem of comment placed in the top level node let.

This PR also try to give user freedom as to whether they want to put a newline after open parentheses or before close parentheses like the following code:
```
  testA = (buildCargoCrate {
    name = "nixpkgs-fmt";
    # TODO: probably want to filter .gitignore or something
    src = sources.nixpkgs-fmt;
  }).nixpkgs-fmt.build;

  testB = forAllSystems (
    system:
    self.apps.${system}.nixpkgs-fmt
  );
```
If they do not put newline after open parentheses like in `testA`, the close parentheses will not be expanded. On the other hand, if user decide to make newline after the open parentheses, then the close parentheses will also be expanded similar to `testB`.

Therefore, if we have `testA` like the following:
```
  testK = (
 buildCargoCrate {
name = "nixpkgs-fmt";
    # TODO: probably want to filter .gitignore or something
src = sources.nixpkgs-fmt;
  }).nixpkgs-fmt.build;
```

then the formatting result will become:
```
testK = (
  buildCargoCrate {
    name = "nixpkgs-fmt";
    # TODO: probably want to filter .gitignore or something
    src = sources.nixpkgs-fmt;
  }
).nixpkgs-fmt.build;
```

Patch:
diff --git a/src/engine/fixes.rs b/src/engine/fixes.rs
index 90ad6dc..1b7c664 100644
--- a/src/engine/fixes.rs
+++ b/src/engine/fixes.rs
@@ -2,16 +2,18 @@ use std::cmp::min;
 
 use rnix::{
     NodeOrToken, SyntaxElement,
-    SyntaxKind::{NODE_STRING, TOKEN_COMMENT, TOKEN_STRING_CONTENT},
+    SyntaxKind::{NODE_STRING, TOKEN_COMMENT, TOKEN_IN, TOKEN_STRING_CONTENT},
     SyntaxNode, SyntaxToken, TextRange, TextUnit,
 };
 
 use crate::{
+    dsl::RuleName,
     engine::{
         indentation::{indent_anchor, IndentLevel},
         BlockPosition, FmtModel,
     },
     pattern::{Pattern, PatternSet},
+    tree_utils::{on_top_level, prev_non_whitespace_token_sibling},
     AtomEdit,
 };
 
@@ -88,10 +90,20 @@ fn fix_string_indentation(
 /// If we indent multiline block comment, we should indent it's content as well.
 fn fix_comment_indentation(token: &SyntaxToken, model: &mut FmtModel) {
     let is_block_comment = token.text().starts_with("/*");
+    let block = model.block_for(&token.clone().into(), BlockPosition::Before);
     if !is_block_comment {
+        let syntax_element = &token.clone().into();
+        let prev_is_token_in = prev_non_whitespace_token_sibling(syntax_element)
+            .map(|e| e.kind() == TOKEN_IN)
+            .unwrap_or(false);
+        let comment_on_top = on_top_level(syntax_element);
+        if prev_is_token_in & comment_on_top {
+            block.set_text("\n", Some(RuleName::new("Top level let comment")));
+            return;
+        }
         return;
     }
-    let block = model.block_for(&token.clone().into(), BlockPosition::Before);
+
     let (old_indent, new_indent) =
         match (indent_level(block.original_text()), indent_level(block.text())) {
             (Some(old), Some(new)) => (old, new),
diff --git a/src/rules.rs b/src/rules.rs
index 16b1edd..1763f76 100644
--- a/src/rules.rs
+++ b/src/rules.rs
@@ -11,8 +11,8 @@ use crate::{
     pattern::p,
     tree_utils::{
         get_parent, has_newline, next_non_whitespace_parent, next_non_whitespace_sibling,
-        next_token_sibling, prev_non_whitespace_parent, prev_non_whitespace_sibling, prev_sibling,
-        prev_token_sibling, walk_tokens,
+        next_token_sibling, not_on_top_level, on_top_level, prev_non_whitespace_parent,
+        prev_non_whitespace_sibling, prev_sibling, prev_token_sibling, walk_tokens,
     },
 };
 
@@ -64,12 +64,16 @@ pub(crate) fn spacing() -> SpacingDsl {
         .test("( 92 )", "(92)")
         .inside(NODE_PAREN).before(T!["("]).when(paren_inside_list).when(parent_has_newline).newline()
         .inside(NODE_PAREN).after(T!["("]).no_space_or_optional_newline()
-        .inside(NODE_PAREN).after(T!["("]).when(paren_has_token_update).newline()        
+        .inside(NODE_PAREN).after(T!["("]).when(paren_has_token_update).newline()
         .inside(NODE_PAREN).before(T![")"]).no_space_or_newline()
         .inside(NODE_PAREN).before(T![")"]).when(paren_inside_list).no_space()
         .inside(NODE_PAREN).before(T![")"]).when(prev_paren_has_update).when(parent_has_newline).newline()
         .inside(NODE_PAREN).before(T![")"]).when(prev_is_attrset).no_space()
-        
+        .inside(NODE_PAREN).before(T![")"]).when(next_node_is_select).no_space()
+        .inside(NODE_PAREN).before(T![")"]).when(prev_paren_is_select).no_space()
+        .inside(NODE_PAREN).before(T![")"]).when(next_paren_is_select).no_space() 
+        .inside(NODE_PAREN).before(T![")"]).when(paren_on_top_level).no_space_or_newline()
+
         .test("{foo = 92;}", "{ foo = 92; }")
         .inside(NODE_ATTR_SET).after(T!["{"]).single_space_or_newline()
         .inside(NODE_ATTR_SET).before(T!["{"]).when(next_parent_has_update).newline()
@@ -222,16 +226,46 @@ fn parent_has_newline(element: &SyntaxElement) -> bool {
     element.parent().map(|e| has_newline(&e)).unwrap_or(false)
 }
 
+fn next_is_attrset(element: &SyntaxElement) -> bool {
+    next_non_whitespace_sibling(element)
+        .and_then(|e| e.into_node().map(|n| n.kind() == NODE_ATTR_SET))
+        .unwrap_or(false)
+}
+
 fn prev_is_attrset(element: &SyntaxElement) -> bool {
     prev_non_whitespace_sibling(element)
         .and_then(|e| e.into_node().map(|n| n.kind() == NODE_ATTR_SET))
         .unwrap_or(false)
 }
 
-fn next_is_attrset(element: &SyntaxElement) -> bool {
-    next_non_whitespace_sibling(element)
-        .and_then(|e| e.into_node().map(|n| n.kind() == NODE_ATTR_SET))
+fn next_node_is_select(element: &SyntaxElement) -> bool {
+    next_non_whitespace_parent(element)
+        .and_then(|e| e.into_token().map(|n| n.kind() == TOKEN_DOT))
         .unwrap_or(false)
+        & around_paren_no_newline(element)
+}
+
+fn paren_on_top_level(element: &SyntaxElement) -> bool {
+    let parent = match element.parent() {
+        None => return true,
+        Some(it) => it,
+    };
+    match parent.kind() {
+        NODE_ROOT => true,
+        NODE_SELECT | NODE_PAREN | NODE_APPLY => paren_on_top_level(&parent.into()),
+        _ => false,
+    }
+}
+
+fn around_paren_no_newline(element: &SyntaxElement) -> bool {
+    fn after_open_paren_is_newline(element: &SyntaxElement) -> Option<bool> {
+        let get_open_paren = get_parent(element)?.first_child_or_token()?;
+
+        next_token_sibling(&get_open_paren).map(|e| e.text().contains("\n"))
+    }
+    let prev_close_paren_is_newline =
+        prev_token_sibling(element).map(|e| e.text().contains("\n")).unwrap_or(false);
+    (prev_close_paren_is_newline & after_open_paren_is_newline(element).unwrap_or(false)) == false
 }
 
 fn prev_paren_has_update(element: &SyntaxElement) -> bool {
@@ -240,6 +274,20 @@ fn prev_paren_has_update(element: &SyntaxElement) -> bool {
         .unwrap_or(false)
 }
 
+fn prev_paren_is_select(element: &SyntaxElement) -> bool {
+    prev_sibling(element)
+        .and_then(|n| n.last_child().map(|it| it.kind() == NODE_SELECT))
+        .unwrap_or(false)
+        & around_paren_no_newline(element)
+}
+
+fn next_paren_is_select(element: &SyntaxElement) -> bool {
+    prev_sibling(element)
+        .and_then(|n| n.last_child().map(|it| it.kind() == NODE_ATTR_SET))
+        .unwrap_or(false)
+        & around_paren_no_newline(element)
+}
+
 fn next_parent_has_update(element: &SyntaxElement) -> bool {
     next_non_whitespace_parent(element)
         .and_then(|e| e.into_token().map(|it| it.kind() == TOKEN_UPDATE))
@@ -559,24 +607,6 @@ fn inline_lambda(element: &SyntaxElement) -> bool {
     prev_token_sibling(element).map(|t| t.text().contains("\n")) != Some(true)
 }
 
-fn not_on_top_level(element: &SyntaxElement) -> bool {
-    !on_top_level(element)
-}
-
-fn on_top_level(element: &SyntaxElement) -> bool {
-    let parent = match element.parent() {
-        None => return true,
-        Some(it) => it,
-    };
-    match parent.kind() {
-        NODE_ROOT => true,
-        NODE_LAMBDA | NODE_WITH | NODE_ASSERT | NODE_LET_IN | NODE_IF_ELSE => {
-            on_top_level(&parent.into())
-        }
-        _ => false,
-    }
-}
-
 static VALUES: &[SyntaxKind] = &[
     NODE_LAMBDA,
     NODE_IDENT,
diff --git a/src/tree_utils.rs b/src/tree_utils.rs
index 47b1a84..88d06c5 100644
--- a/src/tree_utils.rs
+++ b/src/tree_utils.rs
@@ -1,7 +1,11 @@
 use std::iter::successors;
 
 use rnix::{
-    NodeOrToken, SyntaxElement, SyntaxKind::TOKEN_WHITESPACE, SyntaxNode, SyntaxToken, WalkEvent,
+    NodeOrToken, SyntaxElement,
+    SyntaxKind::{
+        NODE_ASSERT, NODE_IF_ELSE, NODE_LAMBDA, NODE_LET_IN, NODE_ROOT, NODE_WITH, TOKEN_WHITESPACE,
+    },
+    SyntaxNode, SyntaxToken, WalkEvent,
 };
 
 pub(crate) fn walk(node: &SyntaxNode) -> impl Iterator<Item = SyntaxElement> {
@@ -40,6 +44,25 @@ pub(crate) fn prev_sibling(element: &SyntaxElement) -> Option<SyntaxNode> {
         },
     )
 }
+
+pub(crate) fn not_on_top_level(element: &SyntaxElement) -> bool {
+    !on_top_level(element)
+}
+
+pub(crate) fn on_top_level(element: &SyntaxElement) -> bool {
+    let parent = match element.parent() {
+        None => return true,
+        Some(it) => it,
+    };
+    match parent.kind() {
+        NODE_ROOT => true,
+        NODE_LAMBDA | NODE_WITH | NODE_ASSERT | NODE_LET_IN | NODE_IF_ELSE => {
+            on_top_level(&parent.into())
+        }
+        _ => false,
+    }
+}
+
 pub(crate) fn next_token_sibling(element: &SyntaxElement) -> Option<SyntaxToken> {
     successors(element.next_sibling_or_token(), |it| it.next_sibling_or_token()).find_map(
         |element| match element {
@@ -58,6 +81,21 @@ pub(crate) fn prev_token_sibling(element: &SyntaxElement) -> Option<SyntaxToken>
     )
 }
 
+pub(crate) fn prev_non_whitespace_token_sibling(element: &SyntaxElement) -> Option<SyntaxToken> {
+    successors(element.prev_sibling_or_token(), |it| it.prev_sibling_or_token()).find_map(
+        |element| match element {
+            NodeOrToken::Node(_) => None,
+            NodeOrToken::Token(it) => {
+                if it.kind() == TOKEN_WHITESPACE {
+                    None
+                } else {
+                    Some(it)
+                }
+            }
+        },
+    )
+}
+
 pub(crate) fn prev_non_whitespace_parent(element: &SyntaxElement) -> Option<SyntaxElement> {
     successors(get_parent(element)?.prev_sibling_or_token(), |it| it.prev_sibling_or_token())
         .find(|it| it.kind() != TOKEN_WHITESPACE)


Test patch:
diff --git a/test_data/issue-125.bad.nix b/test_data/issue-125.bad.nix
index bcd4f91..69504c9 100644
--- a/test_data/issue-125.bad.nix
+++ b/test_data/issue-125.bad.nix
@@ -1,8 +1,9 @@
 {foo,bar}:
 let
+# comment
 foo = let x = y; in
 z;
-# all in one line
+# comment
 bar = let x = 3; in x;
 faz = let x = 3;
 y = 4; in x;
@@ -15,4 +16,7 @@ x;
 qux = v: let y = 3; in y;
 nux = v: let x = 3;
 y = 3; in y;
-in body
+in
+# comment
+body
+# who put comment in this line?
diff --git a/test_data/issue-125.good.nix b/test_data/issue-125.good.nix
index a3de059..786d26e 100644
--- a/test_data/issue-125.good.nix
+++ b/test_data/issue-125.good.nix
@@ -1,8 +1,9 @@
 { foo, bar }:
 let
+  # comment
   foo = let x = y; in
     z;
-  # all in one line
+  # comment
   bar = let x = 3; in x;
   faz =
     let
@@ -24,4 +25,6 @@ let
       y = 3;
     in y;
 in
+# comment
 body
+# who put comment in this line?
diff --git a/test_data/issue-152.bad.nix b/test_data/issue-152.bad.nix
index 864c8cf..2a612fd 100644
--- a/test_data/issue-152.bad.nix
+++ b/test_data/issue-152.bad.nix
@@ -16,4 +16,5 @@ baz = x:
   #comment
   foo+bar;
 in
+  #comment
 id 1
diff --git a/test_data/issue-152.good.nix b/test_data/issue-152.good.nix
index bbeff49..7c9a863 100644
--- a/test_data/issue-152.good.nix
+++ b/test_data/issue-152.good.nix
@@ -18,4 +18,5 @@ let
     #comment
     foo + bar;
 in
+#comment
 id 1
diff --git a/test_data/issue-178.bad.nix b/test_data/issue-178.bad.nix
new file mode 100644
index 0000000..be46b70
--- /dev/null
+++ b/test_data/issue-178.bad.nix
@@ -0,0 +1,10 @@
+{ 
+testA =(buildCargoCrate {
+name = "nixpkgs-fmt";
+# TODO: probably want to filter .gitignore or something
+src = sources.nixpkgs-fmt;
+}).nixpkgs-fmt.build;
+
+testB=buildLinux (map ({ name }:
+  {inherit name; }) cfg.feeds);
+}
diff --git a/test_data/issue-178.good.nix b/test_data/issue-178.good.nix
new file mode 100644
index 0000000..715efcb
--- /dev/null
+++ b/test_data/issue-178.good.nix
@@ -0,0 +1,10 @@
+{
+  testA = (buildCargoCrate {
+    name = "nixpkgs-fmt";
+    # TODO: probably want to filter .gitignore or something
+    src = sources.nixpkgs-fmt;
+  }).nixpkgs-fmt.build;
+
+  testB = buildLinux (map ({ name }:
+    { inherit name; }) cfg.feeds);
+}