From 81a0624d7613dbd5a4fc69cde95314e7db47bd43 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Mon, 15 Jul 2024 18:00:29 +0200 Subject: [PATCH 1/4] libexpr/print: pretty-print idempotently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When pretty-printing is enabled, previously an unforced thunk would trigger indentation, even when it subsequently does not evaluate to a nested structure. The resulting output looked inconsistent, and furthermore pretty-printing was not idempotent (since pretty-printing the same value again, which is now fully evaluated, will not trigger indentation). When strict evaluation is enabled, force the item before inspecting its type, so that it is properly known whether it contains a nested structure. Furthermore, there is no need to cause indentation for unforced thunks, since the very next operation will be printing them as `«thunk»`. This is mostly a port of https://github.com/NixOS/nix/pull/11100 , but we only force the item when it's going to be forced anyway due to strict pretty-printing, and a new test was written since the REPL testing framework in Lix is different. Co-Authored-By: Robert Hensing Change-Id: Ib7560fe531d09e05ca6b2037a523fe21a26d9d58 --- src/libexpr/print.cc | 14 ++++++++++++-- .../repl_characterization/data/idempotent.test | 13 +++++++++++++ .../repl_characterization/repl_characterization.cc | 1 + 3 files changed, 26 insertions(+), 2 deletions(-) create mode 100644 tests/functional/repl_characterization/data/idempotent.test diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 87db004b2..6eb321c43 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -264,10 +264,15 @@ private: return true; } + if (options.force) { + // The item is going to be forced during printing anyway, but we need its type now. + state.forceValue(*item, item->determinePos(noPos)); + } + // Pretty-print single-item attrsets only if they contain nested // structures. auto itemType = item->type(); - return itemType == nList || itemType == nAttrs || itemType == nThunk; + return itemType == nList || itemType == nAttrs; } void printAttrs(Value & v, size_t depth) @@ -335,10 +340,15 @@ private: return true; } + if (options.force) { + // The item is going to be forced during printing anyway, but we need its type now. + state.forceValue(*item, item->determinePos(noPos)); + } + // Pretty-print single-item lists only if they contain nested // structures. auto itemType = item->type(); - return itemType == nList || itemType == nAttrs || itemType == nThunk; + return itemType == nList || itemType == nAttrs; } void printList(Value & v, size_t depth) diff --git a/tests/functional/repl_characterization/data/idempotent.test b/tests/functional/repl_characterization/data/idempotent.test new file mode 100644 index 000000000..4ab087d45 --- /dev/null +++ b/tests/functional/repl_characterization/data/idempotent.test @@ -0,0 +1,13 @@ +A previously unforced thunk in an attribute set does not lead to indentation when it won't evaluate to a nested structure: + nix-repl> :p let x = 1 + 2; in [ { inherit x; } { inherit x; } ] + [ + { x = 3; } + { x = 3; } + ] + +Same for a list: + nix-repl> :p let x = 1 + 2; in [ [ x ] [ x ] ] + [ + [ 3 ] + [ 3 ] + ] diff --git a/tests/functional/repl_characterization/repl_characterization.cc b/tests/functional/repl_characterization/repl_characterization.cc index c91d6c1e3..f2cf0ca3d 100644 --- a/tests/functional/repl_characterization/repl_characterization.cc +++ b/tests/functional/repl_characterization/repl_characterization.cc @@ -185,5 +185,6 @@ REPL_TEST(repl_overlays_error); REPL_TEST(repl_printing); REPL_TEST(stack_vars); REPL_TEST(errors); +REPL_TEST(idempotent); }; // namespace nix From b5da823138023957d91eb013c2a6350ef6032a80 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Mon, 15 Jul 2024 18:24:16 +0200 Subject: [PATCH 2/4] =?UTF-8?q?libexpr/print:=20never=20show=20empty=20att?= =?UTF-8?q?rsets=20or=20derivations=20as=20=C2=ABrepeated=C2=BB?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repeated value detection logic exists so that the occurrence of large common substructures does not fill up the screen or the computer's memory. However, empty attribute sets and derivations (when their detection is enabled) are always cheap to print, and in practice I have observed them to make up a significant majority of the cases where I was annoyed by the repeated value detection kicking in. Furthermore, `nix-instantiate --eval` already disables this logic for empty attribute sets, and empty lists are already exempted everywhere. For these reasons, always print empty attribute sets and derivations as what they are. Change-Id: I5dac8e7739f9d726b76fd0521ec46f38af94463f --- src/libexpr/print.cc | 7 ++--- tests/unit/libexpr/value/print.cc | 50 +++++++++++++++++++------------ 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 6eb321c43..2ee11d9d3 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -277,13 +277,10 @@ private: void printAttrs(Value & v, size_t depth) { - if (seen && !seen->insert(v.attrs).second) { - printRepeated(); - return; - } - if (options.force && options.derivationPaths && state.isDerivation(v)) { printDerivation(v); + } else if (seen && !v.attrs->empty() && !seen->insert(v.attrs).second) { + printRepeated(); } else if (depth < options.maxDepth) { increaseIndent(); output << "{"; diff --git a/tests/unit/libexpr/value/print.cc b/tests/unit/libexpr/value/print.cc index cdbc8f8f9..1f99b562d 100644 --- a/tests/unit/libexpr/value/print.cc +++ b/tests/unit/libexpr/value/print.cc @@ -641,20 +641,24 @@ TEST_F(ValuePrintingTests, ansiColorsBlackhole) TEST_F(ValuePrintingTests, ansiColorsAttrsRepeated) { - BindingsBuilder emptyBuilder(state, state.allocBindings(1)); + Value vZero; + vZero.mkInt(0); - Value vEmpty; - vEmpty.mkAttrs(emptyBuilder.finish()); + BindingsBuilder innerBuilder(state, state.allocBindings(1)); + innerBuilder.insert(state.symbols.create("x"), &vZero); + + Value vInner; + vInner.mkAttrs(innerBuilder.finish()); BindingsBuilder builder(state, state.allocBindings(10)); - builder.insert(state.symbols.create("a"), &vEmpty); - builder.insert(state.symbols.create("b"), &vEmpty); + builder.insert(state.symbols.create("a"), &vInner); + builder.insert(state.symbols.create("b"), &vInner); Value vAttrs; vAttrs.mkAttrs(builder.finish()); test(vAttrs, - "{ a = { }; b = " ANSI_MAGENTA "«repeated»" ANSI_NORMAL "; }", + "{ a = { x = " ANSI_CYAN "0" ANSI_NORMAL "; }; b = " ANSI_MAGENTA "«repeated»" ANSI_NORMAL "; }", PrintOptions { .ansiColors = true }); @@ -662,19 +666,23 @@ TEST_F(ValuePrintingTests, ansiColorsAttrsRepeated) TEST_F(ValuePrintingTests, ansiColorsListRepeated) { - BindingsBuilder emptyBuilder(state, state.allocBindings(1)); + Value vZero; + vZero.mkInt(0); - Value vEmpty; - vEmpty.mkAttrs(emptyBuilder.finish()); + BindingsBuilder innerBuilder(state, state.allocBindings(1)); + innerBuilder.insert(state.symbols.create("x"), &vZero); + + Value vInner; + vInner.mkAttrs(innerBuilder.finish()); Value vList; state.mkList(vList, 3); - vList.bigList.elems[0] = &vEmpty; - vList.bigList.elems[1] = &vEmpty; + vList.bigList.elems[0] = &vInner; + vList.bigList.elems[1] = &vInner; vList.bigList.size = 2; test(vList, - "[ { } " ANSI_MAGENTA "«repeated»" ANSI_NORMAL " ]", + "[ { x = " ANSI_CYAN "0" ANSI_NORMAL "; } " ANSI_MAGENTA "«repeated»" ANSI_NORMAL " ]", PrintOptions { .ansiColors = true }); @@ -682,20 +690,24 @@ TEST_F(ValuePrintingTests, ansiColorsListRepeated) TEST_F(ValuePrintingTests, listRepeated) { - BindingsBuilder emptyBuilder(state, state.allocBindings(1)); + Value vZero; + vZero.mkInt(0); - Value vEmpty; - vEmpty.mkAttrs(emptyBuilder.finish()); + BindingsBuilder innerBuilder(state, state.allocBindings(1)); + innerBuilder.insert(state.symbols.create("x"), &vZero); + + Value vInner; + vInner.mkAttrs(innerBuilder.finish()); Value vList; state.mkList(vList, 3); - vList.bigList.elems[0] = &vEmpty; - vList.bigList.elems[1] = &vEmpty; + vList.bigList.elems[0] = &vInner; + vList.bigList.elems[1] = &vInner; vList.bigList.size = 2; - test(vList, "[ { } «repeated» ]", PrintOptions { }); + test(vList, "[ { x = 0; } «repeated» ]", PrintOptions { }); test(vList, - "[ { } { } ]", + "[ { x = 0; } { x = 0; } ]", PrintOptions { .trackRepeated = false }); From 40c39aa5d2e8ff64c88d293744a3b7ba643a2274 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Mon, 15 Jul 2024 18:38:37 +0200 Subject: [PATCH 3/4] libexpr/print: do not show elided nested items when there are none When the configured maximum depth has been reached, attribute sets and lists are printed with ellipsis to indicate the elision of nested items. Previously, this happened even in case the structure being printed is empty, so that such items do not in fact exist. This is confusing, so stop doing it. Change-Id: I0016970dad3e42625e085dc896e6f476b21226c9 --- src/libexpr/print.cc | 4 ++-- tests/unit/libexpr/value/print.cc | 17 +++++++++++++---- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 2ee11d9d3..b7ff50f48 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -281,7 +281,7 @@ private: printDerivation(v); } else if (seen && !v.attrs->empty() && !seen->insert(v.attrs).second) { printRepeated(); - } else if (depth < options.maxDepth) { + } else if (depth < options.maxDepth || v.attrs->empty()) { increaseIndent(); output << "{"; @@ -355,7 +355,7 @@ private: return; } - if (depth < options.maxDepth) { + if (depth < options.maxDepth || v.listSize() == 0) { increaseIndent(); output << "["; auto listItems = v.listItems(); diff --git a/tests/unit/libexpr/value/print.cc b/tests/unit/libexpr/value/print.cc index 1f99b562d..ddf8b8b99 100644 --- a/tests/unit/libexpr/value/print.cc +++ b/tests/unit/libexpr/value/print.cc @@ -193,6 +193,9 @@ TEST_F(ValuePrintingTests, vBlackhole) TEST_F(ValuePrintingTests, depthAttrs) { + Value vZero; + vZero.mkInt(0); + Value vOne; vOne.mkInt(1); @@ -203,10 +206,16 @@ TEST_F(ValuePrintingTests, depthAttrs) Value vAttrsEmpty; vAttrsEmpty.mkAttrs(builderEmpty.finish()); + BindingsBuilder builderNested(state, state.allocBindings(1)); + builderNested.insert(state.symbols.create("zero"), &vZero); + Value vAttrsNested; + vAttrsNested.mkAttrs(builderNested.finish()); + BindingsBuilder builder(state, state.allocBindings(10)); builder.insert(state.symbols.create("one"), &vOne); builder.insert(state.symbols.create("two"), &vTwo); - builder.insert(state.symbols.create("nested"), &vAttrsEmpty); + builder.insert(state.symbols.create("empty"), &vAttrsEmpty); + builder.insert(state.symbols.create("nested"), &vAttrsNested); Value vAttrs; vAttrs.mkAttrs(builder.finish()); @@ -220,9 +229,9 @@ TEST_F(ValuePrintingTests, depthAttrs) vNested.mkAttrs(builder2.finish()); test(vNested, "{ nested = { ... }; one = 1; two = 2; }", PrintOptions { .maxDepth = 1 }); - test(vNested, "{ nested = { nested = { ... }; one = 1; two = 2; }; one = 1; two = 2; }", PrintOptions { .maxDepth = 2 }); - test(vNested, "{ nested = { nested = { }; one = 1; two = 2; }; one = 1; two = 2; }", PrintOptions { .maxDepth = 3 }); - test(vNested, "{ nested = { nested = { }; one = 1; two = 2; }; one = 1; two = 2; }", PrintOptions { .maxDepth = 4 }); + test(vNested, "{ nested = { empty = { }; nested = { ... }; one = 1; two = 2; }; one = 1; two = 2; }", PrintOptions { .maxDepth = 2 }); + test(vNested, "{ nested = { empty = { }; nested = { zero = 0; }; one = 1; two = 2; }; one = 1; two = 2; }", PrintOptions { .maxDepth = 3 }); + test(vNested, "{ nested = { empty = { }; nested = { zero = 0; }; one = 1; two = 2; }; one = 1; two = 2; }", PrintOptions { .maxDepth = 4 }); } TEST_F(ValuePrintingTests, depthList) From 768d1f29a2dcd9ed9552fafb4ab836ea2e400738 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Thu, 18 Jul 2024 19:08:20 +0200 Subject: [PATCH 4/4] doc/release-notes: add for pretty printing improvements Change-Id: I829581a3f5b8b742e6c866dcdbbc635f91afceb5 --- doc/manual/rl-next/pretty-printing.md | 58 +++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 doc/manual/rl-next/pretty-printing.md diff --git a/doc/manual/rl-next/pretty-printing.md b/doc/manual/rl-next/pretty-printing.md new file mode 100644 index 000000000..f7953f9ff --- /dev/null +++ b/doc/manual/rl-next/pretty-printing.md @@ -0,0 +1,58 @@ +--- +synopsis: "Eliminate some pretty-printing surprises" +cls: [1616, 1617, 1618] +prs: [11100] +credits: [alois31, roberth] +category: Improvements +--- + +Some inconsistent and surprising behaviours have been eliminated from the pretty-printing used by the REPL and `nix eval`: +* Lists and attribute sets that contain only a single item without nested structures are no longer sometimes inappropriately indented in the REPL, depending on internal state of the evaluator. +* Empty attribute sets and derivations are no longer shown as `«repeated»`, since they are always cheap to print. + This matches the existing behaviour of `nix-instantiate` on empty attribute sets. + Empty lists were never printed as `«repeated»` already. +* The REPL by default does not print nested attribute sets and lists, and indicates elided items with an ellipsis. + Previously, the ellipsis was printed even when the structure was empty, so that such items do not in fact exist. + Since this behaviour was confusing, it does not happen any more. + +Before: +``` +nix-repl> :p let x = 1 + 2; in [ [ x ] [ x ] ] +[ + [ + 3 + ] + [ 3 ] +] + +nix-repl> let inherit (import { }) hello; in [ hello hello ] +[ + «derivation /nix/store/fqs92lzychkm6p37j7fnj4d65nq9fzla-hello-2.12.1.drv» + «repeated» +] + +nix-repl> let x = {}; in [ x ] +[ + { ... } +] +``` + +After: +``` +nix-repl> :p let x = 1 + 2; in [ [ x ] [ x ] ] +[ + [ 3 ] + [ 3 ] +] + +nix-repl> let inherit (import { }) hello; in [ hello hello ] +[ + «derivation /nix/store/fqs92lzychkm6p37j7fnj4d65nq9fzla-hello-2.12.1.drv» + «derivation /nix/store/fqs92lzychkm6p37j7fnj4d65nq9fzla-hello-2.12.1.drv» +] + +nix-repl> let x = {}; in [ x ] +[ + { } +] +```