ghc generates more user-friendly error messages
ClosedPublic

Authored by mikeizbicki on Sep 8 2014, 12:59 AM.

Details

Test Plan

Compiled ghc fine. Opened ghci and fed it invalid code. It gave the improved error messages in response.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mikeizbicki updated this revision to Diff 513.Sep 8 2014, 12:59 AM
mikeizbicki retitled this revision from to ghc generates more user-friendly error messages.
mikeizbicki updated this object.
mikeizbicki edited the test plan for this revision. (Show Details)

There's some obvious extra junk in the diff like the deriving Show bits. I didn't think I was going to be uploading anything when messing around with arc, and now I don't know how to edit the diff :)

Your diff includes a bunch of unrelated whitespace-only changes. Could you remove these? Also there are some other changes that appear to be unnecessary.

The new error messages look nice, though I'm not enough of an expert on GHC's parser to know whether the implementation is correct.

compiler/basicTypes/SrcLoc.lhs
490 ↗(On Diff #513)

You don't seem to use this (probably it was added for debugging?)

compiler/parser/Lexer.x
1687–1693

... or these

2141

I don't think you use this new argument either?

2143–2144

This change doesn't change anything

compiler/parser/Parser.y.pp
2393–2396 ↗(On Diff #513)

Delete this if it isn't used

Oh, I didn't see your comment. You can amend your commit (or just add more commits on top--arc will squash them for review) and rerun arc diff, and it should locate the corresponding Differential and update it automatically.

mikeizbicki updated this revision to Diff 519.Sep 10 2014, 2:59 AM
mikeizbicki edited edge metadata.

removed junk whitespace

mikeizbicki updated this revision to Diff 520.Sep 10 2014, 3:08 AM

removed trailing whitespaces

  1. There's still some spurious whitespace changes. But I think all these are from lint wanting me to remove the extra whitespaces. Is that okay, or should I remove them? Also, why is lint configured to have all these whitespace and line too wide warnings if so much of the codebase doesn't follow the rules?
  2. The commits don't pass the build test because the tests assume the old error messages and the new errors don't match. Is there any easy way to update these tests without manually copying and pasting a bunch of stuff?
  3. As for the correctness, the part I'm least sure about is the let statements. It seems to work for lets in the non-monadic case and doesn't interfere with the monadic lets. There might be some edge case I'm not considering though, and the applicative do might be different somehow.
  1. You should just be able to fix up whitespace in the area you are touching. You shouldn't need to fix up the whole file. That makes it hard to find the actual functional changes. We want to eventually remove all tabs and trailing whitespace, so the lint rules are there to avoid their introduction in new code. But we don't want to fix up the whitespace all at once because that will cause a lot of extra work for people who have large changesets on a branch. (Though we have considered doing so anyways.)
  2. Yes, see https://ghc.haskell.org/trac/ghc/wiki/Building/RunningTests/Updating.
compiler/parser/Lexer.x
2143–2144

This syntactic change does nothing, should be removed

2157–2158

s is unused here, remove it (normally validate would fail with an unused variable warning, but we disable warnings in this file as it gets incorporated into generated code)

Is there an easy way to tell arc to ignore the changes on certain lines in a file? I'm trying to figure out how to undo all the whitespace changes without opening up an editor and manually retyping everything back in.

Basically fine, thank you for taking time to help

I'll take the parser stuff on trust! I've added a note about TcErrors.

Simon

compiler/typecheck/TcErrors.lhs
1095

Better English "maybe you have not applied a function to enough arguments)"

1101

I believe (but it's hard to tell) that this is the only significant change in this file. Can you add a Note to explain the intent, give an example, and link to Trac Trac #9613?

Is there an easy way to tell arc to ignore the changes on certain lines in a file? I'm trying to figure out how to undo all the whitespace changes without opening up an editor and manually retyping everything back in.

If I were you I would fix your commit in git as follows:

  • git reset HEAD^ to "uncommit" the commit, but leave the changes in the working copy
  • git add -p and add only the changes you want (the non-whitespace changes)
  • git diff --staged to make sure you didn't stage any whitespace changes
  • git diff to make sure you didn't fail to commit any real changes (I suppose git diff -b should produce no output, then)
  • git commit -C 73944a768449 to create a new commit with the same commit message as your original commit 73944a768449
  • git checkout -p to throw away the whitespace changes in your working copy
  • arc diff to update this Differential
mikeizbicki updated this revision to Diff 840.Oct 10 2014, 4:41 PM

removed whitespace noise

mikeizbicki updated this revision to Diff 842.Oct 10 2014, 4:50 PM

missed one line

ezyang removed a subscriber: ezyang.Oct 27 2014, 2:05 PM
austin accepted this revision.Nov 19 2014, 6:31 PM
austin edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Nov 19 2014, 6:31 PM
This revision was automatically updated to reflect the committed changes.