Surprising error message with bang pattern
ClosedPublic

Authored by v0d1ch on Aug 3 2018, 8:40 AM.

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
v0d1ch updated this revision to Diff 18025.Sep 18 2018, 6:54 AM
  • Move check to infixexp_top

¯\_(ツ)_/¯ maybe I should move it even further into the parser and find a place where we still have whitespace knowledge

sgraf added a comment.Sep 18 2018, 7:30 AM

There isn't really a place with more whitespace knowledge in the parser than where you added the check. Whitespace is discarded by the Lexer (well, mostly, except indentation stuff), the only way to recover it is by comparing where tokens begin and end. So this is basically the way to go, but I think the same check could also take place in the renamer. Whatever, both is fine, I guess.

compiler/parser/Parser.y
2486–2493

I believe you want advanceSrcLoc (srcSpanStart (getLoc $2)) '!' == srcSpanStart (getLoc $3) here. Note that $2 binds the qop (which could be the bang) and $3 (which corresponds to x in the test case) binds exp10_top.

Note that advanceSrcLoc <...> '!' doesn't test whether $2 was a bang, so this would still catch all single-character operators like f + x = .... It should probably also test that $2 is indeed a bang.

And if I'm not mistaken, this should really catch whitespace: If $3 doesn't immediately come after $2, then there must be some kind of separating whitespace. Otherwise there would be some other token in-between, hence the test.

Also, I don't think we even need advanceSrcLoc (srcSpanStart (getLoc $2)), becuase it's basically equivalent to stcSpanEnd (getLoc $2), which makes the test even simpler and more understandable. If that's not enough, we could give it a descriptive name, like noSeparatingWhitespace.

2487

Ultimately, we should probably think of a better message than what this generates (e.g. "Did you forget to activate -XBangPatterns? If not, add a space before the bang").

testsuite/tests/printer/T13600.hs
4 ↗(On Diff #18025)

This should also have the case where we don't want to emit a warning, e.g.

f4 :: [a] -> Int -> a
f4 ! x = f4 Prelude.! x

And also some tests for local let bindings, e.g.

a :: IO Int
a =
   let f4 !x = return (sum x)
   in f4 [1,2,3]

a :: Int
a =
  let f4 ! x = f4 Prelude.! x
  in [1,2,3] ! 1
v0d1ch updated this revision to Diff 18027.Sep 18 2018, 9:27 AM
  • Check for space and move tests
v0d1ch added inline comments.Sep 18 2018, 9:38 AM
compiler/parser/Parser.y
2486–2493

@sgraf This now checks if $3 is immediately preceded by $2 and there is no space in between. But if I put space between ! and x I still get an error if BangPatterns are not turned on. I tried that in the repl with 8.4 compiler also just to double check. This comment suggests that it should work with a space https://ghc.haskell.org/trac/ghc/ticket/13600#comment:2

sgraf added inline comments.Sep 18 2018, 9:46 AM
compiler/parser/Parser.y
2486–2493

Oh, that's weird. Hmm... Let me check out the code and see for myself. Warning for f ! x is not an option, of course.

v0d1ch added inline comments.Sep 18 2018, 9:48 AM
compiler/parser/Parser.y
2486–2493

I think I can cover that case too and suggest a BangPatterns since we mean to define f4 not an infix (!)

sgraf added inline comments.Sep 18 2018, 10:15 AM
compiler/parser/Parser.y
2486–2493

Ahh, OK, now I understand. No, we definitely shouldn't warn every time someone wants to define f ! x without -XBangPatterns. That was the whole point about being sensitive to whitespace, after all. Also, I'd never write bang patterns with a space between the bang and the binder that gets ... annotated.

So, the current behavior looks perfect to me! :)

testsuite/tests/parser/should_fail/T13600.hs
4 ↗(On Diff #18027)

In addition to the variants I already mentioned, Could you also add a test that validates if it works with e.g. f !(Just _)? It's kind of bogus to mean to use BangPatterns here, but I think the warning should be emitted nonetheless.

v0d1ch updated this revision to Diff 18029.Sep 18 2018, 10:29 AM
  • Add one more test case

@sgraf I added another test case just in case :) If you think this is good enough I can ping @bgamari to review again.

v0d1ch updated this revision to Diff 18030.Sep 18 2018, 10:32 AM
  • Remove passing test record
v0d1ch retitled this revision from WIP: Surprising error message with bang pattern to Surprising error message with bang pattern.Sep 18 2018, 10:34 AM

Some remaining points:

  • Can you maybe add a testcase for each of the 5 variants I mentioned? e.g. f !x, f ! x, let f ! x, let f !x, f !(Just _). You can also put them all into one file, no need to split them up.
  • These should be warnings, not errors (because you reuse hintBangPat). These are all syntactically valid haskell programs, after all! You should probably write them in a way that they compile without -XBangPatterns, but generate warnings nonetheless if there was no space.
  • The message is misleading (because you reuse hintBangPat). There are no illegal bang patterns involved here, it's just a hint for the programmer who probably intended other semantics.

