kkinnear/zprint

Could some formatting decisions be based on `sexpr`ed nodes?

lread opened this issue · 5 comments

lread commented

Hi @kkinnear, I hope all is well with you!

While reviewing PR clj-commons/rewrite-clj#306 for rewrite-clj, I noticed that a single zprint test failed when running zprint 1.2.9 tests against the changes in the rewrite-clj PR.

Here's the failing test:

FAIL in (guide-tests) (guide_test.cljc:2538)
(zprint-str jr3 {:parse-string? true, :fn-map {":require" [:none {:list {:option-fn (partial jrequireguide :require)}}]}})

matches: "(ns ^:no-doc zprint.zprint\n  #?@(:cljs [[:require-macros\n              [zprint.macros :refer [dbg dbg-pr dbg-form dbg-print zfuture]]]])\n  (:require\n    #?@(:clj [[zprint.macros :refer [dbg-pr dbg dbg-form dbg-print zfuture]]])\n    [clojure.string     "
>>>  expected diverges: ":as s]\n    [zprint.finish      :refer [newline-vec]]\n    [zprint.zfns        :refer [zstring znumstr zbyte-array? zcomment? zsexpr\n                                zseqnws zseqnws-w-nl zfocus-style zstart zfirst\n                                zfirst-no-comment zsecond znthnext zcount zmap\n                                zanonfn? zfn-obj? zfocus zfind-path zwhitespace?\n                                zlist? zcount-zloc-seq-nc-nws zvector? zmap?\n                                zset? zcoll? zuneval? zmeta? ztag zlast zarray?\n                                zatom? zderef zrecord? zns? zobj-to-vec\n                                zexpandarray znewline? zwhitespaceorcomment?\n                                zmap-all zpromise? zfuture? zdelay? zkeyword?\n                                zconstant? zagent? zreader-macro?\n                                zarray-to-shift-seq zdotdotdot zsymbol? znil?\n                                zreader-cond-w-symbol? zreader-cond-w-coll?\n                                zlift-ns zfind zmap-w-nl zmap-w-nl-comma\n                                ztake-append znextnws-w-nl znextnws\n                                znamespacedmap? zmap-w-bl zseqnws-w-bl zsexpr?]]\n    [zprint.comment     :refer [blanks inlinecomment? length-before]]\n    '[zprint.ansi :refer [color-str]]\n    ~`[zprint.config :refer [validate-options merge-deep]]\n    [zprint.zutil       :refer [add-spec-to-docstring]]\n    [rewrite-clj.parser :as p]\n    [rewrite-clj.zip    :as z]\n    #_[taoensso.tufte :as tufte :refer (p defnp profiled profile)]))"
>>>    actual diverges: "  :as s]\n    [zprint.finish        :refer [newline-vec]]\n    [zprint.zfns          :refer\n                            [zstring znumstr zbyte-array? zcomment? zsexpr\n                             zseqnws zseqnws-w-nl zfocus-style zstart zfirst\n                             zfirst-no-comment zsecond znthnext zcount zmap\n                             zanonfn? zfn-obj? zfocus zfind-path zwhitespace?\n                             zlist? zcount-zloc-seq-nc-nws zvector? zmap? zset?\n                             zcoll? zuneval? zmeta? ztag zlast zarray? zatom?\n                             zderef zrecord? zns? zobj-to-vec zexpandarray\n                             znewline? zwhitespaceorcomment? zmap-all zpromise?\n                             zfuture? zdelay? zkeyword? zconstant? zagent?\n                             zreader-macro? zarray-to-shift-seq zdotdotdot\n                             zsymbol? znil? zreader-cond-w-symbol?\n                             zreader-cond-w-coll? zlift-ns zfind zmap-w-nl\n                             zmap-w-nl-comma ztake-append znextnws-w-nl znextnws\n                             znamespacedmap? zmap-w-bl zseqnws-w-bl zsexpr?]]\n    [zprint.comment       :refer [blanks inlinecomment? length-before]]\n    '[zprint.ansi :refer [color-str]]\n    ~`[zprint.config :refer [validate-options merge-deep]]\n    [zprint.zutil         :refer [add-spec-to-docstring]]\n    [rewrite-clj.parser   :as p]\n    [rewrite-clj.zip      :as z]\n    #_[taoensso.tufte :as tufte :refer (p defnp profiled profile)]))"

