haskell / core-libraries-committee

95 stars 16 forks source link

Data.List should re-export GHC.List.List #182

Closed Kleidukos closed 1 year ago

Kleidukos commented 1 year ago

Proposal

Data.List should re-export GHC.List.List

It is terribly awkward to have

import Data.List qualified as List
import GHC.List (List)

Impact Assessment

I'll fill out this part when I am able to compile the ghc-9.4.4 branch of GHC, for the moment I'm facing hardships with "missing dependencies".

treeowl commented 1 year ago

I don't understand what makes this awkward. Also, when do you need List?

Kleidukos commented 1 year ago

Well, for once, awkwardness levels are influenced by how arm-twisting it is. Today we have the following places for List-related things:

The first one is the official entry point for List operations in the base library. The last two are internal modules for GHC, which should not be accessed directly.

Moreover it is highly uncommon to have such a primordial type defined in an internal module, have its operations re-exported in a public-facing module, but not the type itself.

Hence this proposal to solve a very odd mismatch between GHC.List and Data.List

Also, when do you need List?

When I'm writing a tutorial for beginners and have no desire to showcase the inconsistency of our user experience.

treeowl commented 1 year ago

You haven't yet explained the purpose of importing List at all, which everything else must lean on.

Kleidukos commented 1 year ago

When one imports a type it is usually to use it in a type signature.

mixphix commented 1 year ago

FWIW, I'm in favour of exporting this name from Data.List. I don't foresee many clashes, other than perhaps type synonyms that would then be obsoleted. Some further justification:

phadej commented 1 year ago

Isn't this a moral duplicate of https://github.com/haskell/core-libraries-committee/issues/22. i.e. the plan is accepted, but someone needs to write the patches to aid monomorphisation migration of Data.List.

mixphix commented 1 year ago

I truly don’t understand how this could possibly be interpreted as having anything to do with monomorphization.

dixonary commented 1 year ago

As a relative outsider who is also regularly using List - including in a new learners context - this is an objectively sensible change and I support it without reservation.

It does not appear to be a duplicate (moral or otherwise) of #22 except in that it also concerns lists.

ocharles commented 1 year ago

@phadej perhaps you read this issues as Data.List should re-export GHC.List (the module), while this issue is really about Data.List should re-export GHC.List.List (the type).

tomjaguarpaw commented 1 year ago

To attempt to fill in some blanks (and I didn't know about this until I just looked), it appears that base-4.18.0.0 added data List a which is another way of saying [a]. base-4.17.1.0 didn't have it.

List can be preferable to [] in some ways, for example

(I don't know what the relative status of [] and List is. I think they are/it is (a) built-in definition(s) but I don't know if one of them counts as a synonym for the other.)

jappeace commented 1 year ago

This is a good idea, you've my support. :)

chshersh commented 1 year ago

This CLC issue looks relevant:

I don't like inconsistency. And I'd pretty weirded out by having List easily accessible in base while Tuple doesn't have the same status, even though the existence of both types pursues the same goal (in my understanding).

So my preference is to have the same decision on both of this issues (162 and 182).

mixphix commented 1 year ago

It is no doubt a source of frustration to potential proposers that the outcome of their proposal should depend on the outcomes of other, tangentially-related proposals, for the sake of consistency, rather than accepting the specific incremental changes a proposer puts forth (and declares that they have the bandwidth to pursue). The "inconsistency" may spur another member of the community to do that work for you anyway, after all!

chshersh commented 1 year ago

@mixphix To clarify, I didn't ask the author of this proposal to do more work neither my intention was to block their work by the outcome of another proposal.

It was a non-binding ask to other CLC members to vote the same on both proposals (in any order of vote).

phadej commented 1 year ago

@ocharles I see. thanks.

In https://github.com/haskell/core-libraries-committee/issues/29 CLC argued that "GHC" stuff could as well be exported just from GHC.List. I don't think that GHC proposal 475 will affect ordinary user just yet.

Btw, was there a CLC proposal to add data List to GHC.List in the first place?

Bodigrim commented 1 year ago

Btw, was there a CLC proposal to add data List to GHC.List in the first place?

No, despite me explicitly asking to raise one. There is also no changelog entry and no @since annotation. I'm not sure what kind of explanation, which does not violate an assumption of good faith, is applicable here. CC @int-index.

int-index commented 1 year ago

I'm not sure how to interpret that last message, but if it's an offer of help with writing CLC proposals, changelog entries, and @since annotations, I'll gladly take it. Someone needs to do it.

Kleidukos commented 1 year ago

@int-index yeah sure I can add the changelog entry & @since annotation as part of the MR that this proposal will have. :)