So, emit a warning with a proper message instead. We still want to accept a program like

-- | 
-- >>> [1,2,3] ! 1 == 2
f4 :: [a] -> Int -> a
f4 ! x = f4 !! x
v0d1ch updated this revision to Diff 18045.Sep 18 2018, 4:08 PM
  • Add new warning message

@sgraf I just realized we never check if we actually deal with ! just if infix expression is followed immediately by var. I think it makes sense to add that and also warn about defining infix expression without proper spacing. What is your take ?
Also in your example I don't understand how would this compile without errors:

f4 :: [a] -> Int -> a
f4 ! x = f4 !! x

Aren't you writing a type signature for f4 and then defining infix operator ! so f4 lacks its implementation ?

sgraf added a comment.Sep 19 2018, 2:05 AM

@sgraf I just realized we never check if we actually deal with ! just if infix expression is followed immediately by var.

That's what I meant when I wrote

Note that advanceSrcLoc <...> '!' doesn't test whether $2 was a bang, so this would still catch all single-character operators like f + x = .... It should probably also test that $2 is indeed a bang.

I think it makes sense to add that and also warn about defining infix expression without proper spacing. What is your take ?

What should we warn about? There is no extension we could activate for e.g. f +x. I mean, it's weird, but I don't think warnings should concern source code formatting.

Also in your example I don't understand how would this compile without errors:

f4 :: [a] -> Int -> a
f4 ! x = f4 !! x

Aren't you writing a type signature for f4 and then defining infix operator ! so f4 lacks its implementation ?

Right, scrap that type signature, or replace it by (!) :: [a] -> Int -> a. The point still stands: we don't want to emit a warning here.

v0d1ch updated this revision to Diff 18057.Sep 19 2018, 9:13 AM
  • Add another warning message
sgraf added a comment.Sep 20 2018, 2:47 AM

A few more notes. We're getting there!

compiler/parser/Parser.y
2486–2493

I think this is just for debugging purposes, but I want to make a mental note that this should be gone when we commit.

2489

I suggest that instead of testing for equality, you define a helper function that pattern matches on $2 to see if it is a (L _ (HsVar _ (L _ op))) where op == bang_RDR. Kind-of like splitBang does it in RdrHsSyn.

