Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Kleidukos opened this issue Jun 28, 2023 · 92 comments
Closed

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

Kleidukos opened this issue Jun 28, 2023 · 92 comments
Labels
approved Approved by CLC vote base-4.20 Implemented in base-4.20 (GHC 9.10)

Comments

@Kleidukos
Copy link
Member

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
Copy link

treeowl commented Jun 28, 2023

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

@Kleidukos
Copy link
Member Author

Kleidukos commented Jun 28, 2023

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

  • Data.List
  • GHC.List
  • GHC.OldList

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
Copy link

treeowl commented Jun 28, 2023

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

@Kleidukos
Copy link
Member Author

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

@mixphix
Copy link
Collaborator

mixphix commented Jun 29, 2023

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:

  • Many modules are designed with overlapping names, to aid the programmer's memory.
    import Data.List qualified as List (uncons, (\\), ...)
    import Data.Map qualified as Map (fromList, (\\), ...)
    import Data.Set qualified as Set (fromList, (\\), ...)
    import Data.Text qualified as Text (uncons, ...)
    import Data.Vector qualified as Vector (fromList, uncons, ...)
  • Many of these modules are named after their type, which is usually imported unqualified:
    -- import Data.List (List)
    import Data.Map (Map)
    import Data.Set (Set)
    import Data.Text (Text)
    import Data.Vector (Vector)
  • The lack of a bona fide prefix name for the type former [] is a historical mistake that has only increased the complexity of parsers, and creates a syntactic anomaly at a level where a new programmer is still being introduced to the language. It's a linked list -- it's incredibly simple and useful, but it's not magic!
  • Here are some examples of how the current syntax -- "punning" between value- and type-level -- can lead to confusion for newcomers.

@phadej
Copy link

phadej commented Jun 29, 2023

Isn't this a moral duplicate of #22. i.e. the plan is accepted, but someone needs to write the patches to aid monomorphisation migration of Data.List.

@mixphix
Copy link
Collaborator

mixphix commented Jun 29, 2023

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

@dixonary
Copy link

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
Copy link

@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
Copy link
Member

tomjaguarpaw commented Jun 29, 2023

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

  • Avoiding type/term punning
  • Avoiding privileged syntax

(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
Copy link

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

@chshersh
Copy link
Member

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
Copy link
Collaborator

mixphix commented Jun 29, 2023

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
Copy link
Member

@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
Copy link

phadej commented Jun 29, 2023

@ocharles I see. thanks.

In #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
Copy link
Collaborator

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
Copy link

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
Copy link
Member Author

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

@hasufell
Copy link
Member

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
Copy link
Member Author

@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
Copy link

@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
Copy link
Member Author

I can retrofit this proposal to include both

@int-index
Copy link

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
Copy link
Collaborator

mixphix commented Jun 30, 2023

@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
Copy link

tek commented Jun 30, 2023

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

I included some of that in #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
Copy link
Collaborator

Bodigrim commented Jul 7, 2023

@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.

@Bodigrim Bodigrim added the awaits-impact-assessment Awaits an impact assessment (e. g., using https://github.com/Bodigrim/clc-stackage) label Jul 7, 2023
@int-index
Copy link

int-index commented Jul 8, 2023

@phadej
Copy link

phadej commented Jul 8, 2023

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

@int-index
Copy link

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
Copy link
Collaborator

@Kleidukos if there is no progress within two weeks, I'll close as abandoned.

@Kleidukos
Copy link
Member Author

@Bodigrim I should be able to give a proper patch list on Monday! :)

@Kleidukos
Copy link
Member Author

Alright, I was actually wrong in my first observation, there are five patches needed:

In the last patch I toyed a bit with CPP, which is what should be expected of the final patches that I shall send to these packages once the version of base that will hold the new export will be known.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Oct 9, 2023

Thanks @Kleidukos! Could you also point us to an MR with the proposed change? Sorry if you mentioned it already, I’m from mobile.

@Kleidukos
Copy link
Member Author

Kleidukos commented Oct 9, 2023

@Bodigrim Sorry I believe I don't fully understand what you mean, if you are referring to concrete patches for the build problems, they are listed in my above comment (which is what I read I should do before approval, and then submit the patches to the packages after approval). Or perhaps are you thinking of a MR to head.hackage? Or did I really misunderstand the assessment section of the guide? (in which case I am sorry)

@Bodigrim
Copy link
Collaborator

Bodigrim commented Oct 9, 2023

All good with impact assessment, many thanks :) I'm talking about a draft GHC MR, adding the reexport (just two lines of changes and changelog, nothing fancy).