hasufell commented 1 year ago

This might sound nitpicky, but is List an internal?

And if so, we should probably have GHC devs clarify, before we start imposing restrictions on them by doing re-exports without asking.

Although this seems like a no-brainer, we should follow some proper procedure, given the imminent split base proposal.

Kleidukos commented 1 year ago

@int-index We spoke briefly of it on Twitter but perhaps could you emit a clarification regarding the status of the List type? Do we risk anything by exposing it?

int-index commented 1 year ago

@Kleidukos Yes, it's fine. The new names List and Tuple2, Tuple3, Tuple4, etc, are meant to be user-facing. It would be nice to expose the latter from Data.Tuple, too.

Kleidukos commented 1 year ago

I can retrofit this proposal to include both

int-index commented 1 year ago

Doesn't have to be one proposal, that's entirely up to you. I'm just trying to clear up the idea behind GHC Proposal #475. Those new names are meant to be user-facing, but they're exported from GHC.List and GHC.Tuple instead of Data.List and Data.Tuple to minimize potential breakage (no one has done an impact assessment, as far as I know).

mixphix commented 1 year ago

@Kleidukos If you have the capacity to prepare a merge request and impact assessment for exporting both List from Data.List and Tuple<n> from Tuple, it would also be much less effort on the CLC's behalf to vote on a single proposal for the matter, while also guaranteeing consistency. :)

tek commented 1 year ago

(no one has done an impact assessment, as far as I know).

I included some of that in https://github.com/haskell/core-libraries-committee/issues/162, for the names that will be exposed for tuples (though not including the module name Data.Tuple). I can amend that and the List name.

Bodigrim commented 1 year ago

@Kleidukos I'd advise against scope creep. The situation with List is different from Tuple<n>: List is already in base, you are just proposing to move to another, more publicly visible module, while Tuple<n> is not it.

int-index commented 1 year ago

Tuple<n> is also in base.

https://ghc.gitlab.haskell.org/ghc/doc/libraries/ghc-prim-0.10.0-inplace/GHC-Tuple.html

phadej commented 1 year ago

@int-index, that is a link to ghc-prim docs.

int-index commented 1 year ago

that is a link to ghc-prim docs.

Good point. So GHC.List is in base while GHC.Tuple is in ghc-prim? How strange, I didn't even realize that.

Bodigrim commented 1 year ago

Btw, was there a CLC proposal to add data List to GHC.List in the first place?

No, despite me explicitly asking to raise one. There is also no changelog entry and no @since annotation. I'm not sure what kind of explanation, which does not violate an assumption of good faith, is applicable here. CC @int-index.

I'm not sure how to interpret that last message, but if it's an offer of help with writing CLC proposals, changelog entries, and @since annotations, I'll gladly take it. Someone needs to do it.

It's pretty straightforward to interpret: please add a changelog and @since annotations for the changes you sneaked past CLC in !7869 and backport as necessary. Unfortunately, it's too late to revert.

Tuple<n> is also in base.

Thanks for bringing it to the attention of CLC that despite and ignoring a long and thoughtful discussion in https://github.com/haskell/core-libraries-committee/issues/162, a separate MR !9821 introduced unapproved changes to base, namely adding Unit and Tuple<n> to base:Data.Tuple. No changelog and no @since pragmas again. Please revert (both in master and in ghc-9.8 branch). CC @s-and-witch