testsuite/tests/parser/should_fail/T13600.hs
10 ↗(On Diff #18057)

This doesn't compile without -XBangPatterns. To be clear: We want a file that compiles without BangPatterns activated, but emits warnings only for definitions where the bang is put immediately before the second identifier, without any space.

I'd suggest the following:

-- | Should not warn
module T13600a where

f ! x = f !! x

x = [1,2,3] ! 1
  where
    f ! x = f !! x
-- | Should emit 4 warnings
module T13600b where

f !(Just x) = f !! x
f !y = head f

x = [1,2,3] ! Just 1
  where
    f !(Just x) = f !! x
    f !y = head f
testsuite/tests/parser/should_fail/T13600.stderr
4 ↗(On Diff #18057)

This looks... weird. Have you tried to use $$ instead of <+> in your warning, like the hintBangPat error does? This puts the pretty printed expression on a new line instead of seperating the text and the message by a space.

I know I'm entirely responsible for the current warning message, but the warning could be a little less imperative, like

Did you forget to activate BangPatterns? Add a space before the bang to deactivate this warning
v0d1ch updated this revision to Diff 18061.Sep 20 2018, 3:15 AM
  • Add a function to match a bang
v0d1ch added inline comments.Sep 20 2018, 3:19 AM
compiler/parser/Parser.y
2486–2493

I introduced e here so I can use it in warnSpaceAfterBang as the second param.

testsuite/tests/parser/should_fail/T13600.hs
10 ↗(On Diff #18057)

Macro thumbsup:

v0d1ch updated this revision to Diff 18062.Sep 20 2018, 3:29 AM
  • Separate test cases
sgraf added inline comments.Sep 20 2018, 6:11 AM
compiler/parser/RdrHsSyn.hs
1793 ↗(On Diff #18062)

This still throws an error instead of emitting a warning. Have a look at https://hackage.haskell.org/package/ghc-8.4.3/docs/Lexer.html#v:addWarning. You'll have to add the new warning to WarningFlag at some point anyway. Maybe you won't even need to synthesize e.

I also came to realize that this chunk of code probably better fits within https://github.com/ghc/ghc/blob/45befe27495b1a7bca037b6a3eedf2474a0204c8/compiler/parser/Parser.y#L3692.

1802 ↗(On Diff #18062)

if b then True else False === b

testsuite/tests/parser/should_fail/all.T
112 ↗(On Diff #18062)

This should not fail.

v0d1ch updated this revision to Diff 18066.Sep 20 2018, 6:53 AM
  • Move checkIfBang function
v0d1ch added inline comments.Sep 20 2018, 6:57 AM
testsuite/tests/parser/should_fail/all.T
112 ↗(On Diff #18062)

How come ? It has ! glued to the function param. f !(Just x) =

sgraf added inline comments.Sep 20 2018, 8:50 AM
testsuite/tests/parser/should_fail/all.T
112 ↗(On Diff #18062)

It still parses as an operator. Try compiling the file with a released GHC, it's valid Haskell syntax.

v0d1ch updated this revision to Diff 18072.Sep 20 2018, 9:26 AM
  • Add a warning
v0d1ch updated this revision to Diff 18080.Sep 20 2018, 4:56 PM
  • Add one more user doc line
sgraf added a comment.Sep 21 2018, 3:45 AM

Looks nearly finished to me. Only a few final issues to resolve.

compiler/main/DynFlags.hs
3870

Maybe name it "missing-space-after-bang" (dito the flag), because that's when it emits a warning

compiler/parser/RdrHsSyn.hs
1793 ↗(On Diff #18062)

Could you also move the warning to Parser.y?

1795 ↗(On Diff #18080)

A little nit: I think we should remove the space before ?. Also I just went through a number of warning messages mentioning extensions and they all drop the -X prefix. Also they seem to prefer "enable" over "activate".

So: "Did you forget to enable BangPatterns?"

docs/users_guide/using-warnings.rst
1262

Link to bang patterns by

:ghc-flag:`-XBangPatterns`

Also talking about spacing is a little vague. How about

warn for missing space before the second argument of an infix definition of ``(!)`` when :ghc-flag:`-XBangPatterns` are not enabled
1263

I presume this links to a hypthotetical ghc extension SpaceAfterBang, which doesn't exist.

testsuite/tests/parser/should_compile/all.T
121

Please add an empty T13600a.stderr file, so that the testrunner asserts that there were no warnings and a T13600b.stderr file that contains the expected warning messages.

v0d1ch updated this revision to Diff 18087.Sep 21 2018, 10:58 AM
  • PR changes
v0d1ch updated this revision to Diff 18090.Sep 21 2018, 4:09 PM
  • Add stderr
sgraf added inline comments.Sep 22 2018, 4:34 AM
docs/users_guide/using-warnings.rst
1262

The description is still too vague for my tastes. See above.

testsuite/tests/parser/should_compile/all.T
121

This should still be normal, not expect_broken. Also, I think the new warning should be on by default (e.g. included in -W, isn't that already the case?), so that you don't need -Wall. Otherwise, T13600a is pretty useless as a regression test, as that warning could never be triggered in the first place. I'd also suggest to turn off any other warnings (like -Wno-missing-signatures) that still are emitted after you remove -Wall.

v0d1ch updated this revision to Diff 18095.Sep 22 2018, 4:48 PM
  • review changes
v0d1ch added inline comments.Sep 22 2018, 4:52 PM
docs/users_guide/using-warnings.rst
1262

Yeah, sorry, don't know where is my head these days. Updated now.

sgraf added a comment.Sep 22 2018, 5:46 PM

Nice! See below. Just make the warning enabled by default and this thing is good to go.

compiler/main/DynFlags.hs
4587

I think I misunderstood -W. This warning should be on by default, so please move this to standardWarnings.

testsuite/tests/parser/should_compile/all.T
121

If you make the new warning on by default, you can hopefully drop all the flags here and still have the same stderr. At least that's how I would expect the warning to behave.

v0d1ch updated this revision to Diff 18100.Sep 23 2018, 5:09 AM
  • Add warning to standard warnings

It should be fine now. Thank you for all your help @sgraf !

sgraf added a comment.Sep 23 2018, 7:13 AM

No problem. Thanks for your patience!

@bgamari You can have a look at this now, I think.

Great work, @v0d1ch! I've left a small suggestion to improve the error message inline but otherwise this looks quite reasonable.

compiler/basicTypes/SrcLoc.hs
21 ↗(On Diff #18100)

Technically haven't a clue is also correct, it just tends to be used more in British English. I think it would be better if we dropped this change to keep the patch on-topic.

compiler/parser/Parser.y
3726

It might be a good idea to make the motivation for this warning more explicit to the user. For instance, perhaps the second sentence should rather read

If you mean to bind (!) then perhaps you want to add a space before the bang for clarity.

v0d1ch updated this revision to Diff 18123.Sep 25 2018, 4:03 PM
  • PR review changes
v0d1ch added a comment.EditedSep 25 2018, 4:06 PM

@bgamari I think one important bit that we somehow managed to loose along the way is that the wording should be:

If you mean to bind (!) then perhaps you want to add a space AFTER the bang for clarity.

Other than that this looks nice :)

bgamari accepted this revision.Oct 15 2018, 12:47 PM

Thanks @v0d1ch1

This revision is now accepted and ready to land.Oct 15 2018, 12:47 PM
This revision was automatically updated to reflect the committed changes.