expected: "(ns ^:no-doc zprint.zprint\n  #?@(:cljs [[:require-macros\n              [zprint.macros :refer [dbg dbg-pr dbg-form dbg-print zfuture]]]])\n  (:require\n    #?@(:clj [[zprint.macros :refer [dbg-pr dbg dbg-form dbg-print zfuture]]])\n    [clojure.string     :as s]\n    [zprint.finish      :refer [newline-vec]]\n    [zprint.zfns        :refer [zstring znumstr zbyte-array? zcomment? zsexpr\n                                zseqnws zseqnws-w-nl zfocus-style zstart zfirst\n                                zfirst-no-comment zsecond znthnext zcount zmap\n                                zanonfn? zfn-obj? zfocus zfind-path zwhitespace?\n                                zlist? zcount-zloc-seq-nc-nws zvector? zmap?\n                                zset? zcoll? zuneval? zmeta? ztag zlast zarray?\n                                zatom? zderef zrecord? zns? zobj-to-vec\n                                zexpandarray znewline? zwhitespaceorcomment?\n                                zmap-all zpromise? zfuture? zdelay? zkeyword?\n                                zconstant? zagent? zreader-macro?\n                                zarray-to-shift-seq zdotdotdot zsymbol? znil?\n                                zreader-cond-w-symbol? zreader-cond-w-coll?\n                                zlift-ns zfind zmap-w-nl zmap-w-nl-comma\n                                ztake-append znextnws-w-nl znextnws\n                                znamespacedmap? zmap-w-bl zseqnws-w-bl zsexpr?]]\n    [zprint.comment     :refer [blanks inlinecomment? length-before]]\n    '[zprint.ansi :refer [color-str]]\n    ~`[zprint.config :refer [validate-options merge-deep]]\n    [zprint.zutil       :refer [add-spec-to-docstring]]\n    [rewrite-clj.parser :as p]\n    [rewrite-clj.zip    :as z]\n    #_[taoensso.tufte :as tufte :refer (p defnp profiled profile)]))"
  actual: "(ns ^:no-doc zprint.zprint\n  #?@(:cljs [[:require-macros\n              [zprint.macros :refer [dbg dbg-pr dbg-form dbg-print zfuture]]]])\n  (:require\n    #?@(:clj [[zprint.macros :refer [dbg-pr dbg dbg-form dbg-print zfuture]]])\n    [clojure.string       :as s]\n    [zprint.finish        :refer [newline-vec]]\n    [zprint.zfns          :refer\n                            [zstring znumstr zbyte-array? zcomment? zsexpr\n                             zseqnws zseqnws-w-nl zfocus-style zstart zfirst\n                             zfirst-no-comment zsecond znthnext zcount zmap\n                             zanonfn? zfn-obj? zfocus zfind-path zwhitespace?\n                             zlist? zcount-zloc-seq-nc-nws zvector? zmap? zset?\n                             zcoll? zuneval? zmeta? ztag zlast zarray? zatom?\n                             zderef zrecord? zns? zobj-to-vec zexpandarray\n                             znewline? zwhitespaceorcomment? zmap-all zpromise?\n                             zfuture? zdelay? zkeyword? zconstant? zagent?\n                             zreader-macro? zarray-to-shift-seq zdotdotdot\n                             zsymbol? znil? zreader-cond-w-symbol?\n                             zreader-cond-w-coll? zlift-ns zfind zmap-w-nl\n                             zmap-w-nl-comma ztake-append znextnws-w-nl znextnws\n                             znamespacedmap? zmap-w-bl zseqnws-w-bl zsexpr?]]\n    [zprint.comment       :refer [blanks inlinecomment? length-before]]\n    '[zprint.ansi :refer [color-str]]\n    ~`[zprint.config :refer [validate-options merge-deep]]\n    [zprint.zutil         :refer [add-spec-to-docstring]]\n    [rewrite-clj.parser   :as p]\n    [rewrite-clj.zip      :as z]\n    #_[taoensso.tufte :as tufte :refer (p defnp profiled profile)]))"
    diff: - "(ns ^:no-doc zprint.zprint\n  #?@(:cljs [[:require-macros\n              [zprint.macros :refer [dbg dbg-pr dbg-form dbg-print zfuture]]]])\n  (:require\n    #?@(:clj [[zprint.macros :refer [dbg-pr dbg dbg-form dbg-print zfuture]]])\n    [clojure.string     :as s]\n    [zprint.finish      :refer [newline-vec]]\n    [zprint.zfns        :refer [zstring znumstr zbyte-array? zcomment? zsexpr\n                                zseqnws zseqnws-w-nl zfocus-style zstart zfirst\n                                zfirst-no-comment zsecond znthnext zcount zmap\n                                zanonfn? zfn-obj? zfocus zfind-path zwhitespace?\n                                zlist? zcount-zloc-seq-nc-nws zvector? zmap?\n                                zset? zcoll? zuneval? zmeta? ztag zlast zarray?\n                                zatom? zderef zrecord? zns? zobj-to-vec\n                                zexpandarray znewline? zwhitespaceorcomment?\n                                zmap-all zpromise? zfuture? zdelay? zkeyword?\n                                zconstant? zagent? zreader-macro?\n                                zarray-to-shift-seq zdotdotdot zsymbol? znil?\n                                zreader-cond-w-symbol? zreader-cond-w-coll?\n                                zlift-ns zfind zmap-w-nl zmap-w-nl-comma\n                                ztake-append znextnws-w-nl znextnws\n                                znamespacedmap? zmap-w-bl zseqnws-w-bl zsexpr?]]\n    [zprint.comment     :refer [blanks inlinecomment? length-before]]\n    '[zprint.ansi :refer [color-str]]\n    ~`[zprint.config :refer [validate-options merge-deep]]\n    [zprint.zutil       :refer [add-spec-to-docstring]]\n    [rewrite-clj.parser :as p]\n    [rewrite-clj.zip    :as z]\n    #_[taoensso.tufte :as tufte :refer (p defnp profiled profile)]))"
          + "(ns ^:no-doc zprint.zprint\n  #?@(:cljs [[:require-macros\n              [zprint.macros :refer [dbg dbg-pr dbg-form dbg-print zfuture]]]])\n  (:require\n    #?@(:clj [[zprint.macros :refer [dbg-pr dbg dbg-form dbg-print zfuture]]])\n    [clojure.string       :as s]\n    [zprint.finish        :refer [newline-vec]]\n    [zprint.zfns          :refer\n                            [zstring znumstr zbyte-array? zcomment? zsexpr\n                             zseqnws zseqnws-w-nl zfocus-style zstart zfirst\n                             zfirst-no-comment zsecond znthnext zcount zmap\n                             zanonfn? zfn-obj? zfocus zfind-path zwhitespace?\n                             zlist? zcount-zloc-seq-nc-nws zvector? zmap? zset?\n                             zcoll? zuneval? zmeta? ztag zlast zarray? zatom?\n                             zderef zrecord? zns? zobj-to-vec zexpandarray\n                             znewline? zwhitespaceorcomment? zmap-all zpromise?\n                             zfuture? zdelay? zkeyword? zconstant? zagent?\n                             zreader-macro? zarray-to-shift-seq zdotdotdot\n                             zsymbol? znil? zreader-cond-w-symbol?\n                             zreader-cond-w-coll? zlift-ns zfind zmap-w-nl\n                             zmap-w-nl-comma ztake-append znextnws-w-nl znextnws\n                             znamespacedmap? zmap-w-bl zseqnws-w-bl zsexpr?]]\n    [zprint.comment       :refer [blanks inlinecomment? length-before]]\n    '[zprint.ansi :refer [color-str]]\n    ~`[zprint.config :refer [validate-options merge-deep]]\n    [zprint.zutil         :refer [add-spec-to-docstring]]\n    [rewrite-clj.parser   :as p]\n    [rewrite-clj.zip      :as z]\n    #_[taoensso.tufte :as tufte :refer (p defnp profiled profile)]))"