@int-index @s-and-witch please act on the suggestions within two weeks before I have to escalate. I'm sorry, but I'm extremely unhappy with this kind of issues surfacing again.

tek commented 1 year ago

namely adding Unit and Tuple<n> to base:Data.Tuple

Those types aren't really exported from that module as far as I can tell, Haddock must have some special treatment for ghc-prim that got it confused. Very strange, it also skipped Tuple16 through Tuple64

int-index commented 1 year ago

please add a changelog and @since annotations

OK, "please" works way better than "violate an assumption of good faith", so I'll consider doing it.

the changes you sneaked past CLC

That concludes my considerations, I have better things to do than shuffle documentation upon your request if you can't even make the request without pairing it with accusations in the same sentence.

I did not sneak anything past anyone, I implemented a section of an accepted GHC Proposal. You know, ordinarily I would expect that someone implementing accepted proposals would get gratitude instead of whatever it is that you are expressing, but I am not easily discouraged, so we don't need to dwell on this. I hope, however, that if someone else contributes to GHC by implementing accepted proposals, they get "thank you" instead of this. (Anyway, they'll certainly get a "thank you" from me).

Please revert (both in master and in ghc-9.8 branch).

No such action will take place. The changes implement GHC Proposal #475. If you want to see those changes reverted, the first step is to amend the GHC proposal, the second step is to implement that amendment (i.e. revert the changes).

two weeks before I have to escalate

What does that even mean, do you have issues with GHC Proposals getting implemented?

I'm sorry, but I'm extremely unhappy with this kind of issues surfacing again.

Just a few messages ago I was mostly confused, but by the time I finished typing this response, I certainly share the feeling of unhappiness with you. Perhaps the problem lies in different expectations. Here is how I see the situation:

  1. There is an accepted GHC proposal, namely GHC Proposal #475.
  2. @s-and-witch and I implemented parts of this proposal.
  3. We didn't add @since annotation and changelog entries; an unfortunate omission, but surely in an open source project anyone could fix this. Perhaps the person who spotted the problem?
hasufell commented 1 year ago

No such action will take place. The changes implement GHC Proposal #475. If you want to see those changes reverted, the first step is to amend the GHC proposal, the second step is to implement that amendment (i.e. revert the changes).

That's unfortunately not how it works. We have talked with @simonpj and @adamgundry during ZuriHac and made some great progress on our mutual understanding of CLC <-> GHC SC.

An accepted GHC proposal that touches base is not guaranteed to be accepted by CLC.

It is the responsibility of the steering committee to contact CLC for such proposals, source their (non-binding) opinions and finally raise an MR and a CLC proposal and wait for approval before merging.

We do want this to synchronize better, which is why that's described in the joint HF tech proposal: https://github.com/Ericson2314/tech-proposals/blob/ghc-base-libraries/proposals/accepted/051-ghc-base-libraries.rst#62ghc-proposals-process

int-index commented 1 year ago

An accepted GHC proposal that touches base is not guaranteed to be accepted by CLC.

That's new. You linked to a fork of tech-proposals, so it's only an idea for a new process and it hasn't been accepted by the HF. And I see two major issues with this idea:

  1. Proposal authors shouldn't need to interact with two committees instead of one. It is generally fine to have coordination between committees, so I can imagine a system where GHC SC cannot approve a proposal on its own and must contact CLC when a proposal touches base. Proposal authors mustn't be bothered with the intricacies of this process, they need to write a single document, submit it on a single platform, and then either have it accepted or rejected, whether by GHC SC alone or by a joint GHC SC + CLC committee.
  2. Proposal implementors need a single source of truth for the accepted changes. If the proposal document has been marked accepted, it means that it's ready for implementation. Now, I have no problem whatsoever with involving CLC before a proposal is accepted, but once it's done, it's done. It's quite important to be able to point potential contributors to a document that describes the changes we want to see in GHC and its libraries, and it can't be that parts of this document are actually accepted and parts of it are kind-of-accepted-but-non-binding-and-another-proposal-on-another-platform-is-pending.

