tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
958 stars 83 forks source link

Guess that TypeApplications is used without being enabled #452

Closed neongreen closed 3 years ago

neongreen commented 4 years ago

Since we don't have #56, people who have TypeApplications in their default-extensions get their f @Foo turned silently into f@Foo.

We can detect this by spotting EAsPat when rendering expressions and erroring out.

mrkkrp commented 4 years ago

I don't understand how this is going on work. If TypeApplications are not enabled, you can't get @ parsed as a type application. In addition to that EAsPat is for xs@(x:_) sort of thing, right?

neongreen commented 4 years ago

Yes, you don't get it parsed as a type application, you get it parsed as an as-pattern. The AST is not well-typed enough, so it does not prohibit as-patterns in expression context.

neongreen commented 4 years ago

https://gitlab.haskell.org/ghc/ghc/wikis/design/exp-pat-frame

The approach GHC uses is to parse patterns as expressions and rejig later. This turns out to be suboptimal [...]

mrkkrp commented 4 years ago

What if it was a genuine "at pattern"?

neongreen commented 4 years ago

In p_pat we would render it as usually.

In p_hsExpr we would error out, similarly to how GHC does at a certain stage after parsing:

/tmp/ta.hs:1:8: error:
    Pattern syntax in expression context: f@Foo
    Did you mean to enable TypeApplications?
neongreen commented 4 years ago

We already have a comment there:

  -- These four constructs should never appear in correct programs.
  -- See: https://github.com/tweag/ormolu/issues/343
  EWildPat NoExt -> txt "_"
  EAsPat NoExt n p -> do
    p_rdrName n
    txt "@"
    located p p_hsExpr
  EViewPat NoExt p e -> do
    located p p_hsExpr
    space
    txt "->"
    breakpoint
    inci (located e p_hsExpr)
  ELazyPat NoExt p -> do
    txt "~"
    located p p_hsExpr

Btw, I believe we can error out on ELazyPat and EViewPat too. We should not error out on EWildPat because that's what holes get parsed into.

Regarding #343, I believe that it should be fixed now that UnicodeSyntax is disabled by default, so we don't have to handle EViewPat for the case described in #343 to work. We should add it to the testsuite anyway, though.

neongreen commented 4 years ago

Actually, it has just occurred to me – what syntax does TypeApplications steal precisely? We have a comment that says

    TypeApplications, -- steals (@) operator on some cases

but it doesn't seem to steal that operator:

main = main
  where
    (@) = (+)
/tmp/ta.hs:3:6: error: parse error on input ‘@’
  |
3 |     (@) = (+)
  |      ^

It also isn't mentioned in the summary of stolen syntax.

Perhaps we should just enable it by default?

neongreen commented 4 years ago

Oh damn, found it:

{-# LANGUAGE TypeApplications #-}

main = case [1] of
  xs @ (x:_) -> print (x, xs)
  xs@[] -> print xs
/tmp/ta.hs:4:3: error: Parse error in pattern: xs @(x : _)
  |
4 |   xs @ (x:_) -> print (x, xs)
  |   ^^^^^^^^^^
mrkkrp commented 4 years ago

What do you mean by "error out"? Can you describe the whole plan step by step? I still do not understand what you want to implement, sorry. I'd like to have clear description of problem you want to solve and then your proposed solution.

Perhaps we should just enable it by default?

No, I remember this wasn't enabled at first and we found a failing case. @utdemir may remember what exactly.

neongreen commented 4 years ago

I still do not understand what you want to implement, sorry.

image

I'd like to have clear description of problem you want to solve

The problem is that currently, when a user formats f = g @Bar, they get f = g@Bar. If they have TypeApplications enabled in default-extensions, this will actually cause the code to stop compiling.

mrkkrp commented 4 years ago

@neongreen Thanks. Now it's clear. I'd be OK with a PR for this.

georgefst commented 3 years ago

I'd be OK with a PR for this.

Just to be clear, do you mean a PR adding TypeApplications to the default set? It's even in the new GHC2021, which feels like another argument in favour of assuming its use.

mrkkrp commented 3 years ago

Just enabling it by default is will break things because it steals syntax. There should be something more clever, ask @neongreen, because I have forgotten and it's not obvious from skimming the thread.

neongreen commented 3 years ago

I proposed guessing that -XTypeApplications should be enabled by detecting EAsPat in expression context.

I am not going to implement this myself, though.

amesgen commented 3 years ago

Most likely, this issue can be closed as soon as ghc-lib-parser-9.0 is used, namely due to the new whitespace-sensitivity of !, ~, @ and $ which means that the example

main = case [1] of
  xs @ (x:_) -> print (x, xs)
  xs@[] -> print xs

from above will fail to parse even without TypeApplications being enabled:

<interactive>:3:6: error:
    Found a binding for the ‘@’ operator in a pattern position.
    Perhaps you meant an as-pattern, which must not be surrounded by whitespace

With ghc-lib-parser-9.0, I can't think of an example where enabling/disabling TypeApplications would make any difference (please mention it here if you find one!), so we will be able to enable TypeApplications by default in the next release!