Details
- Reviewers
bgamari alanz - Commits
- rGHC0b0cb484eb0b: Surprising error message with bang pattern
- Trac Issues
- #13600
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.
¯\_(ツ)_/¯ maybe I should move it even further into the parser and find a place where we still have whitespace knowledge
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 |
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 |
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. |
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 (!) |
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. |
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
@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 ?
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 !! xAren'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.
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 |
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. |
testsuite/tests/parser/should_fail/all.T | ||
---|---|---|
112 ↗ | (On Diff #18062) | How come ? It has ! glued to the function param. f !(Just x) = |
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. |
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. |
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. |
docs/users_guide/using-warnings.rst | ||
---|---|---|
1262 | Yeah, sorry, don't know where is my head these days. Updated now. |
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. |
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
|
@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 :)