So I agree with you that it's important to get CLC involved, but for the sake of proposal authors and proposal implementors, this should be a committee-to-committee interaction, not a person-to-two-committees interaction; and acceptance of a proposal needs to be a single atomic operation instead of having kind-of-accepted documents floating around in the ghc-proposals repo.

Not to mention, both 475 and its implementation predate this discussion and this idea for a new process, so accusations of "sneaking" changes past a committee are baseless and quite frankly out of line.

hasufell commented 1 year ago

That's new.

It's not.

Please re-read the charta: https://github.com/haskell/core-libraries-committee#base-package

Nowhere does it say that GHC steering committee gets a free pass.

You linked to a fork of tech-proposals, so it's only an idea for a new process and it hasn't been accepted by the HF.

Not accurate. This proposal has been a joint effort between CLC and GHC steering committee. I'm confused and surprised why you haven't been informed about its progression, since multiple GHC SC members were involved.

The PR is already up, @Ericson2314 is a co-author and is fixing up wording issues as we continue to discuss it in public and in private: https://github.com/haskellfoundation/tech-proposals/pull/51

And no, we don't expect the HF to vote on it formally. It's not their responsibility and they have basically already expressed their votum by asking the CLC to get on board: https://github.com/haskell/core-libraries-committee/issues/145

CLC hasn't formally acked it yet, but it's fair to say it seems to enjoy wide consensus so far.

But that doesn't really matter. It's more of a clarification, not a new rule.

Does that answer your questions?

s-and-witch commented 1 year ago

Please revert (both in master and in ghc-9.8 branch). CC @s-and-witch

@s-and-witch please act on the suggestions within two weeks before I have to escalate.

Please point to the exact lines in libraries/base touched by !9821 that you want to be reverted within two weeks before I have to escalate.

int-index commented 1 year ago

I'm confused and surprised why you haven't been informed about its progression, since multiple GHC SC members were involved.

@hasufell I was informed, but apparently I did not pay enough attention. My understanding was that a new package ghc-experimental is getting split out of base, and I don't particularly care how definitions are distributed across packages (I'd simply add both base and ghc-experimental to my build-depends), so I was not compelled to participate in the discussion. However, I didn't realize that it affected already accepted proposals such as 475 retroactively, nor that it meant that GHC Proposals couldn't be used as a source of truth for accepted changes to GHC and its libraries. Now that you brought this up, I raised this concern on the GHC SC mailing list.

Does that answer your questions?

Well, I didn't ask any, but your message does clear things up, so thank you. In particular, the fact that "it's more of a clarification, not a new rule" is quite a shift in perspective.

Now, there are two ways to interpret it.

  1. It could mean that GHC Proposals can't be taken as a source of truth to guide implementation. This seems to be the interpretation that you are suggesting, but I've already raised my objection to this point of view.
  2. It could mean that the GHC Steering Committee should have contacted the Core Libraries Committee before accepting 475, as this proposal changes base.

Looking at the mailing list archives, I see that it was I who shepherded 475. I coordinated the discussion between the GHC SC members, but it didn't occur to me to reach out to an external committee. Seems like a similar thing happened with 330, which I also shepherded. I will adjust my practice in light of this discussion, so if I shepherd any proposals that affect base in the future, I will get (binding!) CLC feedback before declaring the proposal accepted.

It's not doing anyone any good to have proposals that are accepted-but-not-really.

hasufell commented 1 year ago

It could mean that GHC Proposals can't be taken as a source of truth to guide implementation. This seems to be the interpretation that you are suggesting, but I've already raised my objection to this point of view.

This is correct. Further, @adamgundry explained to us in person at ZuriHac that GHC proposals are quite different from CLC proposals: sometimes a GHC proposal is accepted, but later it is discovered that the implementation is impossible or otherwise problematic and the whole thing is dropped. Regardless of CLC, an accepted GHC proposal is already not guaranteed to make it into GHC.

This is quite different from how CLC operates and might be one of the reasons for tension: CLC requires a working implementation before accepting anything.

It could mean that the GHC Steering Committee should have contacted the Core Libraries Committee before accepting 475, as this proposal changes base.

