-
Notifications
You must be signed in to change notification settings - Fork 272
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
Amend 475 to use ghc-experimental for exports #603
Conversation
Looks good to me. |
proposals/0475-tuple-syntax.rst
Outdated
many *unsafe* internals. | ||
|
||
This proposal lists several definitions to be exported from ``GHC.Prelude``, but leaves other | ||
definitions to be added in separate proposal(s). | ||
|
||
``GHC.Tuple`` will clash with the same module from ``ghc-prim``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid the clashes? Nothing compels us to create a conflicting GHC.Tuple
in ghc-experimental
, at least not for the sake of this proposal. Just leave it in ghc-prim
?
The new GHC.Sum
and GHC.Prelude
can go to ghc-experimental
, of course, as there is no conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid the clashes? Nothing compels us to create a conflicting GHC.Tuple in ghc-experimental, at least not for the sake of this proposal. Just leave it in ghc-prim?
Good qn. But if we leave it in ghc-prim
then clients of the new tuple stuff will be force to depend on ghc-prim
which we do not want. Rather, I think we should transition ghc-prim:GHC.Tuple
to become ghc-prim:GHC.Internals.Tuple
. Quite how fast we do that isn't clear to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our new GHC.Tuple
depends on base
(it needs TypeNats
and possibly TypeError
), so leaving it in ghc-prim
is a no-go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that the module naming scheme is clear, GHC.Tuple
can stay in ghc-prim
!
proposals/0475-tuple-syntax.rst
Outdated
peculiar to GHC that are safe to use in ordinary code. | ||
This module is *not* intended as a replacement to ``Prelude``, | ||
but instead a complement to it. This module is a counterpart to ``GHC.Exts``, which exports | ||
1. Create three new modules in ``ghc-experimental`` called ``GHC.Prelude``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GHC.Prelude
is actually taken by the GHC API for the custom Prelude used by GHC itself. Are we absolutely sure we want to repurpose this name, when we could simply pick a different one? Need opinion from @goldfirere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh -- that is annoying.
It demonstrates that we are using the module prefix "GHC." in two different ways:
- Modules of GHC itself (e.g. GHC.Core.Opt.Simplify)
- Modules in
ghc-experimental
.
These are not the same purpose.
Maybe ghc-experimental
should start with GHCX.
? Thus GHCX.Prelude
, GHCX.Tuple
. Or some other prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GHCX.*
is just a slightly different take on GHC.Exts.*
that @tek previously suggested, right? I believe GHC.Exts.*
is good and descriptive. For example, GHC.Exts.Tuple
is "GHC's extensions to tuples". When those get stabilized, they can be reexported from Data.Tuple
(with CLC's approval)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder about the broader situation here - would the idea be that all GHC.Exts.*
modules/names end up in non-GHC.Exts.*
modules if/when brought into base
? Since if anything is added to base
also under the GHC.Exts.*
moniker, you would still end up with a naming conflict during the deprecation period from ghc-experimental
(edit: actually I suppose there might not be a deprecation period if it's just reexported from base
, and stays in ghc-experimental
, but that doesn't affect my question I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GHC.Exts
exposes some unsafe operations, as in "crash the process" unsafe, which the original text uses as contrast for the purpose of GHC.Prelude
…would calling the new namespace GHC.Exts.*
associate ghc-experimental
with that notion?
On the other hand, GHC.Exts
might be a candidate for migration to ghc-internals
.
There is also a large number of GHC.*
modules in base
that might be a source for additional conflicts later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the prefix GHC.
for things in ghc-experimental strange, as they are neither about GHC (e.g. a plugins) or part of GHC (e.g. GHC.Prelude). It's as if I start to prefix my library modules with Nomeata.
. I expect developers to find this naming confusing (me already included.)
How about we append Experimental
to the module name from base where things might end up in? Data.Tuple.Experimental
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing is should GHC's experiments have a monopoly on such names?
Data.Tuple.GlasgowExperiment
lol
What it is comes first, "compilation" is not relevant and thus unmentioned, who experiment and so not to conflict with other experiments comes last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now called Prelude.Experimental
.
Or prefix it, so that (I can't speak for others but I'm quite open to renaming the library itself too, incidentally, but just |
|
This somewhat overlaps with the discussion at haskellfoundation/tech-proposals#53. I would suggest we use |
That seems like a reasonable schema to me, and it saves us the discussion about But exporting |
Indeed, so the benefits are limited. I was just thinking about what else |
MR now reflects these changes: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/8820/diffs?commit_id=9be5e1645df92e217a2ce5f806940cba016277f7 |
29ee893
to
3579d91
Compare
@nomeata would you submit, please? |
I am not sure if this needs the full committee process. Isn’t it the only reasonable choice given the (accepted) plan around the use of ghc-experimental? If @simonpj or @adamgundry could sanity-check the diff I’m inclined to just merge as it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pushing this on @tek! I have a few minor nitpicks but I agree that a full committee review seems unnecessary given that most of this is essentially predetermined.
I'm wondering whether it helps or hinders to give all the fully-qualified module names referring to modules in ghc-prim
. Arguably, where things live in ghc-prim
/ghc-internals
are implementation details that needn't be part of the specification, provided we are clear about what is being exposed from base
(nothing?) and ghc-experimental
. But I'll leave this up to your preference.
proposals/0475-tuple-syntax.rst
Outdated
the definitions below in ``GHC.Tuple`` on ``GHC.TypeLits``.) :: | ||
#. Export the following definitions from ``Data.Tuple.Experimental`` and | ||
``Prelude.Experimental``. (Implementation note: These would probably end up | ||
in a new module ``Data.Tuple.Prim``, due to the dependency from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this renaming intended? Further down the proposal still refers to GHC.Tuple.Prim
. Given the name I assume this would be a new module in ghc-prim
, but it wouldn't hurt to be explicit. It's not obvious to me why we need all of GHC.Prim
, GHC.Tuple
and GHC.Tuple.Prim
, but those are implementation details that aren't essential to the specification anyway.
in a new module ``Data.Tuple.Prim``, due to the dependency from | |
in a new module ``GHC.Tuple.Prim``, due to the dependency from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, now that we use Data.Tuple.Experimental
instead of GHC.Tuple
(which was supposed to move to base
), we could probably use the latter for the new definitions instead of GHC.Tuple.Prim
. I'll change that and see if it creates any issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, but GHC.Tuple.Prim
is already in master due to an older MR that implements only a small part of the proposal – should we discard it anyway and break the API of ghc-prim
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamgundry what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ghc-prim
has no stability guarantees, so you should feel free to remove GHC.Tuple.Prim
unless there is some implementation reason to keep it.
where there are *n-1* commas. | ||
|
||
#. An occurrence of ``GHC.Exts.Unit#`` is pretty-printed as ``(# #)``. | ||
#. An occurrence of ``GHC.Types.Unit#`` is pretty-printed as ``(# #)``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seemingly have GHC.Types.Unit#
here, but GHC.Exts.Unit#
elsewhere, and when Unit#
is introduced it says it "would likely be exported from GHC.Prim
originally". (And similarly for the other types.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those should all have been changed to GHC.Types
, I overlooked a few occurrences.
GHC.Prim
is a virtual module, so my assumption is that Richard hadn't determined the precise module yet at the point of writing it.
The straightforward change would have been to use Data.Tuple.Experimental
instead of GHC.Exts
– I used GHC.Types
because I now know what we landed on eventually…do you think that's too much retconning? We could also change the other sentence to say "would likely be exported from GHC.Types
originally".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah no, there's no equivalent of GHC.Exts
with the new schema…
Co-authored-by: Adam Gundry <[email protected]>
Please could the propsal say clearly
Currently all these changes are mixed together, and only module names are given, not packages, so I can't work out the answer. Could you simply summarise in one place? I think there is quite a bit of (1) changes, and they need agreement from CLC. E.g. changes to I notice mention of |
Good point. I was assuming there would be no EDIT: actually, I realise now that |
I think that, more than just edinting point 10, it would help the reader a lot if there was a brief summy of
The existing structure is fine; just a summary is needed so that the reader can quickly get that overview. |
I used the libraries section we added to the template last week. Other than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proposals/0475-tuple-syntax.rst
Outdated
#. Export the following definitions from ``GHC.Exts`` and ``GHC.Prelude``. These replace | ||
the existing tuple definitions (in ``GHC.Classes``) today. (Note that ``(...) =>`` is special syntax, and does not | ||
#. Replace the existing constraint tuples in ``GHC.Classes`` with the following | ||
definitions and export them from ``Data.Tuple.Experimental`` and ``Prelude.Experimental``. :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitions and export them from ``Data.Tuple.Experimental`` and ``Prelude.Experimental``. :: | |
definitions and export them from ``Data.Tuple.Experimental`` and ``Prelude.Experimental``. |
proposals/0475-tuple-syntax.rst
Outdated
#. ``ghc-experimental``: | ||
- ``Data.Tuple.Experimental`` will export all tuple-related entities. | ||
- ``Data.Sum.Experimental`` will export all sum-related entities. | ||
- ``Prelude.Experimental`` will export all of the above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RST list syntax is somehow not quite right here.
proposals/0475-tuple-syntax.rst
Outdated
the definitions below in ``GHC.Tuple`` on ``GHC.TypeLits``.) :: | ||
#. Export the following definitions from ``Data.Tuple.Experimental`` and | ||
``Prelude.Experimental``. (Implementation note: These would probably end up | ||
in a new module ``Data.Tuple.Prim``, due to the dependency from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ghc-prim
has no stability guarantees, so you should feel free to remove GHC.Tuple.Prim
unless there is some implementation reason to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally fine, thank you. Just cosmetic suggestions.
proposals/0475-tuple-syntax.rst
Outdated
This proposal lists several definitions to be exported from ``Prelude.Experimental``, | ||
but leaves other definitions to be added in separate proposal(s). | ||
|
||
All of the wired-in types must be declared in ``ghc-prim``, and re-exported from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't part of the specification -- it's an implementation note. Mabye
Implementation note: many type definitions will be "wired-in", defined in `ghc-prim`, and simply re-exported from `ghc-experimental`.
proposals/0475-tuple-syntax.rst
Outdated
These would probably end up in a new module ``GHC.Tuple.Prim``, due to the dependency from | ||
the definitions below in ``GHC.Tuple`` on ``GHC.TypeLits``.) :: | ||
#. Replace the existing tuples in ``GHC.Tuple`` with the following definitions | ||
and export them from ``Data.Tuple.Experimental`` and ``Prelude.Experimental``. :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not talk about replacing anything. That's an implementation matter. Perhaps just
Export the following type definitions from `Data.Tuple.Experimental`.
``Prelude.Experimental`` exports definitions peculiar to GHC that are safe to | ||
use in ordinary code, but whose API may evolve rapidly over subsequent releases. | ||
This module is *not* intended as a replacement to ``Prelude``, but instead a | ||
complement to it. This module is a counterpart to ``GHC.Exts``, which exports | ||
many *unsafe* internals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhpas add
`Prelude.Experimental` simple re-exports the all the exports of `Data.Tuple.Experimental` and `Data.Sum.Experimental`, thus
module Prelude.Experimental (
module Data.Tuple.Experimental,
module Data.Sum.Experimental
) where
import Data.Tuple.Experimental
import Data.Sum.Experimental
Now you don't need to talk about `Prelude.Experimental` any futher.
In fact you could specify `Prelude.Experimental` as a separate bullet.
proposals/0475-tuple-syntax.rst
Outdated
#. Export the following definitions from ``GHC.Exts`` and ``GHC.Prelude``. These replace | ||
the existing tuple definitions (in ``GHC.Classes``) today. (Note that ``(...) =>`` is special syntax, and does not | ||
#. Replace the existing constraint tuples in ``GHC.Classes`` with the following | ||
definitions and export them from ``Data.Tuple.Experimental`` and ``Prelude.Experimental``. :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment. Don't talk about replacing or about GHC.Classes
. Those are implementation matters. Simply write down the definitions visible through Data.Tuple.Experimental
.
I agree we don't need to consult but I think it would be polite to send an email to the CLC mailing list, drawing their attention to these changes. |
@adamgundry @simonpj I believe I have implemented all your suggestions, and sent an email to the CLC. |
Co-authored-by: Adam Gundry <[email protected]>
@adamgundry , shall I merge? |
There's something I don't understand here. Apparently there is one change to
but I don't understand what "the |
(Please don't hold up moving forward with this proposal on my account. It's probably just me being dim or having forgotten something I knew about before. But the CLC was explicitly informed of this proposal and this CLC member doesn't understand the one small part that seems to impinge on the CLC.) |
@tomjaguarpaw that's just to say that the implementation of the part of the proposal that introduces |
So #475 was partially implemented and now you're revising it so that the rest of the implementation uses |
@tomjaguarpaw that's correct |
Great, thanks for clarifying! |
small tweak: mentioned |
LGTM, yes please! |
Motivated by recent conversations with the CLC about stability of the API of
base
, we devised the plan to introduce a new package,ghc-experimental
, that shall become the initial home for exports of new entities that should be tried out by the community before committing to a stable API.This amendment changes the modules exporting the new tuple- and sum-related entities to use that new package.
I'd appreciate feedback, in particular about whether the creation of this package should be part of the proposal or be handled separately, and whether the deprecation of
ghc-prim:GHC.Tuple
for the rename toGHC.Internal.Tuple
should be included here.