Opened 12 years ago

Closed 9 years ago

Last modified 4 years ago

#1595 closed bug (fixed)

duplicate "not in scope" error when giving multiple vars type-signatures at once

Reported by: Isaac Dupree Owned by: michalt
Priority: normal Milestone: 7.2.1
Component: Compiler Version: 6.6.1
Keywords: Cc: michal.terepeta@…, david.waern@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect warning at compile-time Test Case: rename/should_fail/T1595
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

(bug in 6.6.1 and 6.7)

a, b :: Integer -> Tpyo
a = undefined
b = undefined

results in

gw.hs:2:19: Not in scope: type constructor or class `Tpyo'

gw.hs:2:19: Not in scope: type constructor or class `Tpyo'

Expected result: only one of the same error per location in file of mistake, not one per function being defined.

Attachments (4)

0001-Change-TypeSig-to-take-a-list-of-names-fixes-1595.patch (17.2 KB) - added by michalt 9 years ago.
Patch for GHC.
0001-Follow-the-change-of-TypeSig-in-GHC.patch (31.8 KB) - added by michalt 9 years ago.
Patch for Haddock.
0001-Add-a-simple-test-for-ticket-1595.patch (2.4 KB) - added by michalt 9 years ago.
Simple test.
0001-Change-TypeSig-to-take-a-list-of-names-fixes-1595-new.patch (17.7 KB) - added by michalt 9 years ago.
Updated patch.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 12 years ago by simonpj

Milestone: 6.1

Good point. This is a rare case where GHC does not preserve the original source syntax, and really it should. At the moment we have in HsBinds

data Sig name	-- Signatures and pragmas
  = 	-- An ordinary type signature
	-- f :: Num a => a -> a
    TypeSig	(Located name)	-- A bog-std type signature
		(LHsType name)

Really that (Located name) should be a list [Located name]. Making this change consistently should not be hard. Still, it's not very important, so I think I'll milestone it for 6.10, not 6.8.

Simon

comment:2 Changed 11 years ago by simonmar

Architecture: UnknownUnknown/Multiple

comment:3 Changed 11 years ago by simonmar

Operating System: UnknownUnknown/Multiple

comment:4 Changed 11 years ago by igloo

Milestone: 6.10 branch_|_

comment:5 in reply to:  1 Changed 9 years ago by michalt

Cc: michal.terepeta@… added
Type of failure: None/Unknown

Replying to simonpj:

[...] Really that (Located name) should be a list [Located name]. Making this change consistently should not be hard. Still, it's not very important, so I think I'll milestone it for 6.10, not 6.8.

Simon

If this is the right way to solve it then I can try to do it. But if there are a lot assumptions throughout the compiler that Sig refers to only one name then is it still worth it..? (I can imagine that such a change might unnecessarily complicate some things)

comment:6 Changed 9 years ago by simonpj

I've checked... it'd be an easy change with no global impact.

I'm about to commit a minor refactoring that'll make it a bit easier (getting rid of HsBinds.sigName altogether).

So go for it!

Simon

comment:7 Changed 9 years ago by michalt

Owner: set to michalt

Great! I'll try to fix it over the weekend.

comment:8 Changed 9 years ago by michalt

This might take a bit longer --- it seems that haddock uses TypeSig in a few places (like its backends) and I'll probably also wait for your refactoring.

Changed 9 years ago by michalt

Patch for GHC.

Changed 9 years ago by michalt

Patch for Haddock.

comment:9 Changed 9 years ago by michalt

Status: newpatch
Type of failure: None/UnknownIncorrect warning at compile-time

So here's the initial version of the patches. Everything compiles and validates. As for Haddock, in a few places I'm not sure if I'm not breaking anything; I've added small FIXME notes in such places. Anyway, let me know if there's anything that could be improved.

Btw. what is the recommended way of getting the Haddock patch merged? Should it go through upstream? Currently the git repo distributed with GHC contains a few more patches/changes than the darcs one..