This is what the tech-proposal suggests... consulting CLC beforehand to get a non-binding opinion.

But it's also fine for GHC SC to accept a proposal and say "we'll have to convince CLC later". It's just a matter of expectations, how you run your process and what your contributors are willing to get into.


In that light, I hope you can see why @Bodigrim is unpleasantly surprised and why merging this MR without CLC approval can be interpreted as disrespecting the authority of the CLC committee.

I think we have to work towards a situation of mutual respect and trust, for the sake of our contributors.

CLC doesn't find any pleasure in blocking progress, but we may have different goals and priorities than GHC SC, for example. And it's important to acknowledge that.

s-and-witch commented 1 year ago

In that light, I hope you can see why @Bodigrim is unpleasantly surprised and why merging this MR without CLC approval can be interpreted as disrespecting the authority of the CLC committee.

There are things that CLC committee can do to avoid such situations in the future, for example add into headers of already accepted GHC proposals cautions like "this proposal partially/fully blocked on CLC, see discussion [here](https://github.com/haskell/core-libraries-committee/......)"

I'm not involved in all these administrative processes, I have no time to read all the discussions in all the organizations (GHC proposals, CLC, HF? Something else? IDK) to know what I can implement and what I can't. Do you agree with that no one can/should enforce developers to be involved in all kinds of bureaucratic processes?

If the answer is "yes", then lacking of information about CLC in accepted GHC proposals is a design gap on your side, not on mine. But it turns out that I am a sneaky developer, that (to show disrespect to the authority of the CLC committee, right?) somehow silently merged (after two months of review with 7 other participants of the discussions. Are they partners in my crime?) the patch (that even did not introduce API changes in base) and now I should revert changes (what changes? I did not even touch base:Data.Tuple in the patch) to avoid escalation.

Bodigrim commented 1 year ago

OK, "please" works way better than "violate an assumption of good faith", so I'll consider doing it.

Thanks, this is much appreciated.

I did not sneak anything past anyone, I implemented a section of an accepted GHC Proposal.

You seem to assume that GHC SC has an authority to approve changes to base. It does not, and it had no such authority when GHC Proposal 475 was approved. I understand that at times the line can be blurry, but in this case you were specifically notified that this change requires CLC approval.

I hope this clarifies.

int-index commented 1 year ago

Those types aren't really exported from that module as far as I can tell, Haddock must have some special treatment for ghc-prim that got it confused. Very strange, it also skipped Tuple16 through Tuple64

@tek Indeed, this seems to be the case. I found this code in Haddock.Interface.Create:

    -- See Note [Exporting built-in items]
    special_exports
      | mdl == gHC_TYPES  = listAvail <> eqAvail
      | mdl == gHC_PRIM   = funAvail
      | mdl == pRELUDE    = listAvail <> funAvail
      | mdl == dataTupleModule = tupsAvail
      | mdl == dataListModule  = listAvail
      | otherwise         = []

And here is the referenced Note:

-- Note [Exporting built-in items]
--
-- Some items do not show up in their modules exports simply because Haskell
-- lacks the concrete syntax to represent such an export. We'd still like
-- these to show up in docs, so we manually patch on some extra exports for a
-- small number of modules:
--
--   * "GHC.Prim" should export @(->)@
--   * "GHC.Types" should export @[]([], (:))@ and @(~)@
--   * "Prelude" should export @(->)@ and @[]([], (:))@
--   * "Data.Tuple" should export tuples up to arity 15 (that is the number
--     that Haskell98 guarantees exist and that is also the point at which
--     GHC stops providing instances)
--

The fix should be relatively simple. Now that (~), List, and Tuple<n> are names that can be mentioned in export lists, we can drop the special cases.

tek commented 1 year ago

I found this code in Haddock.Interface.Create:

Damn, I grepped for 10 minutes and couldn't find it. Good to know!

hasufell commented 1 year ago

Do you agree with that no one can/should enforce developers to be involved in all kinds of bureaucratic processes?

Yes, we have to document this better for contributors. This is what the tech-proposal tries to do https://github.com/haskellfoundation/tech-proposals/pull/51

