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

Clean up and simplify the treatment of implicit binding #532

Draft
wants to merge 59 commits into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Contributor

@Ericson2314 Ericson2314 commented Aug 14, 2022

The review process of #448, to be quite frank, felt lacking. Some last minute changes (which @goldfirere said elsewhere were done based on others unwritten concerns, i.e. weren't even something he was himself championing!) weakened the story around implicit binding, and when I had brought this up I didn't get much by way of replies.

Lest this appear too theoretical, I was also helping @tek implement some of this stuff, and the lack of clear story around implicit binding got in the way there too. One of the reasons variable binding is so inconsistent in GHC Haskell is that the implementation contains lots of repetitive and poorly abstracted code --- in such a situation there are few impediments to stop the various instances of should-have-been abstractions "drifting apart". @tek and I were able to do a great job deduplicate things, but the few remaining places were implicit variables could never be turned off meant we couldn't go all the way. Bummer!


But that's enough grumbling, here are the changes I would have liked to see happen. I have editing the existing proposals for my sanity; I new proposal atop the old one which is atop others is too much and would melt my brain. As such, the motivations are editted and should be self-contained, but I will provide an extra "meta-motivation here".

I had a description here of what I did, a "meta-motivation", but then I accidentally deleted. I guess that's a sign it should be in the repo? It is still in a more conversational GitHub comment not proposal style, but I am happy to move it around / change the language as the committee suggests.

Rendered meta-proposal


Because the information is scattered around previous proposals and the baseline of what GHC implements, it was very difficult to right this proposal and keep everything in my brain.

I know I am a broken record about this, but please let's do something like the "aspirational users guide". The end goal of all this variable stuff is quite simple ---- simpler and more uniform than what we have before it! But the balkanized way we discuss and propose these things and (if need be) their migrations is extremely cognitively burdening.

I feel in writing this proposal, in discussing the weird interactions between proposals both unmerged and merged, code quality issues in GHC, etc., we are pushing against the limit of the complexity and nuance the current proposal process can deal with. And that's a real shame, because complexity we fail to deal with is not complexity that doesn't happen, but complexity that does happen, slipping by beneath our radar. I want to change the process to make there be less work and drudgery, not more.

I am not at all confident I got everything in my revisions correct, but I am quite exhausted thinking about it, and so I open this now in hopes that any lingering errors are easy to catch and fix with more eyeballs.


It is probably easier to go to "file changed" and click the right side icon for a "rich text diff", but out of tradition I include these links.

Rendered meta-proposal
Rendered Principles
Rendered 425 (modified existing)
Rendered 448 (modified existing)

See associated PR for detailed meta-motivation, as this is a change
against proposals (representing changes).
@AntC2
Copy link
Contributor

AntC2 commented Aug 15, 2022

#523 fixes this, by hinting at (it is not fully specified yet) the let type var = _ in syntax ...

Note that bit of #523 got taken out -- with no indication if/when it might be reawakened.

Otherwise, your 'meta-proposal' is so meta, I just don't understand what it's saying/or how it's different to all the other swirl of sub- and sub-sub-proposals. As I understand it, those 'principles' proposals are more setting a direction, and explicitly say they can be tweaked in light of concrete/detailed proposals. So perhaps what you want to refine would come across better in context of something concrete?

Perhaps some code examples would help?

@Ericson2314 Ericson2314 marked this pull request as draft August 15, 2022 12:45
pattern synonym -> pattern signature. Whoops!
principles.rst Outdated Show resolved Hide resolved

- Positive-position signatures' free vars cause implicit ``forall ... .``

- Negative position free vars cause different sorts of binding:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you include a code example illustrating the difference between negative and positive position free vars?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit unsure about to what extent to include examples in the proposed change spec.

principles.rst Outdated Show resolved Hide resolved
principles.rst Outdated Show resolved Hide resolved
principles.rst Outdated Show resolved Hide resolved
principles.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
Ericson2314 and others added 5 commits August 15, 2022 11:09
Thanks!

Co-authored-by: Torsten Schmits <tek@users.noreply.github.com>
Thanks!

Co-authored-by: Torsten Schmits <tek@users.noreply.github.com>
@Ericson2314
Copy link
Contributor Author

Ericson2314 commented Aug 15, 2022

@AntC2

#523 fixes this, by hinting at (it is not fully specified yet) the let type var = _ in syntax ...

Note that bit of #523 got taken out -- with no indication if/when it might be reawakened.

I am not referring to the removed let in patterns, but just regular let ... in in expressions.

Otherwise, your 'meta-proposal' is so meta, I just don't understand what it's saying/or how it's different to all the other swirl of sub- and sub-sub-proposals. As I understand it, those 'principles' proposals are more setting a direction, and explicitly say they can be tweaked in light of concrete/detailed proposals. So perhaps what you want to refine would come across better in context of something concrete?

Yes it is "so meta" :). The underlying proposals should still stand alone. The meta-proposal is just supposed to be for someone thing that "well the old and new versions for 448 both seem fine to me, how did we decide between them?".

Perhaps some code examples would help?

There are new examples in 448, do note.

proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0448-type-variable-scoping.rst Outdated Show resolved Hide resolved
proposals/0532-type-variable-scoping.rst Outdated Show resolved Hide resolved
Ericson2314 and others added 2 commits August 22, 2022 17:40
Thanks @tek!

Co-authored-by: Torsten Schmits <tek@users.noreply.github.com>
@Ericson2314 Ericson2314 marked this pull request as ready for review August 22, 2022 22:08
@Ericson2314
Copy link
Contributor Author

In https://gitlab.haskell.org/ghc/ghc/-/merge_requests/8629#note_449873 @goldfirereprovided the missing context of how the previous revisions came about, and also signalled (If I understand correct) basic agreement with the thrust of this change. Horray!

Since this is an "in the weeds" modification to an existing proposal, and since @goldfirere (the original author, to be clear) doesn't need convincing, I am submitting this straight to the committee.

CC @nomeata

@nomeata
Copy link
Contributor

nomeata commented Aug 23, 2022

Since this builds on top of #448, I’d say @aspiwack is the natural shepherd.

@nomeata nomeata changed the title Clean up and simplify the treatment of implicit binding Clean up and simplify the treatment of implicit binding (under review) Aug 23, 2022
@nomeata nomeata added the Pending shepherd recommendation The shepherd needs to evaluate the proposal and make a recommendataion label Aug 23, 2022
@aspiwack
Copy link
Contributor

As the shepherd for this proposal, I am unwilling to bring this proposal to the committee in its current shape. It does too many things, and it will be impossible to have an effective discussion about it.

To summarize (I hope I'm not forgetting anything), the current PR:

  1. modifies the current list of design principles
  2. changes an already-accepted proposition to refer to these principles
  3. modifies Modern Scoped Type Variables #448 to replace the extension -XImplicitForall with -XImplicitBinds and fold what was previously a warning into it
  4. changes some language that @Ericson2314 doesn't like

In order to yield productive discussions and informed decisions, the proposal should be more focused:

  • (1) and (3) should be different proposals.
  • You shouldn't do (2): it doesn't make much sense to. It's akin to rewriting history and claiming that a proposal was meant to meet a goal that didn't exist at the time
  • You shouldn't do (4) either: there is not enough value in this. Frankly this is not worth your time, or the time of the committee. Only change things that need changing.

I'll mark this proposal as Needs revision The proposal needs changes in response to shepherd or committee review feedback in the meantime.

@aspiwack aspiwack removed the Pending shepherd recommendation The shepherd needs to evaluate the proposal and make a recommendataion label Aug 24, 2022
@aspiwack
Copy link
Contributor

I, personally, won't be there at ZuriHac, so no IRL chit-chatting for me I'm afraid. We'll figure some other way.

@aspiwack aspiwack added Needs revision The proposal needs changes in response to shepherd or committee review feedback and removed Pending committee review The committee needs to evaluate the proposal and make a decision labels May 25, 2023
@aspiwack
Copy link
Contributor

(I've marked the proposal as “Needs revision” to signal that the next action in on John and not me, for internal bookkeeping in the community. The real status is: some discussion about scope is pending; but I say it's a good enough approximation)

@Ericson2314 Ericson2314 marked this pull request as draft July 12, 2023 01:07
@Ericson2314
Copy link
Contributor Author

Based on conversation with @simonpj then @int-index, I am going to put this on pause instead propose a series of tiny changes to #448 we can hopefully digest much more easily.

The first such change is #604

@@ -552,8 +552,14 @@ Additionally, this choice edges us closer to the `Lexical Scoping Principle`_,
because we no longer have to check whether ``a`` is in scope before identifying the ``a`` in ``f (Just @a x) = ...`` is a binding site or an occurrence.

The other change in this restatement is the use of new extension ``-XTypeAbstractions`` instead of the current status of piggy-backing on the combination of ``-XTypeApplications`` and ``-XScopedTypeVariables`` (*both* need to be enabled today).
This proposal suggests instead that ``-XScopedTypeVariables`` implies ``-XTypeAbstractions`` so that we remain backward-compatible with what is current implemented
(though there may be some redundant enablings of ``-XTypeApplications`` that would no longer be needed).
This proposal suggests instead that ``-XScopedTypeVariables`` and ``-XScopedTypeVariables`` should enable this feature in a deprecated manner, to encourage users to use ``-XTypeAbstractions`` instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This proposal suggests instead that ``-XScopedTypeVariables`` and ``-XScopedTypeVariables`` should enable this feature in a deprecated manner, to encourage users to use ``-XTypeAbstractions`` instead.
This proposal suggests instead that ``-XScopedTypeVariables`` and ``-XTypeApppications`` should enable this feature in a deprecated manner, to encourage users to use ``-XTypeAbstractions`` instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nomeata Sorry for the noise, the other PR will take care of that. This one is on hold until it and one I have yet to open are resolved -- trying to avoid huge diffs and get the least controversial parts taken care of first.

I just pushed to this one to make sure I wasn't accumulating conflicts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a different PR? Ah, right! Ignore me then :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs revision The proposal needs changes in response to shepherd or committee review feedback
Development

Successfully merging this pull request may close these issues.

None yet

8 participants