@Kleidukos
Copy link
Member Author

Heavens, apologies for the evening brain. Sure, it's underway. :)

@Kleidukos
Copy link
Member Author

@Bodigrim Bodigrim removed the awaits-impact-assessment Awaits an impact assessment (e. g., using https://github.com/Bodigrim/clc-stackage) label Oct 12, 2023
@Bodigrim
Copy link
Collaborator

Dear CLC members, following the non-binding discussion above, let's vote on the proposal to re-export GHC.List.List from Data.List. The (trivial) MR is available at https://gitlab.haskell.org/ghc/ghc/-/merge_requests/11426 (disregard CI failures, they are inconsequential). The impact assessment is available above: just a few name clashes, which the proposer has patches ready to be submitted (and there are 6+ months until GHC 9.10 to incorporate the change).

@mixphix @hasufell @tomjaguarpaw @parsonsmatt @angerman @velveteer

@hasufell
Copy link
Member

+1 under the condition that the documentation remarks raised here are taken care of

@tomjaguarpaw
Copy link
Member

+1

@Kleidukos
Copy link
Member Author

@hasufell Please see https://gitlab.haskell.org/ghc/ghc/-/merge_requests/11426, feedback would be welcome on the topic of documentation.

@parsonsmatt
Copy link

+1

@Bodigrim
Copy link
Collaborator

@mixphix @angerman @velveteer just a gentle reminder to vote.

@velveteer
Copy link
Contributor

+1

1 similar comment
@mixphix
Copy link
Collaborator

mixphix commented Oct 18, 2023

+1

@Bodigrim
Copy link
Collaborator

+0. I'm generally opposed to have two names for the same thing and reluctant to make sacrifices just for the sake of non-punning Dependent Haskell syntax. But given that it's List internally already, and both names resurface in documentation already, embacing the change might be a reasonable option.

Thanks all, there are enough votes of unconditional support to approve. Further documentation improvements, if any, do not require a CLC vote.

@Bodigrim Bodigrim added the approved Approved by CLC vote label Oct 18, 2023
@Kleidukos
Copy link
Member Author

Thanks everyone for your patience. 🙇

sthagen pushed a commit to sthagen/ghc-ghc that referenced this issue Oct 19, 2023
Kleidukos added a commit to Kleidukos/ptr that referenced this issue Nov 16, 2023
This is done to allow build with CLC Proposal #182
(haskell/core-libraries-committee#182)
Kleidukos added a commit to Kleidukos/ptr that referenced this issue Nov 16, 2023
This is done to allow build with CLC Proposal #182
(haskell/core-libraries-committee#182)
Kleidukos added a commit to Kleidukos/strict-list that referenced this issue Nov 16, 2023
This is done to allow build with CLC Proposal #182
(haskell/core-libraries-committee#182)
Kleidukos added a commit to Kleidukos/ptr that referenced this issue Nov 16, 2023
This is done to allow build with CLC Proposal #182
(haskell/core-libraries-committee#182)
Kleidukos added a commit to Kleidukos/rattletrap that referenced this issue Nov 16, 2023
Kleidukos added a commit to Kleidukos/rattletrap that referenced this issue Nov 16, 2023
@Bodigrim Bodigrim added the base-4.20 Implemented in base-4.20 (GHC 9.10) label Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved by CLC vote base-4.20 Implemented in base-4.20 (GHC 9.10)
Projects
None yet
Development

No branches or pull requests