comment:10 Changed 9 years ago by simonpj

Cc: david.waern@… added

Thank you Michael! Can you make a testsuite test or two as well? Did you validate?

David, Simon M: I'm happy with the GHC changes but I'm not qualified to review the Haddock patch, but I expect it is fine. The change is that instead of HsBinds.Sig being defined like this:

data Sig name	
  = TypeSig (Located name) (LHsType name)
  | ...other constructors..

it takes a list of names, thus

data Sig name	
  = TypeSig [Located name] (LHsType name)
  | ...other constructors..

thus making HsSyn reflect exactly the Haskell source syntax

f,g :: Int

The changes to Haddock are simply to follow this representation change.

Michael has left a few FIXME comments, all relating to "can I just pick the first name from the list".

Would you like to review? If I don't hear otherwise I'll just commit in a couple of weeks.

Changed 9 years ago by michalt

Simple test.

comment:11 in reply to:  10 Changed 9 years ago by michalt

Replying to simonpj:

Thank you Michael! Can you make a testsuite test or two as well? Did you validate?

Yes I did run validate and everything was ok. :)

As for the test, I've just attached a pretty trivial one. If you have some idea for another one, please let me know. Btw I wasn't really sure in which test directory should I put it and I've picked typecheck. Hope that's ok.

comment:12 in reply to:  10 Changed 9 years ago by waern

Replying to simonpj:

Would you like to review? If I don't hear otherwise I'll just commit in a couple of weeks.

I've reviewed the patch and it seems fine. I will apply it to upstream and add a few patches to answer the FIXME comments.

comment:13 Changed 9 years ago by simonpj

David: thanks. We need to simultaneously commit to GHC and Haddock or builds will fail. Will you validate and commit to GHC as well as Haddock?

Simon

comment:14 in reply to:  13 Changed 9 years ago by waern

Replying to simonpj:

David: thanks. We need to simultaneously commit to GHC and Haddock or builds will fail. Will you validate and commit to GHC as well as Haddock?

Simon

I tried to apply the GHC patch but got stuck on an evil conflict in TcClassDcl.tcClassSigs which I couldn't see how to fix easily.

comment:15 Changed 9 years ago by igloo

Milestone: _|_7.2.1

Changed 9 years ago by michalt

Updated patch.

comment:16 Changed 9 years ago by michalt

I've rebased the patch. The only changes worth attention (compared to the previous patch) are in:

  • TcClassDcl.tcClassSigs
  • Parser.y.pp along with a small change of RdrHsSyn.checkValSig

It would be great if someone could have a look at those changes (Simon?). I validated (but without haddock, i.e. HADDOCK_DOCS = NO). Thanks!

comment:17 Changed 9 years ago by simonpj

Now I'm confused. I think David Waern has a revised Haddock patch, and possibly a revised GHC one too. And Michael has a revised GHC patch, attached.

David: you have commit rights to both Haddock and GHC. Can you merge and commit? Thanks.

Simon

comment:18 in reply to:  17 Changed 9 years ago by michalt

Replying to simonpj:

Now I'm confused. I think David Waern has a revised Haddock patch, and possibly a revised GHC one too. And Michael has a revised GHC patch, attached.

David: you have commit rights to both Haddock and GHC. Can you merge and commit? Thanks.

Simon

The only changes in my "revised" patch are the ones necessary to make it apply against current master (i.e. resolve the conflicts that David mentioned in comment 14). Sorry for confusion.

comment:19 Changed 9 years ago by waern

Sorry, I've been a bit busy lately and forgot to update the ticket. I have already committed rebased patches to both Haddock and GHC. I did the same changes as Michael and in addition I made similar changes to GenericSig, so that it also takes a list of names.

comment:20 Changed 9 years ago by michalt

Resolution: fixed
Status: patchclosed

Thanks!

comment:21 Changed 9 years ago by simonpj

Test Case: rename/should_fail/T1595
Note: See TracTickets for help on using tickets.