So please bear with us.

int-index commented 1 year ago

Alright, so I've pushed some code that should address the concerns raised in this thread.

  1. haddock!35 fixes Haddock rendering of List, Unit, Solo, and Tuple<n>. The tuple names are not exported from Data.Tuple, it's a rendering oddity. I now understand what the request to revert was about, it was based on incorrect information presented to us by Haddock. Fortunately, there is nothing to revert.
  2. ghc!10835 adds the missing changelog.md entries and the @since annotations that @Bodigrim asked for. List is originally defined in ghc-prim, so its @since annotation specifies the version of ghc-prim rather than base. I don't know if there is a way to add a @since annotation for a reexport.

I also opened #186 to get approval from the CLC regarding the List export from GHC.List.

simonpj commented 1 year ago

This confused thread is no one's fault, and no one has acted in bad faith.

Rather, it is a consequence of a structural lack of clarity about how the GHC Steering Committe and the Core Libraries Committee can work together without tripping each other up.

The good news is that we have been working hard on that in the last couple of months, and we have a detailed plan. The current step is seeking agreement from the GHC SC and CLC.

Returning to this thread, the trouble is that GHC proposal #475 predated that plan, and yet is a prime case in point. Vlad and his colleagues are not to blame for that; they proceeded in good faith. But we now need to work out what best to do next.

I don't have an opinion about which is best, but I think the best course of action is to make clear which of these you'd like to propose (or a third path), and seek agreement for that path. I believe you are taking the second path? But CLC proposals #182 and #185 don't cover the (substantial) additive elements of that MR, do they? Or are Tuple2, Tuple3 etc only exported from ghc-prim and not exposed in base at all? I'm sorry, but I have lost track of what the exact library changes are. Maybe you can just lay them out for us?

Meanwhile @bodigrim perhaps you can help us by saying what else the CLC needs on this one, beyond the MR that @int-index posted today

@int-index, I know this is frustrating. It's not your fault that you have become a vicitim of shortcomings our process. But we are where we are, and Julian/Andrew are right: changes to base need CLC approval.

We (the GHC SC) will try to do better in future. My apologies for having failed to do so this time round.

Bodigrim commented 1 year ago

Alright, so I've pushed some code that should address the concerns raised in this thread.

Thanks a ton for a swift turnaround on this, @int-index. I believe we are all good now.

tek commented 1 year ago

@simonpj

Or are Tuple2, Tuple3 etc only exported from ghc-prim and not exposed in base at all? I'm sorry, but I have lost track of what the exact library changes are.

They were intended to be exposed in GHC.Prelude, a new module in base, for which we asked for approval in #162, which spurred more discussion about split base. The most likely scenario, in my view, is for them to be exposed from `ghc-experimental.

However, so far the new TupleN definitions are indeed only exported from ghc-prim.

simonpj commented 1 year ago

They were intended to be exposed in GHC.Prelude, a new module in base, for which we asked for approval in https://github.com/haskell/core-libraries-committee/issues/162, which spurred more discussion about split base. The most likely scenario, in my view, is for them to be exposed from `ghc-experimental.

Ah, thank you. Perhaps the group of people working on this (you, Vlad, etc) can decide what you would prefer, and then re-launch #162, or alterantively withdraw it in favour of ghc-experimental?

We defininitely don't want to encourage users to depend on ghc-prim!

tek commented 1 year ago

I started a discussion at https://gitlab.haskell.org/ghc/ghc/-/merge_requests/8820 .

phadej commented 1 year ago

We defininitely don't want to encourage users to depend on ghc-prim!

Why not? If I want magicDict (and I want it often enough), why can't look for a GHC specific thing in ghc-prim where it's defined? It's not like it being in ghc-experimental will make it any more stable, will it?

In other words, what is the value of re-exporing TupleN from somewhere else? I guess we'll see, but IMHO ghc-prim is in fact better structured than GHC.Exts in base, which is just a kitchen sink with all the sharp chef knives mixed up with silver spoons.