Closed expipiplus1 closed 1 year ago
This issue was already brought up to my attention in #15, and at that time we didn't think there was a good solution for this. I have yet to review your PR (which I can't right now), but right from the beginning, some things are making me tick:
however the implementation was much more clunky so I went with the simpler solution of adding another case to Annotation.
I think this is not a good solution as we lose the semantics of the annotation type: instead of just describing the style of the document, we now have to worry about things which aren't style (and user-defined Doc
fall into this category).
making withUnicode and tabSize their own types to make the printDiagnostic interface slightly less difficult.
I think we could do the same for the colors switch too, but this is a very minor change as the colors are already separated from the rendering logic.
Now here are my thoughts: originally, in #15, we debated whether it would be a good thing to have functions converting message type into Doc
s:
One fix (which is very dirty and completely voids the purpose of the
Pretty
typeclass, which is flawed anyway) would be for diagnose to internally passmsg -> Doc ann
functions to every rendering function. But this is very cumbersome and error-prone. And this also bloats the function interfaces. (https://github.com/Mesabloo/diagnose/issues/15#issuecomment-1287827523)
This should work, and we could have two versions of printDiagnostic
involved:
printDiagnostic
would simply use printDiagnosticNoPretty pretty
(where pretty
comes from the Pretty
typeclass);printDiagnosticNoPretty f
would require f :: msg -> Doc AnsiAnnotation
(or some other type of annotation, in which case we would need to convert to the final type of annotation with another user-passed function). This allows us to use Doc
s inside reports pretty easily. Unfortunately, this solution does not quite get us to a clear semantics division, but this becomes the user's fault (kinda) instead of our fault.This seems to me a better solution to the problem than the one you propose, and also should involve less changes in the codebase (basically, we only have to pass the prettifying function everywhere, why not as an implicit parameter, and it should mostly be all the changes required).
Let's discuss this more thoroughly before merging this PR (I also don't like merging PR knowing that we have a very big change coming soon, see #14; there are still a few more things to change before publishing a new version unfortunately).
Thank you for taking the time to write up your thoughts.
I think this is not a good solution as we lose the semantics of the annotation type: instead of just describing the style of the document, we now have to worry about things which aren't style (and user-defined
Doc
fall into this category).
Specifically what semantics are these? Whatever they are can't they be retained by a clause saying "If the Annotation
is OtherStyle
then its provenance is the user code, and the user should be prepared to deal with whatever they put in"?
I think we could do the same for the colors switch too, but this is a very minor change as the colors are already separated from the rendering logic.
Indeed, however I think that being able to pass in unadordnedStyle
(or whatever other colorless style) is a better solution here as we can remove this boolean for free.
One fix (which is very dirty and completely voids the purpose of the
Pretty
typeclass, which is flawed anyway) would be for diagnose to internally passmsg -> Doc ann
functions to every rendering function. But this is very cumbersome and error-prone. And this also bloats the function interfaces.
It's true that we're only using pretty
as a glorified fromString
. However there's an additional (unenforced) requirement for input msg
s we're relying on: which is that it doesn't insert any Nesting
or Nest
. One way or another, I think we'll have to either
replaceLinesWith
Doc
s altogether (including smuggling them in via a Pretty
instance or msg -> Doc ann
)(The specific issue with these constructions is that if one renders nest 10 foo
then the pipes and other flourishes are also indented, oh no!)
(The absolute correct fix here would be to remove prettyprinter
from the layouting, instead using it to layout individual messages and other ornamentation, and then proceeding to forward the rendered blocks to some compositing engine capable of overlaying blocks of text sensibly, although convenient for rendering the Doc
that one gets back from diagnose
is nonsense, as semantically related parts of text (for example multiple lines in the user message) have been interleaved with the diagnose
's ornamentation)
Unfortunately, this solution does not quite get us to a clear semantics division, but this becomes the user's fault (kinda) instead of our fault.
This should work, and we could have two versions of
printDiagnostic
involved:* `printDiagnostic` would simply use `printDiagnosticNoPretty pretty` (where `pretty` comes from the `Pretty` typeclass); * `printDiagnosticNoPretty f` would require `f :: msg -> Doc AnsiAnnotation` (or some other type of annotation, in which case we would need to convert to the final type of annotation with another user-passed function). This allows us to use `Doc`s inside reports pretty easily.
TBH I don't think such an interface should be exposed if it's not possible to correctly use it, in that case only allow passing in Text
.
This seems to me a better solution to the problem than the one you propose, and also should involve less changes in the codebase (basically, we only have to pass the prettifying function everywhere, why not as an implicit parameter, and it should mostly be all the changes required).
I should probably split this one up into the miscellaneous tidyings, the actual relevant change is not big (just changing the type sigs throughout Internal
and the change to replaceLinesWith
Specifically what semantics are these? Whatever they are can't they be retained by a clause saying "If the
Annotation
isOtherStyle
then its provenance is the user code, and the user should be prepared to deal with whatever they put in"?
Alright, a took a better look at the code (more specifically how OtherStyle
is being used) and this completely voids my complaint. In fact, it is way better than what I imagined/understood initially.
Indeed, however I think that being able to pass in
unadordnedStyle
(or whatever other colorless style) is a better solution here as we can remove this boolean for free.
That's something that I never considered, hence the simpler interface with booleans (which, in the case of colors, was only useful to remove all annotations, which is what a style can do).
It's true that we're only using
pretty
as a glorifiedfromString
. However there's an additional (unenforced) requirement for inputmsg
s we're relying on: which is that it doesn't insert anyNesting
orNest
. One way or another, I think we'll have to either
- Implement the "denesting" logic I've implemented here in
replaceLinesWith
- Disallow passing in
Doc
s altogether (including smuggling them in via aPretty
instance ormsg -> Doc ann
)
Fortunately, we require msg
to be Pretty
in all the rendering spots. This means that (unless somebody is willing to write a very sub-optimal instance Pretty (Doc ann)
) we cannot pass Doc
s currently, hence any msg
cannot contain such constructs (Nesting
and Nest
).
The problem specifically manifests itself when Doc
s are embed within other docs.
(The absolute correct fix here would be to remove
prettyprinter
from the layouting, instead using it to layout individual messages and other ornamentation, and then proceeding to forward the rendered blocks to some compositing engine capable of overlaying blocks of text sensibly, although convenient for rendering theDoc
that one gets back fromdiagnose
is nonsense, as semantically related parts of text (for example multiple lines in the user message) have been interleaved with thediagnose
's ornamentation)
I'm not sure that I entirely follow this, but from what I understand, having a two-layer rendering (rendering each individual block, e.g. the source code, the messages with the "arrows", the side rule, etc.) would be a lot better to create new layouts (which is the point of the issue/PR linked earlier, to allow for user-defined layouts), as one would simply need to write the logic for placing blocks and some default color style, and all blocks would be placed accordingly by the "compositing engine".
Alright, a took a better look at the code (more specifically how
OtherStyle
is being used) and this completely voids my complaint. In fact, it is way better than what I imagined/understood initially.
Great :)
It's true that we're only using
pretty
as a glorifiedfromString
. However there's an additional (unenforced) requirement for inputmsg
s we're relying on: which is that it doesn't insert anyNesting
orNest
. One way or another, I think we'll have to either
- Implement the "denesting" logic I've implemented here in
replaceLinesWith
- Disallow passing in
Doc
s altogether (including smuggling them in via aPretty
instance ormsg -> Doc ann
)Fortunately, we require
msg
to bePretty
in all the rendering spots. This means that (unless somebody is willing to write a very sub-optimal instancePretty (Doc ann)
) we cannot passDoc
s currently, hence anymsg
cannot contain such constructs (Nesting
andNest
). The problem specifically manifests itself whenDoc
s are embed within other docs.
What I mean is that even the Pretty
instance is too much. For example this breaks in the current version of Diagnose
{-# LANGUAGE OverloadedStrings #-}
module A where
import Error.Diagnose
import Prettyprinter
data MyMsg = MyMsg
instance Pretty MyMsg where
pretty MyMsg = nest 2 (vsep ["foo", "bar", "baz"])
main :: IO ()
main =
printDiagnostic stderr True True 2 defaultStyle $
def `addReport` Err Nothing MyMsg [(Position (1, 3) (1, 8) "some_test.txt", This MyMsg)] [Note MyMsg]
If nothing else, then this patch fixes this bug.
Ah yeah, indeed. I never thought about this case.
Instead of continuing on a half broken base, perhaps we should investigate directly on the alternative of the compositing engine? I think this is the way of continuing forward, and should fix all those problems (both newlines and nests and all other problems that were not found yet) because everything is pre-rendered in isolation, and put together afterwards. Not sure how one would create such engine though. If you have any idea, let me know. :)
Honestly, I would rather see this merged as-is, as I think it's a clear improvement over the status-quo.
Having said that, the way I would implement such a compositing engine would be:
width = totalWith - maximum (myOffset : offsetsOfMsgsAboveMe)
SimpleDocStream
using those widthsSimpleDocStream -> [SimpleDocStream]
(Care must be taken to preserve annotations across line breaks!)[SimpleDocStream]
outerline <> replicate ' ' (horizontalMsgPos - length outerline) <> horizontalMsgLine
) (No reason for a little more complication, one could support true overlapping here)Hey, is there anything else that you'd be willing to include in the PR or can it be merged? Overall it looks good to me. I'll try and work on the compositing engine at some point in the future, to see what I can come up with and how easier it makes writing layouts.
I think it should be good to go. I've been using it for a bit without any obvious issues.
Thanks!
I originally implemented it with the user supplying a function
Annotation -> ann
along with theirDoc ann
s. This made it certain that Diagnose wasn't messing with the user's types, however the implementation was much more clunky so I went with the simpler solution of adding another case toAnnotation
.Here are two examples of: colorful user portions of messages and newlines + indentation in one of the messages
Also included:
printDiagnostic
interface slightly less difficult.