Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#8022 closed bug (fixed)

Outdated documentation for the -fwarn-lazy-unlifted-bindings warning

Reported by: asr Owned by: thoughtpolice
Priority: highest Milestone: 8.0.1
Component: Documentation Version: 7.6.3
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

The section 4.8 of 'The Glorious Glasgow Haskell Compilation System User's Guide, Version 7.6.3' says:

-fwarn-lazy-unlifted-bindings: ... This will be an error, rather than a warning, in GHC 7.2.

I guess the above sentence should be removed.

Attachments (1)

0001-Make-lazy-unlifted-bindings-an-error-by-default.patch (4.6 KB) - added by thoughtpolice 6 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by simonpj

difficulty: Unknown

Actually -fwarn-lazy-unlifted-bindings is currently (HEAD) still a warning. The comment (TcBinds, line 1351) says

        -- This should be a checkTc, not a warnTc, but as of GHC 6.11
        -- the versions of alex and happy available have non-conforming
        -- templates, so the GHC build fails if it's an error:

Can anyone comment on whether the Haskell Platform versions of alex and happy are now conforming?

Simon

comment:2 Changed 6 years ago by asr

I compiled Alex 3.0.5 and Happy 1.18.10 (the versions in the Haskell Platform 2013.2.0.0) with GHC 7.6.3 using the -fwarn-lazy-unlifted-bindings warning. The compilation didn't generate any warning.

comment:3 Changed 6 years ago by simonpj

Milestone: 7.8.1
Owner: set to thoughtpolice
Priority: normalhighest

OK. -fwarn-lazy-unlifted-bindings is on by default (documentation here), with a message saying

Foo.hs:6:11: Warning:
  Pattern bindings containing unlifted types should use an outermost bang pattern:

