Repository: nix-community/nixpkgs-fmt
Pull request #214 (2020-05-18 18:05:32)

Alternative interpolation indentation strategy
This is an approach alternative to #213.

Here, the idea is that we indent string block as a whole, without
looking into it contents at all. To that end we:

* introduce `walk_non_whitespace_non_interpol` funciton, which walks
the tree skipping interpolated nodes
* tweak `node_indent_ranges` to also just shift stuff inside
  intepolated fragments
* remove existing depth-based interpolation strategy


Patch:
diff --git a/src/engine.rs b/src/engine.rs
index b961c09..646b6b9 100644
--- a/src/engine.rs
+++ b/src/engine.rs
@@ -11,7 +11,7 @@ use crate::{
     dsl::{IndentDsl, RuleName, SpacingDsl},
     engine::fmt_model::{BlockPosition, FmtModel, SpaceBlock, SpaceBlockOrToken},
     pattern::PatternSet,
-    tree_utils::walk_non_whitespace,
+    tree_utils::{walk_non_whitespace, walk_non_whitespace_non_interpol},
     AtomEdit, FmtDiff,
 };
 
@@ -49,7 +49,7 @@ pub(crate) fn reformat(
     let mut model = FmtModel::new(node.clone());
 
     let anchor_set = PatternSet::new(indent_dsl.anchors.iter());
-    for element in walk_non_whitespace(&node) {
+    for element in walk_non_whitespace_non_interpol(&node) {
         let block = model.block_for(&element, BlockPosition::Before);
         if !block.has_newline() {
             // No need to indent an element if it doesn't start a line
@@ -81,7 +81,7 @@ pub(crate) fn reformat(
 
     // Finally, do custom touch-ups like re-indenting of string literals and
     // replacing URLs with string literals.
-    for element in walk_non_whitespace(&node) {
+    for element in walk_non_whitespace_non_interpol(&node) {
         fixes::fix(element, &mut model, &anchor_set)
     }
 
diff --git a/src/engine/fixes.rs b/src/engine/fixes.rs
index db1015c..d0251fa 100644
--- a/src/engine/fixes.rs
+++ b/src/engine/fixes.rs
@@ -2,37 +2,26 @@ use std::cmp::min;
 
 use rnix::{
     NodeOrToken, SyntaxElement,
-    SyntaxKind::{NODE_STRING, NODE_STRING_INTERPOL, TOKEN_COMMENT, TOKEN_STRING_CONTENT},
+    SyntaxKind::{NODE_STRING, TOKEN_COMMENT, TOKEN_STRING_CONTENT, TOKEN_WHITESPACE},
     SyntaxNode, SyntaxToken, TextRange, TextUnit,
 };
 
+use super::indentation::single_line_comment_indent;
 use crate::{
     engine::{
-        indentation::{
-            indent_anchor, indent_custom_anchor, single_line_comment_indent,
-            string_interpol_indent, IndentLevel,
-        },
+        indentation::{indent_anchor, IndentLevel},
         BlockPosition, FmtModel,
     },
     pattern::{Pattern, PatternSet},
-    tree_utils::walk_non_whitespace,
     AtomEdit,
 };
 
 pub(super) fn fix(element: SyntaxElement, model: &mut FmtModel, anchor_set: &PatternSet<&Pattern>) {
     match element {
         NodeOrToken::Node(node) => {
-            let multiline_string = node.children_with_tokens().any(|e| {
-                e.as_token()
-                    .map(|n| n.text().trim_start_matches(' ').starts_with('\n'))
-                    .unwrap_or(false)
-            });
-            if node.kind() == NODE_STRING && multiline_string {
+            if let NODE_STRING = node.kind() {
                 fix_string_indentation(&node, model, anchor_set)
             }
-            if node.kind() == NODE_STRING_INTERPOL {
-                fix_string_interpolation(&node, model, anchor_set)
-            }
         }
         NodeOrToken::Token(token) => {
             if let TOKEN_COMMENT = token.kind() {
@@ -47,39 +36,15 @@ fn fix_string_indentation(
     model: &mut FmtModel,
     anchor_set: &PatternSet<&Pattern>,
 ) {
-    let element: SyntaxElement = node.clone().into();
     let quote_indent = {
-        let inside_interpolation = node.ancestors().any(|e| e.kind() == NODE_STRING_INTERPOL);
-        let default_indent = IndentLevel::default();
+        let element: SyntaxElement = node.clone().into();
         let block = model.block_for(&element, BlockPosition::Before);
         if block.text().contains('\n') {
             IndentLevel::from_whitespace_block(block.text())
         } else {
-            let multiline_interpol_string = node
-                .ancestors()
-                .find(|e| e.kind() == NODE_STRING_INTERPOL)
-                .map(|n| {
-                    n.descendants_with_tokens()
-                        .take_while(|d| d.kind() != NODE_STRING)
-                        .any(|t| t.as_token().map(|e| e.text().contains("\n")).unwrap_or(false))
-                })
-                .unwrap_or(false);
-
-            if inside_interpolation {
-                if multiline_interpol_string {
-                    match indent_anchor(&element, model, anchor_set) {
-                        None => return,
-                        Some((_element, indent)) => indent,
-                    }
-                } else {
-                    indent_custom_anchor(&element, model, NODE_STRING_INTERPOL, anchor_set)
-                        .unwrap_or(default_indent)
-                }
-            } else {
-                match indent_anchor(&element, model, anchor_set) {
-                    None => return,
-                    Some((_element, indent)) => indent,
-                }
+            match indent_anchor(&element, model, anchor_set) {
+                None => return,
+                Some((_element, indent)) => indent,
             }
         }
     };
@@ -176,20 +141,6 @@ fn fix_comment_indentation(
     }
 }
 
-fn fix_string_interpolation(
-    node: &SyntaxNode,
-    model: &mut FmtModel,
-    anchor_set: &PatternSet<&Pattern>,
-) {
-    for element in walk_non_whitespace(node) {
-        if element.parent().map(|it| it.text_range().start()) == Some(element.text_range().start())
-        {
-            continue;
-        }
-        string_interpol_indent(&element, model, anchor_set);
-    }
-}
-
 /// For indented string like
 ///
 /// ```nix
@@ -203,9 +154,9 @@ fn fix_string_interpolation(
 /// hello, `"    "` before world and `""` before the last `''`.
 fn node_indent_ranges(indented_string: &SyntaxNode) -> impl Iterator<Item = TextRange> {
     indented_string
-        .children_with_tokens()
+        .descendants_with_tokens()
         .filter_map(|it| it.into_token())
-        .filter(|it| it.kind() == TOKEN_STRING_CONTENT)
+        .filter(|it| it.kind() == TOKEN_STRING_CONTENT || it.kind() == TOKEN_WHITESPACE)
         .flat_map(|string_bit| {
             let start_offset = string_bit.text_range().start();
             string_indent_ranges(string_bit.text())
diff --git a/src/engine/indentation.rs b/src/engine/indentation.rs
index b86f586..f30189f 100644
--- a/src/engine/indentation.rs
+++ b/src/engine/indentation.rs
@@ -3,13 +3,13 @@ use std::{
     fmt,
 };
 
-use rnix::{SmolStr, SyntaxElement, SyntaxKind, SyntaxKind::*, SyntaxNode, SyntaxToken, TextUnit};
+use rnix::{SmolStr, SyntaxElement, SyntaxKind::*, SyntaxNode, SyntaxToken, TextUnit};
 
 use crate::{
     dsl::{IndentRule, Modality, RuleName},
     engine::{BlockPosition, FmtModel, SpaceBlock, SpaceBlockOrToken},
     pattern::{Pattern, PatternSet},
-    tree_utils::{prev_non_whitespace_token_sibling, prev_token_sibling},
+    tree_utils::prev_non_whitespace_token_sibling,
 };
 
 const INDENT_SIZE: u32 = 2;
@@ -184,51 +184,6 @@ pub(super) fn default_indent(
     block.set_indent(anchor_indent, RuleName::new("Preserve indentation"));
 }
 
-pub(super) fn string_interpol_indent(
-    element: &SyntaxElement,
-    model: &mut FmtModel,
-    anchor_set: &PatternSet<&Pattern>,
-) {
-    let indent = IndentLevel::default();
-    let anchor_indent = match indent_anchor(&element, model, anchor_set) {
-        Some((_anchor, indent)) => indent,
-        _ => IndentLevel::default(),
-    };
-
-    let anchor_custom =
-        indent_custom_anchor(&element, model, NODE_STRING_INTERPOL, anchor_set).unwrap_or(indent);
-
-    let block = model.block_for(&element, BlockPosition::Before);
-    if !block.has_newline() {
-        // No need to indent an element if it doesn't start a line
-        return;
-    }
-    match element {
-        e if e.kind() == NODE_APPLY => {
-            let node_string_count =
-                e.ancestors().filter(|e| e.kind() == NODE_STRING_INTERPOL).count();
-            let node_string = e
-                .ancestors()
-                .find(|e| e.kind() == NODE_STRING)
-                .and_then(|t| prev_token_sibling(&t.into()).map(|e| e.text().contains("\n")))
-                .unwrap_or(false);
-            if node_string_count < 2 && !node_string {
-                block
-                    .set_indent(anchor_custom.indent(), RuleName::new("String Interpolation Value"))
-            } else {
-                block.set_indent(anchor_custom, RuleName::new("String Interpolation Value"))
-            }
-        }
-        e if e.kind() == TOKEN_INTERPOL_END => {
-            block.set_indent(anchor_indent.indent(), RuleName::new("String Interpolation Value"))
-        }
-        e if e.kind() == TOKEN_SQUARE_B_CLOSE => {
-            block.set_indent(anchor_custom, RuleName::new("String Interpolation Value"))
-        }
-        _ => block.set_indent(anchor_custom.indent(), RuleName::new("String Interpolation Value")),
-    }
-}
-
 pub(super) fn single_line_comment_indent(
     token: &SyntaxToken,
     model: &mut FmtModel,
@@ -280,29 +235,6 @@ pub(super) fn indent_anchor(
     None
 }
 
-pub(super) fn indent_custom_anchor(
-    element: &SyntaxElement,
-    model: &mut FmtModel,
-    kind: SyntaxKind,
-    anchor_set: &PatternSet<&Pattern>,
-) -> Option<IndentLevel> {
-    let indent = IndentLevel::default();
-    // Calculates current indent on NODE_STRING_INTERPOLATION inside NODE_STRING
-    let parent = element.parent()?;
-    let init_indent = match indent_anchor(&parent.clone().into(), model, anchor_set) {
-        None => indent,
-        Some((_, top_indent)) => top_indent,
-    };
-
-    let original_anchor = parent.ancestors().filter(|e| e.kind() == kind);
-    fn count_indent(mut acc: IndentLevel, _node: SyntaxNode) -> IndentLevel {
-        acc += IndentLevel::default().indent();
-        acc
-    }
-    let node_indent = original_anchor.fold(init_indent, count_indent);
-    Some(node_indent)
-}
-
 impl FmtModel {
     /// Calculates current indent level for node.
     fn indent_of(&mut self, node: &SyntaxNode) -> IndentLevel {
diff --git a/src/tree_utils.rs b/src/tree_utils.rs
index 2e37a1d..7e5c2a5 100644
--- a/src/tree_utils.rs
+++ b/src/tree_utils.rs
@@ -3,8 +3,8 @@ use std::iter::successors;
 use rnix::{
     NodeOrToken, SyntaxElement,
     SyntaxKind::{
-        NODE_APPLY, NODE_ASSERT, NODE_IF_ELSE, NODE_LAMBDA, NODE_LET_IN, NODE_ROOT, NODE_WITH,
-        TOKEN_WHITESPACE,
+        NODE_APPLY, NODE_ASSERT, NODE_IF_ELSE, NODE_LAMBDA, NODE_LET_IN, NODE_ROOT,
+        NODE_STRING_INTERPOL, NODE_WITH, TOKEN_WHITESPACE,
     },
     SyntaxNode, SyntaxToken, WalkEvent,
 };
@@ -22,6 +22,29 @@ pub(crate) fn walk_non_whitespace(node: &SyntaxNode) -> impl Iterator<Item = Syn
     })
 }
 
+pub(crate) fn walk_non_whitespace_non_interpol(
+    node: &SyntaxNode,
+) -> impl Iterator<Item = SyntaxElement> {
+    let mut interpool_level = 0;
+    node.preorder_with_tokens().filter_map(move |event| {
+        match &event {
+            WalkEvent::Enter(element) if element.kind() == NODE_STRING_INTERPOL => {
+                interpool_level += 1
+            }
+            WalkEvent::Leave(element) if element.kind() == NODE_STRING_INTERPOL => {
+                interpool_level -= 1
+            }
+            _ => (),
+        }
+        match event {
+            WalkEvent::Enter(element) => {
+                Some(element).filter(|it| interpool_level == 0 && it.kind() != TOKEN_WHITESPACE)
+            }
+            WalkEvent::Leave(_) => None,
+        }
+    })
+}
+
 pub(crate) fn walk_tokens(node: &SyntaxNode) -> impl Iterator<Item = SyntaxToken> {
     walk(node).filter_map(|element| match element {
         NodeOrToken::Token(token) => Some(token),


Test patch:
diff --git a/test_data/indent_string_literal_interpolation.bad.nix b/test_data/indent_string_literal_interpolation.bad.nix
index ec61f44..f76540f 100644
--- a/test_data/indent_string_literal_interpolation.bad.nix
+++ b/test_data/indent_string_literal_interpolation.bad.nix
@@ -9,68 +9,55 @@ postFixup =  ''
       '';
 
 foo = ''
-    bar = ${builtins.concatStringsSep " " [
-1
-2
-3
-]}
-    bla = hoi
-  '';
+  bar = ${builtins.concatStringsSep " " [
+    1
+    2
+    3
+  ]}
+  bla = hoi
+'';
 
 bar = ''
-foo
-${
-foo
-}
-foo
+  foo
+  ${
+    foo
+  }
+  foo
   '';
 
 baz =
     ''
 foo
 ${
-foo
+  foo
 }
 foo
     '';
 
 qux =
-    ''
+  ''
     bar = ${builtins.concatStringsSep " " [
-1
-2
-3
-]}
+      1
+      2
+      3
+    ]}
     bla = hoi
-    '';
-
-singleAsciiDoc = value: ''
-  ${
-if lib.hasAttr "example" value
-then ''
-Example::
-${
-builtins.toJSON value.example
-}
-''
-else "No Example:: {blank}"
-    }
   '';
 
 nested_antiquotation = mkBefore
   ''
-  ${optionalString cfg.earlySetup ''
-  ${optionalString cfg.earlySetup ''
-  setfont -C /dev/console $extraUtils/share/consolefonts/font.psf
-  ''}
-  setfont -C /dev/console $extraUtils/share/consolefonts/font.psf
-  ''}
+    ${optionalString cfg.earlySetup ''
+      ${optionalString cfg.earlySetup ''
+        setfont -C /dev/console $extraUtils/share/consolefonts/font.psf
+      ''}
+      setfont -C /dev/console $extraUtils/share/consolefonts/font.psf
+    ''}
   '';
 
 singleAsciiDoc = value: ''
 Example::
 ${
-builtins.toJSON value.example
+  builtins.toJSON value.example
 }
 '';
 }
diff --git a/test_data/indent_string_literal_interpolation.good.nix b/test_data/indent_string_literal_interpolation.good.nix
index 1534054..36adc04 100644
--- a/test_data/indent_string_literal_interpolation.good.nix
+++ b/test_data/indent_string_literal_interpolation.good.nix
@@ -44,19 +44,6 @@
       bla = hoi
     '';
 
-  singleAsciiDoc = value: ''
-    ${
-      if lib.hasAttr "example" value
-      then ''
-        Example::
-        ${
-          builtins.toJSON value.example
-        }
-      ''
-      else "No Example:: {blank}"
-    }
-  '';
-
   nested_antiquotation =
     mkBefore
       ''
diff --git a/test_data/issue-199.bad.nix b/test_data/issue-199.bad.nix
index a2fc44e..bc286d9 100644
--- a/test_data/issue-199.bad.nix
+++ b/test_data/issue-199.bad.nix
@@ -1,7 +1,7 @@
 let
 # comment
 
- 
+
 a = (if true then 1 else 2);
 b = "${if true then "1" else "2"}";
 
@@ -10,18 +10,18 @@ b = "${if true then "1" else "2"}";
 
 
 c = ''
- ${foo (if true then ''
-   bla
- '' else ''
-   blub
+  ${foo (if true then ''
+    bla
+  '' else ''
+    blub
   '')}
 '';
 
 d = ''
- ${if true then ''
-   bla
- '' else ''
-   blub
+  ${if true then ''
+    bla
+  '' else ''
+    blub
   ''}
 '';
 
diff --git a/test_data/nixpkgs_repository/lib_strings.good.nix b/test_data/nixpkgs_repository/lib_strings.good.nix
index 9f91ea4..92b7e3c 100644
--- a/test_data/nixpkgs_repository/lib_strings.good.nix
+++ b/test_data/nixpkgs_repository/lib_strings.good.nix
@@ -7,7 +7,7 @@
     assert lib.assertMsg
       (strw <= width)
       "fixedWidthString: requested string length (${
-        toString width}) must not be shorter than actual length (${
-        toString strw})";
+          toString width}) must not be shorter than actual length (${
+          toString strw})";
     if strw == width then str else filler + fixedWidthString reqWidth filler str;
 }
diff --git a/test_data/nixpkgs_repository/nixos_lib_testing_python.good.nix b/test_data/nixpkgs_repository/nixos_lib_testing_python.good.nix
index 92b84e0..3482503 100644
--- a/test_data/nixpkgs_repository/nixos_lib_testing_python.good.nix
+++ b/test_data/nixpkgs_repository/nixos_lib_testing_python.good.nix
@@ -13,7 +13,7 @@
           mkdir -p $out/bin
           echo -n "$testScript" > $out/test-script
           ${lib.optionalString (!skipLint) ''
-            ${python3Packages.black}/bin/black --check --diff $out/test-script
+          ${python3Packages.black}/bin/black --check --diff $out/test-script
           ''}
           ln -s ${testDriver}/bin/nixos-test-driver $out/bin/
           vms=($(for i in ${toString vms}; do echo $i/bin/run-*-vm; done))
diff --git a/test_data/nixpkgs_repository/nixos_modules_config_console.good.nix b/test_data/nixpkgs_repository/nixos_modules_config_console.good.nix
index 6d3693d..9afcab1 100644
--- a/test_data/nixpkgs_repository/nixos_modules_config_console.good.nix
+++ b/test_data/nixpkgs_repository/nixos_modules_config_console.good.nix
@@ -1,10 +1,10 @@
 {
   boot.initrd.preLVMCommands = mkBefore ''
-    kbd_mode ${if isUnicode then "-u" else "-a"} -C /dev/console
-    printf "\033%%${if isUnicode then "G" else "@"}" >> /dev/console
-    loadkmap < ${optimizedKeymap}
-    ${optionalString cfg.earlySetup ''
-      setfont -C /dev/console $extraUtils/share/consolefonts/font.psf
+      kbd_mode ${if isUnicode then "-u" else "-a"} -C /dev/console
+      printf "\033%%${if isUnicode then "G" else "@"}" >> /dev/console
+      loadkmap < ${optimizedKeymap}
+      ${optionalString cfg.earlySetup ''
+    setfont -C /dev/console $extraUtils/share/consolefonts/font.psf
     ''}
   '';
 }