Opened 5 years ago

Last modified 8 months ago

#9244 new feature request

Compiler could warn about type variable shadowing, and hint about ScopedTypeVariables

Reported by: stusmith Owned by: kanetw
Priority: normal Milestone: 8.10.1
Component: Compiler Version: 7.6.3
Keywords: TypeErrorMessages Cc: kanetw
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect warning at compile-time Test Case:
Blocked By: Blocking:
Related Tickets: #1316, #3691, #11438, #10581, #11539, #12716 Differential Rev(s):
Wiki Page:

Description (last modified by thomie)

GHC already warns about variable shadowing:

$ cat test.hs
module Test where

timesTwoPlusOne x
    = timesTwo x + 1
  where timesTwo x = x * 2

$ ghc -fwarn-name-shadowing test.hs
...
Warning:
    This binding for `x' shadows the existing binding
      bound at <location>

However the similar warning doesn't happen for type variables.

$ cat T9244.hs
module T9244 where

import Control.Exception

tryMaybe :: IO a -> IO (Maybe a)
tryMaybe action = do
    result <- (try action) :: IO (Either SomeException a)
    return $ case result of
        Left _  -> Nothing
        Right v -> Just v

$ ghc -fwarn-name-shadowing T9244.hs
...
    Couldn't match type `a' with `a1'
      `a' is a rigid type variable bound by
          the type signature for tryMaybe :: IO a -> IO (Maybe a)
          at types.hs:<line>:13
      `a1' is a rigid type variable bound by
           an expression type signature: IO (Either SomeException a1)
           at types.hs:<line>:15
    Expected type: IO a1
      Actual type: IO a
...

Here, I thought that the 'a' in the function's type declaration was the same 'a' in the expression type declaration. However in Haskell 98, they are completely different variables.

Suggestion: if a type variable is renamed by the compiler due to a clash with another type variable, issue a warning that the second shadows the first, and give a hint about using -XScopedTypeVariables and forall.

Alternative suggestion: if an error is displayed, where the error contains a renamed type variable, issue a hint that the second shadows the first, and give a hint about using -XScopedTypeVariables and forall.

Change History (14)

comment:1 Changed 5 years ago by simonpj

I think that's a fine idea. The easiest place to try this might be the renamer. At the moment the type signature for tryMaybe

tryMaybe :: IO a -> IO (Maybe a)

does not bring a into scope in the body. You need -XScopedTypeVariables, and you need an explicit forall:

tryMaybe :: forall a. IO a -> IO (Maybe a)

But it might be reasonable for the renamer to suggest that this is perhaps what you intended, when it comes across that a mentioned in

(try action) :: IO (Either SomeException a)

Simon

comment:2 Changed 5 years ago by thomie

Description: modified (diff)

comment:3 Changed 4 years ago by thomie

Milestone: 8.2.1

Also reported as #1316, #3691, #11438 and maybe others.

@goldfire writes in ticket:11438#comment:1 (I'm moving the discussion here):

An easy way of suggesting ScopedTypeVariables just came to mind: pretend the extension is always on. When looking up a type variable, if the extension is off but the variable would be in scope otherwise, suggest the extension, while returning a lookup failure (because the variable really isn't in scope!). Getting caught on ScopedTypeVariables is a fairly common occurrence in my experience, so I think it's worth putting in a bit of effort to do better here. (Even better would be to look for type variables in a signature that's missing a forall to suggest adding the forall, but that can be a separate task.)

comment:4 Changed 4 years ago by thomie

@goldfire in #10581:

The ScopedTypeVariables extension is one of the few that doesn't recommend itself when it should be used.

I recognize that this may be hard to do, but here is a strawman proposal: Always behave as if ScopedTypeVariables is on at binding sites for type variables (in an annotation with forall). Then, at occurrences of type variables, check if the extension is on. If a type variable is in scope but the extension is off, remember that the user probably wants the extension, but then rename the type variable occurrence away from the in-scope one. If a type error ensues, we've remembered that the lack of ScopedTypeVariables may be to blame, and we recommend turning it on.

comment:5 Changed 4 years ago by thomie

#11539 requests to warn about missing forall when -XScopedTypeVariables is on.

comment:6 Changed 4 years ago by kanetw

Cc: kanetw added

comment:7 Changed 3 years ago by thomie

Another request for this feature: #12716.

comment:8 Changed 3 years ago by bgamari

Milestone: 8.2.18.4.1

It doesn't look like this will happen for 8.2.

comment:9 Changed 2 years ago by simonpj

Keywords: TypeErrorMessages added

comment:10 Changed 20 months ago by kanetw

Owner: set to kanetw

Working on this, but it seems pretty complicated.

comment:11 Changed 20 months ago by simonpj

Before implementing much, it'd be good to write a specification of what you are trying to implement. I think it may be something like this:

  • When compiling without -XScopedTypevariables...
  • ...and type variable a would have been in scope if you were compiling with -XScopedTypeVariables...
  • ...and when you are about to add implicit quantification over a
  • ...then emit a warning

Example

f :: forall a. [a] -> [a]
f = ....(let g :: a -> a
             g = ...
         in blah)....

With -XScopedTypeVariables the tyvar a would have been in scope in the body of f. Because it isn't in scope, we are going to implicitly quantify over a in the type signature for g, treating its signature as g :: forall a. a->a. That implicit quantification is what we want to warn about.

Question: what if we had instead written

f :: [a] -> [a]
f = ....(let g :: a -> a
             g = ...
         in blah)....

Now, even with -XScopedTypeVariables the tyvar a would not be in scope. Do we want to warn then too? I guess so.

Writing all this down, with a series of examples, would be helpful.

I can see that it's a bit tricky to implement. We kind of need an extra set of tyvars that would be in scope if -XScopedTypeVariables was on, but aren't in scope because it isn't. Is it worth the fuss? Perhaps. Anyway, it'd be worth explaining your proposed implementation path before investing effort in implementing it.

comment:12 Changed 19 months ago by bgamari

Milestone: 8.4.18.6.1

This ticket won't be resolved in 8.4; remilestoning for 8.6. Do holler if you are affected by this or would otherwise like to work on it.

comment:13 Changed 14 months ago by bgamari

Milestone: 8.6.18.8.1

These won't be fixed for 8.6, bumping to 8.8.

comment:14 Changed 8 months ago by osa1

Milestone: 8.8.18.10.1

Bumping milestones of low-priority tickets.

Note: See TracTickets for help on using tickets.