I propose:

  • To make -fwarn-lazy-unlifted-bindings into a no-op (that says "I do nothing"), and remove it altogether in 7.10.
  • To remove -fwarn-lazy-unlifted-bindings from the "on by default set"
  • To make lazy unlifted bindings an outright error. So GHC would reject
      f x = let I# y = x in ....blah....
    
    (Currently this elicits the warning, because y has type Int# in a lazy-looking binding.)
  • Fix the documentation to match.

It would be possible (and easy to implement) to have a new language extension that allows such bindings, treating them as strict, which is what happens now. But given the long-standing warning and the easy fix, I'm disinclined to add this extra faff and documentation stuff.

Any objections? Let's do this for 7.8.

Simon

comment:4 Changed 6 years ago by thoughtpolice

There was an old bug #4304 which SimonM closed regarding this. Personally I'd rather see the flag go than keep the warning forever and ever, because the fix is extremely simple.

I have a patch that implements this which I'm validating on my tree now.

comment:5 Changed 6 years ago by thoughtpolice

(I edited this comment to remove unnecessary confusion, resulting from a bug on my end. Sorry.)

OK, unfortunately it fails in the stage2 build. Alex-3.0.5 seems fine and looks like it correctly generates lexers we can deal. But happy-1.18.10 does not:

make -r --no-print-directory -f ghc.mk phase=final all
  HC [stage 1] compiler/stage2/build/Parser.o

templates/GenericTemplate.hs:212:14:
    Pattern bindings containing unlifted types should use an outermost bang pattern:
      (sts1@((HappyCons (st1@(action)) (_))))
        = happyDrop k (HappyCons (st) (sts))
    In an equation for ‛happyMonadReduce’:
        happyMonadReduce k nt fn j tk st sts stk
          = happyThen1
              (fn stk tk)
              (\ r -> happyGoto nt j tk st1 sts1 (r `HappyStk` drop_stk))
          where
              (sts1@((HappyCons (st1@(action)) (_))))
                = happyDrop k (HappyCons (st) (sts))
              drop_stk = happyDropStk k stk

templates/GenericTemplate.hs:219:14:
    Pattern bindings containing unlifted types should use an outermost bang pattern:
      (sts1@((HappyCons (st1@(action)) (_))))
        = happyDrop k (HappyCons (st) (sts))
    In an equation for ‛happyMonad2Reduce’:
        happyMonad2Reduce k nt fn j tk st sts stk
          = happyThen1
              (fn stk tk)
              (\ r -> happyNewToken new_state sts1 (r `HappyStk` drop_stk))
          where
              (sts1@((HappyCons (st1@(action)) (_))))
                = happyDrop k (HappyCons (st) (sts))
              drop_stk = happyDropStk k stk
              (off) = indexShortOffAddr happyGotoOffsets st1
              (off_i) = (off +# nt)
              ....

where GenericTemplate.hs is included in all generated parsers. The error is correct in this case I believe, and the binding should be strictified; the defn is:

#define FAST_INT Happy_GHC_Exts.Int#
...
data Happy_IntList = HappyCons FAST_INT Happy_IntList
...

So we would need to make another point-release of happy that is compatible with GHC 7.8.1.

SimonM, could you please weigh in here? Do you think we should still keep this warning since you closed #4304? I can contribute a patch to happy to fix this (it's relatively trivial,) but unfortunately it seems the ship has sailed on the platform versions of Happy working. OTOH, once you have the platform it is fairly easy to install a new version of Happy.

Last edited 6 years ago by thoughtpolice (previous) (diff)

comment:6 Changed 6 years ago by simonpj

Happy is part of the Haskell platform, so people installing HP will get a new GHC and a new Happy both.

So our HP users would be happy (as it were). But bootstrapping 7.7 might be painful.

I see you propose to leave this for 7.10 (#8192). Maybe that's the right thing to do. (Though I'm curious why you started a new ticket.)

Simon

Last edited 6 years ago by simonpj (previous) (diff)

comment:7 Changed 6 years ago by thoughtpolice

Yes, the patch unfortunately can't go in for now when it will totally break bootstrapping. We'll have to see if we can get a fix into Happy.

FWIW, I created the 7.10 ticket because I like keeping tickets for things that we eventually remove. It normally happens when people glance over the code, but I find having a ticket is an easier trail to follow.

comment:8 Changed 6 years ago by thoughtpolice

I have made a pull request for happy here, and once/if happy-1.18.11 is released with this change, we can move on this ticket:

https://github.com/simonmar/happy/pull/8

comment:9 Changed 6 years ago by simonmar

Hang on a minute. I think we decided *not* to warn about these, which is why I removed the bangs from Happy. I'll need to search through my email and commit logs and find out what happened.

comment:10 Changed 6 years ago by simonmar

What I'm remembering is this:

commit 67157c5c25c8044b54419470b5e8cc677be060c3

Author: simonpj@microsoft.com <unknown>
Date:   Tue Nov 16 17:18:43 2010 +0000

    Warn a bit less often about unlifted bindings.
    
    Warn when
       (a) a pattern bindings binds unlifted values
       (b) it has no top-level bang
       (c) the RHS has a *lifted* type
    
    Clause (c) is new, argued for by Simon M
    
    Eg     x# = 4# + 4#      -- No warning
           (# a,b #) = blah  -- No warning
           I# x = blah       -- Warning

Which should mean that the bindings that Happy uses (where bindings of type Int#) do not generate warnings. So is there a bug in the warning?

comment:11 Changed 6 years ago by simonpj

No, the warning looks right to me. As I understand it, we have

data Happy_IntList = HappyCons Int# Happy_IntList
...
f x = ...blah...
    where
      HappyCons p q = ...blah...

The warning is about the unlifted (p::Int#) in a pattern binding for a lifted type Happy_IntList. It's exactly like the third of your examples above

    I# x = blah  -- Warning

So it looks to me as if GHC is generating a warning as advertised, and Happy is generating code that trips that warning. No?

Simon

comment:12 Changed 6 years ago by simonmar

Aha - I was looking at the Int# bindings in Happy's output, and didn't notice that HappyCons binding.

I'll change Happy, but perhaps we don't want to turn this warning on by default just yet? Give the new Happy a chance to make it into the Haskell Platform, so that we don't force everyone to update Happy just to build GHC. It's an extra annoyance that we don't have to force on people.

comment:13 Changed 6 years ago by simonmar

happy 1.18.11 uploaded

comment:14 Changed 6 years ago by thoughtpolice

Status: newpatch

Thanks Simon!

As for the warning for 7.8: personally, I don't think asking *developers* to upgrade Happy is really that big of a burden, considering there's a small number of us and the fix is rather straightforward (cabal install happy.)

The more annoying aspect might be telling users that they need to upgrade if they want to use 7.8, though.

I'm attaching my patch for this in any case so it isn't lost.

Last edited 6 years ago by thoughtpolice (previous) (diff)

comment:15 Changed 6 years ago by simonpj

Milestone: 7.8.17.10.1

This isn't a big deal. It was an oversight not to have changed Happy a couple of years ago, but hardly a big deal. Let's milestone the change for 7.10 (with high prio so we don't forget again) and meanwhile leave the status quo.

Simon

comment:16 Changed 6 years ago by Austin Seipp <austin@…>

In 7f23a5dfd8ea061a25f13b1ecef799d834732668/ghc:

Make lazy unlifted bindings an error by default.

This was supposed to happen a long time ago, but later is better than
never. This makes `-fwarn-lazy-unlifted-bindings` into a no-op (with its
own warning) to be removed in GHC 7.10.

This fixes #8022.

Signed-off-by: Austin Seipp <austin@well-typed.com>

comment:17 Changed 6 years ago by Austin Seipp <austin@…>

In cdadf54025e1b854fea17ca928566b8a00636c0c/testsuite:

Fix fallout from making lazy unlifted bindings an error

Issue #8022

Signed-off-by: Austin Seipp <austin@well-typed.com>

comment:18 Changed 6 years ago by thoughtpolice

Resolution: fixed
Status: patchclosed

I chatted with Simon about this for a second at ICFP, and I decided to go ahead and merge this; we already require Happy 1.19.0+ now because of Jan's changes, so this doesn't hurt much (it only means people who get 7.8 and don't use the platform will need to update Happy, which is probably fine.

comment:19 Changed 4 years ago by Ben Gamari <ben@…>

In ad30c760/ghc:

Remove documentation for -Wlazy-unlifted-bindings

This flag was supposed to be removed in 7.10. This finally resolves
Trac #8022.

Test Plan: Read it

Reviewers: austin

Reviewed By: austin

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D1922

GHC Trac Issues: #8022

comment:20 Changed 4 years ago by bgamari

Milestone: 7.10.18.0.1
Note: See TracTickets for help on using tickets.