I dug around in zprint for a while, and my current best guess is that zprint is (at least sometimes) measuring column widths against sexpred nodes.
As you know, a sexpred node does not necessarily match the length of the node in the original source.
With the new PR we are testing zprint against, the width of certain sexpred nodes is a wider, which might have helped to uncover the (potential) bug in zprint.

Before rewrite-clj PR:

user=> (-> "~a" z/of-string z/sexpr)
(unquote a)

After rewrite-clj PR clojure vars are qualified:

user=> (-> "~a" z/of-string z/sexpr)
(clojure.core/unquote a)

What do you think? Could I be right about my assumption?

kkinnear commented

Hello! I saw the PR for you from Ambrose, since I had one too involving unquote. I didn't imagine that yours would impact zprint, though. I didn't look closely at yours, though I thought it just applied to unquote, not other things?

The error you have received comes from, unfortunately, deep in some really complex code trying to justify :require clauses in ns macros.

The short answer is that yes, with some "corrections", zprint will use the size of things that have been sexpred. And this has worked fine up until now. Perhaps this is a bug, but I would say that up to now having things show up when sexpred largely like they will show up from z/print has been a welcome feature. I can go into that in more detail later if necessary, but first let's figure out what is going wrong.

From looking at the expected and actual output, it doesn't look like a clojure.core addition, it looks to me like the thing that got "bigger" is rewrite-clj.parser. The location of the second column is set by the length of require-cli.parser. Coincidentally enough.

Expected output:

