Compiler panic on invalid syntax (unterminated pragma)
ClosedPublic

Authored by RolandSenn on Aug 23 2018, 9:21 AM.

Details

Summary

After a parse error in OPTIONS_GHC issue an error message instead of a compiler panic.

Test Plan

make test TEST=T15053

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.
RolandSenn created this revision.Aug 23 2018, 9:21 AM
RolandSenn edited the summary of this revision. (Show Details)Aug 23 2018, 9:22 AM
Phyx added a subscriber: osa1.Aug 23 2018, 1:41 PM

Thanks for the patch @RolandSenn , the code itself is fine, just not sure about the error message. I also can't remember if we emit error messages in GHC in past or present tense.. so CC @osa1

compiler/main/HeaderInfo.hs
346

Should probably be white-space separated. @osa1 Did you have any opinions on the error message?

Agreed: white-space separated is much better! My version of the error message is just a first proposal.

RolandSenn updated this revision to Diff 17803.Aug 25 2018, 9:02 AM
RolandSenn marked an inline comment as done.Aug 25 2018, 9:05 AM
Phyx added a comment.Aug 26 2018, 4:30 PM

ok, how about something like Error while parsing OPTIONS_GHC pragma, expected whitespace delimited list of options. Input was " -O1 }\n\"\n "

tdammers added inline comments.
testsuite/tests/parser/should_fail/T15053.stderr
5

"non-parsable" or "nonparsable", I think. "non" is not an adjective / adverb.

RolandSenn updated this revision to Diff 17855.Aug 29 2018, 3:01 AM

@Phyx Thanks for your nice text. I changed to your version. I added a colon (:) after Input was. PLease inform me, if you dont like it.
@tdammers Thanks for your comment. In the new error message text, there is no more something like non-parasable.

monoidal accepted this revision.Aug 29 2018, 5:33 PM
This revision is now accepted and ready to land.Aug 29 2018, 5:33 PM
Phyx accepted this revision.Aug 29 2018, 6:19 PM

Thanks @RolandSenn ! looks good to me.

osa1 added a comment.Aug 30 2018, 1:39 AM

Thanks @RolandSenn! Added a minor comment about the error message.

compiler/main/HeaderInfo.hs
346

Sorry saw this too late. I think this is fine but to be more consistent with existing error messages can we do something similar to languagePragParseError? E.g.

Cannot parse OPTIONS_GHC pragma
Expecting whitespace-separated list of GHC options
  E.g. {-# OPTIONS_GHC -Wall -O2 #-}
Input was: ...

@osa1 I started with modifying the languagePragParseError message. But as an non native English speaker I obviously was not very successful... I'll wait 3 or 4 days for additional comments and then decide.
@Phyx Can you live with the message text proposed by Ömer?

osa1 added a comment.Aug 30 2018, 2:14 AM

Ah so you wanted to update languagePragParseError too? Why? To be clear, your wording is fine (but note that I'm also not a native speaker ;-), my concern is about consistency and not the choice of words of tense.

@osa1

Ah so you wanted to update languagePragParseError too? Why?

No, no, no! I didn't want to change the language pragma message text! To write the options pragma message, I copied the message text from languagePragParseError to optionsParseError and modified the copied text until I had what was in my first commit.

RolandSenn updated this revision to Diff 17878.Sep 1 2018, 2:39 PM
  • Error msg more consistent with LANGUAGE pragma error (Trac #15053)
RolandSenn marked an inline comment as done.

I now changed to the error message, that's more consistent with the error message issued on a LANGUAGE pragma error.

RolandSenn requested review of this revision.Sep 1 2018, 2:47 PM
monoidal accepted this revision.Sep 4 2018, 4:19 AM
This revision is now accepted and ready to land.Sep 4 2018, 4:19 AM
osa1 accepted this revision.Sep 4 2018, 6:52 AM

Thanks!

This revision was automatically updated to reflect the committed changes.