containers icon indicating copy to clipboard operation
containers copied to clipboard

Prepare for liftA2 being exported from Prelude

Open googleson78 opened this issue 3 years ago • 18 comments

CLC has approved the proposal to export liftA2 from Prelude (https://github.com/haskell/core-libraries-committee/issues/50).

This is a PR to future proof for that change, while minimising warnings.

googleson78 avatar Jun 16 '22 14:06 googleson78

@googleson78 could you please fix CI?

Bodigrim avatar Jun 17 '22 17:06 Bodigrim

I tried to take into account ghc 8.0.2 this time around.

googleson78 avatar Jun 18 '22 14:06 googleson78

@treeowl @sjakobi could you please take a look? We need this merged before proceeding with https://gitlab.haskell.org/ghc/ghc/-/merge_requests/8449

Bodigrim avatar Jun 18 '22 14:06 Bodigrim

@treeowl could this please be merged?

Bodigrim avatar Jun 27 '22 18:06 Bodigrim

@treeowl just a gentle reminder. Otherwise it blocks the respective GHC MR.

Bodigrim avatar Jul 04 '22 18:07 Bodigrim

Import-styles hiding and (..) should be avoided where possible. Maybe MIN_VERSION_base can help.

I would prefer hiding to additional CPP any day, especially for a boot library, because MIN_VERSION_base is sometimes wrong in nightly GHC builds. Let's wait for @treeowl opinion.

Bodigrim avatar Jul 17 '22 12:07 Bodigrim

It is somehow strange that changes are necessary at all here. The PR description isn't very explicit about what problems it addresses. Let me guess: GHC is complaining whenever liftA2 is explicitly imported from somewhere other than Prelude, because it is already implicitly imported from Prelude. That's silly by GHC. That warning logic is simply odd. All the energy developers are spending into fixing GHC's silly unused import warnings should maybe go into fixing GHC. See https://www.snoyman.com/blog/2020/11/haskell-bad-parts-2/ :

... we all turn on -Wall. Because we're good kids. We want to do the right thing. And this, of course, turns on unused import warnings. And because each new release of GHC and every library on Hackage likes to mess with us, we are constantly exporting new identifiers from different modules.

The amount of time I have spent adding weird hacks to account for the fact that <> is suddenly exposed from Prelude, and therefore my import Data.Monoid ((<>)) isn't necessary, is obscene. The introduction of fiddly CPP to work around this sucks. In fact, this all sucks. It's horrible.

In the light of this, maybe the better patch here is to add -fno-warn-unused-imports to the ghc-options of the .cabal file (resp. -Wno-unused-imports for GHC >= 8) until the oddity in GHC is fixed.

EDIT: I filed https://gitlab.haskell.org/ghc/ghc/-/issues/21879

andreasabel avatar Jul 18 '22 03:07 andreasabel

@treeowl could you please opine on this? We are blocked for more than a month.

Bodigrim avatar Jul 18 '22 19:07 Bodigrim

I agree with @andreasabel that the hiding approach isn't great. Can we avoid it without too much CPP?

treeowl avatar Jul 21 '22 16:07 treeowl

@googleson78 any chance you can pick it up again? If you are busy, I can give it a try myself.

Bodigrim avatar Aug 07 '22 18:08 Bodigrim

If we want to avoid the hiding personally I'd prefer just disabling the warning, as Andreas suggested in

In the light of this, maybe the better patch here is to add -fno-warn-unused-imports to the ghc-options of the .cabal file (resp. -Wno-unused-imports for GHC >= 8) until the oddity in GHC is fixed.

googleson78 avatar Aug 09 '22 10:08 googleson78

@treeowl how do you feel about throwing -fno-warn-unused-imports? I agree that it's probably the least evil.

Bodigrim avatar Aug 09 '22 23:08 Bodigrim

We can definitely disable unused import warnings for older GHC to unstick.

treeowl avatar Aug 09 '22 23:08 treeowl

I reverted the previous changes and disabled that warning globally for containers.

googleson78 avatar Aug 12 '22 15:08 googleson78

Will review after CI

treeowl avatar Aug 12 '22 15:08 treeowl

I meant you can turn off the warnings for old versions. Why do we need to turn them off for all? Is it really too painful otherwise?

treeowl avatar Aug 12 '22 16:08 treeowl

It would need to be turned on for versions newer than whatever ghc/base version ships with the liftA2 export change.

googleson78 avatar Aug 15 '22 10:08 googleson78

Is there a way to confine the mess to a separate module?

treeowl avatar Aug 15 '22 14:08 treeowl

@treeowl @sjakobi could you please take ownership of this issue and merge any solution which suits you? The upstream GHC is blocked on this for two months. Asking a volunteer to explore completely orthogonal solutions just to be rejected on a whim strikes my as unreasonable.

I'd suggest to merge any approach (e. g., the current state of PR) to unblock GHC, and then you can polish it until containers release in whatever direction you wish.

Bodigrim avatar Aug 20 '22 15:08 Bodigrim

I don't consider myself a maintainer of containers at this point, so I take no responsibility for this PR.

Based on the discussion here, I'd suggest to add {-# OPTIONS_GHC -fno-warn-unused-imports #-} and explanatory comments in each of the affected modules.

sjakobi avatar Aug 20 '22 17:08 sjakobi

@treeowl just a kind reminder about this issue. If it's not completed within two weeks, we'll have to patch the GHC mirror of containers to unblock upstream.

Bodigrim avatar Aug 30 '22 23:08 Bodigrim

Ugh. I'll do something tonight or tomorrow.

treeowl avatar Aug 31 '22 00:08 treeowl

@treeowl ping.

Bodigrim avatar Sep 03 '22 21:09 Bodigrim

@treeowl ping.

Time scheduled for this today.

treeowl avatar Sep 04 '22 14:09 treeowl

I'm attempting an alternative approach in #852, which plays "Let's pretend liftA2 has always been in the Prelude.`

treeowl avatar Sep 05 '22 03:09 treeowl

I hate the approach in #852 a bit less, so I'm doing that instead.

treeowl avatar Sep 05 '22 05:09 treeowl

Thanks!

Bodigrim avatar Sep 05 '22 18:09 Bodigrim