(ns ^:no-doc zprint.zprint
  #?@(:cljs [[:require-macros
              [zprint.macros :refer [dbg dbg-pr dbg-form dbg-print zfuture]]]])
  (:require
    #?@(:clj [[zprint.macros :refer [dbg-pr dbg dbg-form dbg-print zfuture]]])
    [clojure.string     :as s]
    [zprint.finish      :refer [newline-vec]]
    [zprint.zfns        :refer [zstring znumstr zbyte-array? zcomment? zsexpr
                                zseqnws zseqnws-w-nl zfocus-style zstart zfirst
                                zfirst-no-comment zsecond znthnext zcount zmap
                                zanonfn? zfn-obj? zfocus zfind-path zwhitespace?
                                zlist? zcount-zloc-seq-nc-nws zvector? zmap?
                                zset? zcoll? zuneval? zmeta? ztag zlast zarray?
                                zatom? zderef zrecord? zns? zobj-to-vec
                                zexpandarray znewline? zwhitespaceorcomment?
                                zmap-all zpromise? zfuture? zdelay? zkeyword?
                                zconstant? zagent? zreader-macro?
                                zarray-to-shift-seq zdotdotdot zsymbol? znil?
                                zreader-cond-w-symbol? zreader-cond-w-coll?
                                zlift-ns zfind zmap-w-nl zmap-w-nl-comma
                                ztake-append znextnws-w-nl znextnws
                                znamespacedmap? zmap-w-bl zseqnws-w-bl zsexpr?]]
    [zprint.comment     :refer [blanks inlinecomment? length-before]]
    '[zprint.ansi :refer [color-str]]
    ~`[zprint.config :refer [validate-options merge-deep]]
    [zprint.zutil       :refer [add-spec-to-docstring]]
    [rewrite-clj.parser :as p]
    [rewrite-clj.zip    :as z]
    #_[taoensso.tufte :as tufte :refer (p defnp profiled profile)]))

Actual:

(ns ^:no-doc zprint.zprint
  #?@(:cljs [[:require-macros
              [zprint.macros :refer [dbg dbg-pr dbg-form dbg-print zfuture]]]])
  (:require
    #?@(:clj [[zprint.macros :refer [dbg-pr dbg dbg-form dbg-print zfuture]]])
    [clojure.string       :as s]
    [zprint.finish        :refer [newline-vec]]
    [zprint.zfns          :refer
                            [zstring znumstr zbyte-array? zcomment? zsexpr
                             zseqnws zseqnws-w-nl zfocus-style zstart zfirst
                             zfirst-no-comment zsecond znthnext zcount zmap
                             zanonfn? zfn-obj? zfocus zfind-path zwhitespace?
                             zlist? zcount-zloc-seq-nc-nws zvector? zmap? zset?
                             zcoll? zuneval? zmeta? ztag zlast zarray? zatom?
                             zderef zrecord? zns? zobj-to-vec zexpandarray
                             znewline? zwhitespaceorcomment? zmap-all zpromise?
                             zfuture? zdelay? zkeyword? zconstant? zagent?
                             zreader-macro? zarray-to-shift-seq zdotdotdot
                             zsymbol? znil? zreader-cond-w-symbol?
                             zreader-cond-w-coll? zlift-ns zfind zmap-w-nl
                             zmap-w-nl-comma ztake-append znextnws-w-nl znextnws
                             znamespacedmap? zmap-w-bl zseqnws-w-bl zsexpr?]]
    [zprint.comment       :refer [blanks inlinecomment? length-before]]
    '[zprint.ansi :refer [color-str]]
    ~`[zprint.config :refer [validate-options merge-deep]]
    [zprint.zutil         :refer [add-spec-to-docstring]]
    [rewrite-clj.parser   :as p]
    [rewrite-clj.zip      :as z]
    #_[taoensso.tufte :as tufte :refer (p defnp profiled profile)]))

Granted, this test does have an unquote in it, but near as I can tell that doesn't really change the output. But the variance based justification is truly complex, so I am probably missing something. There are two lines that don't justify, and so they should be ignored, but it is possible that the one with the unquote is affecting what does justify even though it will not, in the end, participate in justification.

Can you tell me the extent of the change the PR makes? What things that were not previously qualified are qualified now? I'm still trying to figure out why anything getting clojure.core/ added on would make the change you are seeing. Thanks!

lread commented

This rewrite-clj PR would probably never have occurred to me, but the keen eye of @frenchy64 noticed that rewrite-clj was technically incorrect here, and I can't deny that he is right. 🙂

The PR is small and straightforward. To see the changes, have a peek at the diff.

It's a bit hard for me to diagnose what should be happening in zprint, because I don't have a good understanding of what zprint is trying to do here.

But it seems when lines get long we need to wrap, and when we need to wrap, there is vertical alignment going on.

I have no clue how to replicate the failing test scenario from the cmd line, so I've just added the following to the bottom of zprint.guide-test:

(comment
  (defn testing123[ f ]
    (spit (str "out/" f)
          (zprint-str (slurp (str "in/" f))
                      {:parse-string? true,
                       :fn-map {":require" [:none
                                            {:list {:option-fn (partial
                                                                 jrequireguide
                                                                 :require)}}]}})))

  (testing123 "lee1.clj")
  (testing123 "lee2.clj")

  :eoc)

Without the PR, I think I can demonstrate the issue.
But first let's try a wee bit of code as a baseline of maybe what should happen:

in/lee1.clj

(ns lee1
  (:require
    [a.bb              :refer [some long line that will need to wrap because it is indeed very long]]
    [d.e       :as f]))

Produces out/lee1.clj which seems fine:

(ns lee1
  (:require [a.bb :refer [some long line that will need to wrap because it is
                          indeed very long]]
            [d.e  :as f]))

Now let's introduce an unquote in/lee2.clj:

(ns lee2
  (:require
    [a.bb              :refer [some long line that will need to wrap because it is indeed very long]]
    [d.e       :as f]
    ~`[x.y :as z]))

And notice that we have extra indentation going on in out/lee2.clj:

(ns lee2
  (:require [a.bb    :refer [some long line that will need to wrap because it is
                             indeed very long]]
            [d.e     :as f]
            ~`[x.y :as z]))

If we take a look at the column width, it matches the width of unquote:

;;           1234567
;;           unquote
            [d.e     :as f]

Coincidence? Could be, but I kinda doubt it!

kkinnear commented

Thanks for pursuing this into zprint. That's above and beyond!

A few things...

  1. I see only unquote, unquote-splicing and deref changing size due to this PR. Which is good.

  2. zprint absolutely makes column decisions for this particular style based on sizes of things returned by sexpr. But when it actually formats things, it formats them based on the results of z/string, which is what came into the parser.

  3. I think the actual zprint test that is failing is because prior to the PR, the size of the unquote doesn't actually push the second column beyond what rewrite-clj.parser forces. But adding clojure.core/ to the length of unquote makes that line the thing that is determining the location of the second column, thus causing the test to fail. Indeed, clojure.core/unquote is 20 characters, which is the size of the thing which is determining the location of the second column.

  4. For zprint to not use sexpr results to determine column locations, I would have to wander around the ns macro innards using zipper operations instead of clojure operations. Which is certainly possible -- and I do exactly that for formatting, for what that is worth. I have one formatting engine that formats rewrite-clj zippers and clojure structures identically, without knowing which it is operating on. But I haven't used that same approach in determining the column widths, since it was hard enough to start with. That may need to change, I'm still thinking about that. The answer to the next question will inform my thinking.

  5. I put the unquote into the test for jrequireguide for no particular reason that I can recall. I don't expect that anyone actually uses unquote, deref, or unquote-splicing in their ns macros. But I have only the code that I randomly pick out on GitHub or get as issues to show me what people actually do. You have a different view of the Clojure world than I, for sure. Do you think people ever use unquote, deref, or unquote-splicing in their ns macros? If they do, then the current approach using the sexpr sizes is totally broken, and I either have to rewrite the code to only use z/print information or maybe detect use of those vars and elide them.

  6. In any case, this is clearly zprint's problem, and should not impact your ability to push forward with the PR and release rewrite-clj whenever you want. Depending on your response to some of the above, I'll either remove this test or change zprint so that this test has a different expected result. That change will not happen until 1.3.0, which is the next release.

Thank you for running the zprint tests in rewrite-clj! And pursuing this into zprint along the way. I really appreciate it!

It is also nice to hear from you again.

lread commented

I really can't comment on zprint's usage of sexpr.
You already know that it should be used judiciously and with caution.
I'm guessing some accidental usage by zprint might have slipped through the cracks somewhere.

For item 5, I don't know. But the effect is a bit of extra unwanted indentation from zprint, which is not great, but maybe not the end of the world for these perhaps peculiar use cases.

For 6, thanks for the confirmation. I'll go ahead and merge the PR.
We don't have any immediate plans for the next release.

It's always a pleasure to interact with you too @kkinnear.
I'll continue to ping you whenever I see a rewrite-clj change cause an issue for zprint.

lread commented

I think I might go ahead and temporarily disable the failing zprint test over a rewrite-clj to get a green build again.