SrcLoc show instance
mitchellwrosen opened this issue · 11 comments
Why is the Show instance of SrcLoc just const "noLoc", rather than deriving Show or some other formatted instance?
Because then one's ASTs are extremely cluttered when shown.
In that case, shouldn't it be "", not "noLoc"?
Show should produce a string representation consisting of valid Haskell code that, when run, yields a value equal to the shown value. If the Show instance for SrcLoc produced the empty string, then 1) the derived Show instance for any data type containing a field of type SrcLoc would have extra space (why?), and 2) the result of show would not evaluate to a value of type SrcLoc. We could solve 2) by making the Read instance for SrcLoc consume no input, but that doesn't fix 1.
This has been the subject of much debate in the past, just not on Github. In fact, your suggested behavior used to be the chosen behavior, but that also made it impossible to ever read a non-NoLoc SrcLoc.
Makes sense, but it still seems wrong to print "noLoc". It's wordy, and inaccurate. I spent some time debugging my parser because I couldn't figure out why every location was being set to NoLoc (they weren't). Personally, I don't think it's bad that AST output includes the full Show instance of the underlying Loc. It's mostly used for debugging, anyway; making visually appealing string output is what pretty-printers are for.
Show is not for pretty-printing :) I've looked at a lot of Shown ASTs, and having them annotated with source code locations makes them next to useless. Try it yourself! Keep in mind that noLoc is different from NoLoc.
What do you think the Show instance should be?
That was exactly my point - Show is not for pretty-printing; it's for quickly dumping all of the data to the screen for debugging and what have you. If you really cannot make any sense of an AST with all the SrcLoc clutter, then you should just fmap const () over the AST before showing it (assuming your nodes are parameterized by SrcLoc).
So, I think the Show instance should just be derived.
There are many cases where one wants an expression data type that is a Functor but not over SrcLoc, e.g., when using fixpoint.
If you want a location type that doesn't care about equality but that has a Show instance that displays location information, define another newtype.
Fair enough, but then I would suggest that this new newtype be part of this library, so both users who do and do not want to see location information in their derived Show instances have a type to pick from.
Why don't you just fmap locOf over you AST data type before Showing it? ;)
Heh, ok, I can see that we aren't getting anywhere on this issue. I believe the current behavior is incorrect, as the displayed string "noLoc" might as well be "foobar", except that at least "foobar" wouldn't have suggested there was a bug in my parser.
So I will make one more suggestion: please document in the haddocks that the Show instance of SrcLoc is const "noLoc" for readability purposes.
show should produce legal Haskell code, which is why noLoc is preferable to foobar.
I'd be happy to accept a doc patch.