Hi Rick, That looks really excellent, and is a huge improvement. Particularly in combination with Leon's proposed change which more precisely identifies the likely failing part of the grammar, this looks like a big win for not much extra effort.
One thing that I think would help a lot would be if it were possible to show the actual text from the failing expression rather than pretty printing a seq representation of it. This would mean modifying the reader such that it either caches the program text from each top-level form as it reads it, or perhaps re-reading the file on an error. This means the relevant code is likely to look more familiar to the user, and also avoids any need to pretty print. Pretty printing is likely to be complicated since it normally works top-down, and uses the type of each form to decide how to lay its sub-parts out. If you're only pretty-printing a fragment from within, say, a large ns form, pprint is unlikely to format it as the user would expect. Cheers, Colin On 26 August 2016 at 12:59, Rick Moynihan <rick.moyni...@gmail.com> wrote: > I think one obvious area that specs error messages could be improved is > with some basic formatting and cosmetic changes. If spec presented errors > not as a wall of text and syntax but with some simple formatting it would > make a big difference to legibility. > > As a starter for 10, why could we not render the messages at a REPL more > like this? (Note this is basically Brian's error - re-rendered): > > user=> (ns such.sequences (require ,,,,)) > > CompilerException clojure.lang.SpecException: > > Call to clojure.core/ns did not conform to fdef [:args] spec > > There was unexpected extra input in: [2] > > with value: (,,, (require [such.vars :as var] > [such.immigration :as immigrate]) > (require midje.checking.checkers.defining > midje.checking.checkers.chatty > midje.checking.checkers.simple > midje.checking.checkers.combining > midje.checking.checkers.collection)) > > Input failed spec predicate: (cat :attr-map (? map?) > :clauses > :clojure.core.specs/ns-clauses) > > When compiling: (such/sequences.clj:1:1) > > user=> > > Some things to point out: > > 1. Provide some extra context by subclassing IllegalArgumentException as > SpecException. This may also help separate SpecException's from other > IllegalArgumentExceptions too, and help tools do something special on a > SpecException. > > 2. Use of new lines to break up and separate text blocks. > > 3. State that it's an fdef spec, and it was the [:args] bit of that spec > that failed. By stating them together we implicitly associate [:args] with > fdef. Note I'm assuming we can also capture that fdef defined this spec. > > 4. It's a bit unclear what "Extra input" means... so clarify that it was > unexpected. Provide the path [2] as before. > > 5. State the failing value and pretty print it. Note that we also elide > the other passing parameters with a ,,,, > > 6. As before state the predicate that identified the failure in a > humanised manner and the location of the failing form. > > I'm not suggesting these are necessarily good ideas, and I appreciate I've > not considered all of the other cases you might need to, but it seems that > something like the above would be a dramatic if entirely cosmetic > improvement. It would be a shame if clojure.core made no attempt to > humanise the display of these messages and left it entirely up to the > community. > > Thoughts? > > R. > > > On 20 August 2016 at 15:03, Alex Miller <a...@puredanger.com> wrote: > >> >> >> On Saturday, August 20, 2016 at 5:17:59 AM UTC-5, Brian Marick wrote: >>> >>> Yesterday, a bug was filed against Suchwow under 1.9alpha11. It turns >>> out to have been a use of `ns …(require…` instead of `(ns …(:require`. Not >>> in Suchwow, but in Midje. Unfortunately, the Suchwow file the bug report >>> pointed at *also* had that typo - apparently I am prone to it - so adding >>> the colon to the require there didn’t make the problem go away. >>> >>> That caused me to lose my temper and make a fool of myself, which is >>> neither here nor there, except that I apologize to @puredanger. >>> >>> I have two suggestions, though: >>> >>> 1. It has long been the case that Clojure allowed `(ns (require…)` even >>> though that’s strictly incorrect. I suggest that, for backwards >>> compatibility, it be allowed going forward. That is, I think it does no >>> harm for a correct `ns` statement to allow symbols as well as keywords. >>> That wrong code in Midje has been there since Clojure 1.2. >>> >> >> We discussed this before releasing the specs and decided to start on the >> strict side. That said, this is still an alpha and there is plenty of time >> to change our minds prior to official release of 1.9 if that ends up being >> a catastrophic decision. >> >> >>> >>> 2. The following is not a good error message: >>> >>> Exception in thread "main" java.lang.IllegalArgumentException: Call to >>> clojure.core/ns did not conform to spec: >>> In: [2] val: ((require [such.vars :as var] [such.immigration :as >>> immigrate]) (require midje.checking.checkers.defining >>> midje.checking.checkers.chatty midje.checking.checkers.simple >>> midje.checking.checkers.combining midje.checking.checkers.collection)) >>> fails at: [:args] predicate: (cat :attr-map (? map?) :clauses >>> :clojure.core.specs/ns-clauses), Extra input >>> >> >> You left out this next important line too since it points you to exactly >> the file and line where the error occurs: >> >> , compiling:(such/sequences.clj:1:1) >> >> spec produces very detailed error messages driven by the specs and the >> value being validated. I admit that in some cases the output from a spec >> error (particularly for complicated syntaxes where there are wide >> alternative fan-outs) is daunting. However, spec error messages are going >> to be increasingly common for all of us to see and understand and I think >> it is worth taking the time to slow down and actually read them. >> >> > Call to clojure.core/ns did not conform to spec: >> ^^^^^^^^^^^^^^ <- macro that was passed invalid values >> > In: [2] >> ^^^ <- the data path in the :args passed to the macro, here, the >> 2th element is the require clause (ns = 0, such.sequences = 1) >> >> > val: ((require [such.vars :as var] ...) >> ^^ <- the remaining part of the value that did not match (it has >> already matched or "consumed" the first two elements successfully) >> >> > fails at: [:args] >> ^^^^^^ <- the path in the ns fdef spec to the failure point >> >> > predicate: (cat :attr-map (? map?) :clauses >> :clojure.core.specs/ns-clauses), >> ^^^etc -> the remaining part of the spec it was >> trying to match >> >> > Extra input >> specs way of saying that it found something (the val above) but it >> wasn't what the spec expected next >> >> >> I'm not trying to pretend this is as easy to digest as an error message >> that would be produced by hand-written validation and error code, but it's >> also notoriously difficult to cover all possible cases (which is why the >> Clojure DSLs have so many error gaps despite having a lot of that code). We >> are looking to decrease the amount of custom error detection and reporting, >> so anything we do has to be something we can do generically. >> >> For the specific case of macroexpanded error reporting, I think there >> *are* more things we can do here (generically) that will improve >> readability. We *know* we are in the context of checking an fdef spec on a >> macro, so some of the ":args" stuff is maybe not necessary. Having the val >> and predicate for a s/cat forwarded to the point of mismatch is both great >> (as it's specific) but also confusing (because parts of both the input and >> the cat spec) are missing which removes important context. I think there >> are ways to indicate that's happening better. Earlier versions of this also >> reported the full args and we ended up removing that because in many cases >> it feels redundant (or is potentially large). Maybe there is some heuristic >> we could follow on when that would help. >> >> >>> - It would be better to say “`require` should be a keyword, not a >>> symbol” than "fails at: [:args] predicate: (cat :attr-map (? map?) :clauses >>> :clojure.core.specs/ns-clauses), Extra input” >>> >>> - Suggest Clojure.spec error messages follow the “inverted pyramid” >>> structure of news reports: https://en.wikipedia. >>> org/wiki/Inverted_pyramid That would mean the text message about the >>> error would come first, the spec second, and the wrong expression last. >>> >>> - It would be better to name the namespace that has the problem. >>> >> >> As mentioned above, the next line that you omitted points to the file and >> line with the problem (I went to some trouble to ensure that was reported >> properly from the point of use). >> >> >>> - The stack trace adds nothing to the error. If anything, it makes it >>> less understandable, as the sheer amount of text is offputting. >>> >> >> I agree, but this is to some degree an orthogonal issue. Tools (including >> the base REPL) choose what to do when they receive an exception from the >> compiler. I would like to think more carefully about what the compiler is >> producing (informationally) and also how tools should be expected to handle >> these errors. Certainly in cases like this, the stack trace is unnecessary >> and that's true of many compiler errors. That is a fixable problem and I am >> interested in making improvements in this area still in 1.9. >> >> >>> My https://github.com/marick/structural-typing does (a small) part of >>> what clojure.spec does. I went to a lot of effort to get it to produce good >>> error messages, and it turned out OK. I doubt it would be applicable to >>> clojure.spec, but I’d be happy to sign over any code that could be of use. >>> >> >> Thanks, I doubt there's much we could use directly. >> >> >>> It’s unfortunate that reporting errors is so much harder than detecting >>> them. >>> >> >> Indeed. Unfortunately errors are read by people and people are hard. :) >> >> -- >> You received this message because you are subscribed to the Google >> Groups "Clojure" group. >> To post to this group, send email to clojure@googlegroups.com >> Note that posts from new members are moderated - please be patient with >> your first post. >> To unsubscribe from this group, send email to >> clojure+unsubscr...@googlegroups.com >> For more options, visit this group at >> http://groups.google.com/group/clojure?hl=en >> --- >> You received this message because you are subscribed to the Google Groups >> "Clojure" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to clojure+unsubscr...@googlegroups.com. >> For more options, visit https://groups.google.com/d/optout. >> > > -- > You received this message because you are subscribed to the Google > Groups "Clojure" group. > To post to this group, send email to clojure@googlegroups.com > Note that posts from new members are moderated - please be patient with > your first post. > To unsubscribe from this group, send email to > clojure+unsubscr...@googlegroups.com > For more options, visit this group at > http://groups.google.com/group/clojure?hl=en > --- > You received this message because you are subscribed to the Google Groups > "Clojure" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to clojure+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "Clojure" group. To post to this group, send email to clojure@googlegroups.com Note that posts from new members are moderated - please be patient with your first post. To unsubscribe from this group, send email to clojure+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/clojure?hl=en --- You received this message because you are subscribed to the Google Groups "Clojure" group. To unsubscribe from this group and stop receiving emails from it, send an email to clojure+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
