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

Deprecated instances #575

Merged

Conversation

int-index
Copy link
Contributor

@int-index int-index commented Jan 27, 2023

The proposal has been accepted; the following discussion is mostly of historic interest.


GHC allows to deprecate modules, functions, data constructors, type constructors, but not instances. The lack of support for deprecation of instances makes it impossible to remove them gracefully, with an advance warning to the users. We propose to allow {-# WARNING ... #-} and {-# DEPRECATED ... #-} pragmas on instances to correct this.

Rendered

@angerman
Copy link
Contributor

Thank you @int-index for writing this up! It's somewhat telling that the only nitpick I can find is the deprecation message 😅 in the example. I think

See haskell/deepseq#16 (https://github.com/haskell/deepseq/issues/16)

would be better than

See deepseq issue #16

as the second one assumes someone to knows where deepseq is hosted, and how to find issue #16 from there. (the other is that I'm just lazy and like clicking things in terminal 😱).

@int-index
Copy link
Contributor Author

I leave the exact wording to deepseq maintainers.

@angerman
Copy link
Contributor

I leave the exact wording to deepseq maintainers.

Fair, I was more concerned about this setting a precedence, where the next person needing this Deprecation ends up reading the proposal, and then copies the format ;-) Not specifically about this deepseq one. After all it's just an example.

@tbidne
Copy link

tbidne commented Jan 27, 2023

Another ignominious example: IsString ByteString.

@tomjaguarpaw
Copy link
Contributor

Strong agree that we need this. But I wonder whether we should do this via #454. Deprecating instances that way would not invoke many of the objections to that proposal, such as decreased performance.

@adamgundry
Copy link
Contributor

Thanks for writing this up! As the author of #454, I think this proposal is a good alternative to that one. It's simpler and gets most of the remaining value. In particular, I believe this should work:

type WarnInteger :: Type -> Constraint
type family WarnInteger a where
  WarnInteger Integer = UnboundedMemoryWarning
  WarnInteger a       = ()

class UnboundedMemoryWarning
instance UnboundedMemoryWarning {-# WARNING [x-decode-integer] "Integer may require unbounded memory!" #-}

It isn't possible to use type information to construct the text of the warning message or category, unlike #454, but we don't have any really compelling need for that AFAIK.

In Section 3.1, there is a very minor interaction with #541 which changed the syntax of WARNING pragmas to include an optional category. That shouldn't matter for this proposal though, as it doesn't depend on the specific syntax that maybemodwarning accepts.

@angerman
Copy link
Contributor

Strong agree that we need this. But I wonder whether we should do this via #454. Deprecating instances that way would not invoke many of the objections to that proposal, such as decreased performance.

@tomjaguarpaw, just to make sure I understand what you are saying here, you suggest to go via #454 instead, as for this specific case of instance deprecation, the objections to #454 with respect to performance do not apply?

I like the idea in #454, but it appears to me that it would introduce completely separate syntax (however potentially more powerful), for deprecations. At which point I wonder if #454 should just subsume all current applications of deprecation pragmas? It also seems more heavyweight than this. So maybe we can go with this lightweight proposal, and consider improving the general deprecation pragmas with a more generalised solution in #454 (though @adamgundry appears to think that this might not be necessary?). That might then requires us to be able to deprecate deprecation pragmas 🙈

@simonpj
Copy link
Contributor

simonpj commented Jan 27, 2023

I see the intent, but why not use the exact same mechanism as Unsatisfiable? That is:

instance Deprecated "Don't use Eq T" => Eq T where ...

where Deprecated behaves like Unsatisfiable but is non-fatal.

We don't really want two very similar features with entirely different designs, do we?

@angerman
Copy link
Contributor

angerman commented Jan 27, 2023

Am I right in assuming the Unsatisfiable proposal #433 has not been implemented yet? Maybe I'm misreading ghc/ghc#20835. I've tried to play with this as it indeed looks like a good solution to what we want (except for the -fdefer-type-errors; we almost certainly don't want Deprecations to be deferred until runtime), but couldn't figure out how.

@tomjaguarpaw
Copy link
Contributor

tomjaguarpaw commented Jan 27, 2023

To be clear, I think this proposal is essential and urgent, and I'm not too bothered by the UI that we choose to expose for it.


To bikeshed, I like the idea of features implemented "in-band" at the language level as alternatives to "out-of-band" ad hoc syntax. That's why I liked #454. I recall there were some objections to it regarding the cost of picking up a dictionary at run time. Since this proposal is about classes anyway, that objection wouldn't apply.

However, if #454's author Adam prefers this proposal, perhaps I'm off base. Yet Simon seems to be thinking the same as me.

Anyway, I don't mind. Let's get this done. It's essential.

@adamgundry
Copy link
Contributor

@angerman Indeed #433 has not yet been implemented. (I'd love someone to pick it up!)

@simonpj You're essentially suggesting #454. 😄 The pragma-based approach suggested here is closer to the existing deprecation/warning pragmas; the class-based approach is closer to Unsatisfiable. But for the foreseeable future we won't be able to use the class-based approach for deprecating modules or types, and there are performance concerns about using it for deprecating functions. Thus I think we will need to stick with pragmas for some cases, and so I'm tempted to stick with them in all cases, rather than having completely different mechanisms for module/type/function deprecation and for instance deprecation.

Ultimately I think either approach is reasonable, and I'm happy with whichever approach ends up being taken.

@angerman
Copy link
Contributor

angerman commented Jan 27, 2023

I fully agree with @tomjaguarpaw, and even if this is just a stop-gap measure to improve the overall ecosystem's ability to properly deprecate instance methods (and it follows the exact same pattern we have for existing deprecations instead of a different syntax). This seems even fairly lightweight? Maybe we can make a case if it turns out to be as lightweight as assumed, to be in 9.6.x to assist with deprecation (and migration) needs. The ticket that spawned this has started in 2016.

Edit: clarification, I did not mean 9.6.2. I should have been more explicit with that, instead of writing 9.6.x I was more thinking of a back port into 9.6.4 or some subsequent release, to specifically allow for proper deprecation, and thus make migration and compat with subsequent release (9.8, ...) easier. The whole motivation behind this is to have the ability to inform downstream consumers of deprecations, so they can plan ahead.

@phadej
Copy link
Contributor

phadej commented Jan 27, 2023

to be as lightweight as assumed, to be in 9.6.x

No. simply no. People have already started on working on compat for GHC-9.6.

We have been "sold" a six month GHC release cadence so we don't need to hastily try to get features in, for the price of more GHCs to test-with / migrate to etc. IMO the promise is still to be delivered, but let's not make it impossible resulting in the worst of both: unpredicatable yet frequent release cadence.

@angerman
Copy link
Contributor

angerman commented Jan 27, 2023

to be as lightweight as assumed, to be in 9.6.x

No. simply no. People have already started on working on compat for GHC-9.6.

We have been "sold" a six month GHC release cadence so we don't need to hastily try to get features in, for the price of more GHCs to test-with / migrate to etc. IMO the promise is still to be delivered, but let's not make it impossible resulting in the worst of both: unpredicatable yet frequent release cadence.

Three things:

  • I'm perfectly fine with this being in 9.8.
  • This tries to explicitly address the the "compat" / migrate / ... part in general.
  • This should in no way impact existing code, nor surface language nor anything and be purely additional.

The 9.6.x suggestion was exclusively to help packages like deepseq be able to deprecate instances.

@phadej
Copy link
Contributor

phadej commented Jan 27, 2023

It will impact GHC folks to deliver GHC-9.6 on schedule.

This should in no way impact anything and be purely additional.

Famous last words.

@int-index
Copy link
Contributor Author

The feature window for 9.6 is closed, and this proposal is not a bugfix, it's a new user-facing feature. Not only that, it's a proposal for a feature: not accepted yet, not implemented yet. We are not in a position to discuss its addition to 9.6

@simonpj
Copy link
Contributor

simonpj commented Jan 27, 2023

@simonpj You're essentially suggesting #454. 😄 The pragma-based approach suggested here is closer to the existing deprecation/warning pragmas; the class-based approach is closer to Unsatisfiable.

OK fine. My point is simply this: we should either

  • Use Unsatisfaiable and Deprecated, or
  • Use pragmas, and abandon Unsatisfiable and Deprecated

I'm uncomfortable about doing both. I hear a note of urgency, but it's a bad idea to put in a feature (pragmas) that we are going to deprecated in the forseeable future.

Let's just decide what we want and do that.

@int-index
Copy link
Contributor Author

@simonpj I disagree. Unsatisfiable is a constraint because it affects program semantics. We accept or reject a different set of programs, and the evidence for Unsatisfiable allows to omit implementations of instance methods.

{-# DEPRECATED ... #-} is a warning. It only affects GHC's stderr output, but not the build artifacts or the set of accepted programs. So it makes sense as a pragma.

@simonpj
Copy link
Contributor

simonpj commented Jan 27, 2023

the evidence for Unsatisfiable allows to omit implementations of instance methods

Interesting. I didn't know that. (Side issue, but I'd be interested in an example.)

It's true that one affects semantics and the other does not, but that doesn't mean that their designs must look so different. All the motivations for Unsatisfiable apply equally to Deprecated: see this section

@phadej
Copy link
Contributor

phadej commented Jan 27, 2023

All the motivations for Unsatisfiable apply equally to Deprecated: see this section

That is a proposal about custom type warnings constraint. Not about Unsatisfiable.

@phadej
Copy link
Contributor

phadej commented Jan 27, 2023

But @simonpj is right if #454 is accepted and implemented, it subsumes most the motivation for this proposal. At least the trigger motivation point (the deepseq's NFData (a -> b) instance) can be warned specifically about (as it doesn't do what you might expect), even if maintainers decide to not remove it at the end.

@int-index
Copy link
Contributor Author

It's true that one affects semantics and the other does not, but that doesn't mean that their designs must look so different.

I think it means exactly that. When I see a constraint, I know that it will be desugared into a dictionary and it will be carried around at runtime unless specialized, and it may or may not be solved. This is something I would expect from Unsatisfiable, too, which is the constraint counterpart of Void. We have the uninhabited data type and the uninhabited constraint — wonderful.

And I want none of that when I reason about deprecations and warnings, which should be strictly limited to messages that GHC emits. The pragma syntax is a strong indicator of that. And it's not a new pragma, I'm just extending the existing practice, for which I cited over 3 thousand uses on Hackage.

Our users, library authors, know how to use deprecation pragmas. Let them use the familiar feature in instances.

@Bodigrim
Copy link

Bodigrim commented Jan 28, 2023

OK fine. My point is simply this: we should either

* Use `Unsatisfaiable` and `Deprecated`, or

* Use pragmas, and abandon `Unsatisfiable` and `Deprecated`

I'm uncomfortable about doing both. I hear a note of urgency, but it's a bad idea to put in a feature (pragmas) that we are going to deprecated in the forseeable future.

Are you seriously suggesting that {-# DEPRECATED #-} pragmas might be "deprecated in the foreseeable future"? That would be quite a breakage for no good reason. If that's an intention behind introduction of Unsatisfiable / Deprecated constraints, please don't implement them as it will do more harm than good, anthithetical to their purpose to reduce breakage.

@JakobBruenker
Copy link
Contributor

As additional motivation, see my comment here: haskell/core-libraries-committee#86 (comment)

The context is that we want to restrict the type of Data.NonEmpty.Unzip from Functor f => f (a, b) -> (f a, f b) to NonEmpty (a, b) -> (NonEmpty a, NonEmpty b). But that means we ideally also want a deprecation period where only the general use is marked as deprecated. With this proposal, you could presumably accomplish this by writing

unzip :: UnzipDeprecation f => Functor f => f (a,b) -> (f a, f b)
unzip xs = (fst <$> xs, snd <$> xs)

class UnzipDeprecation f

instance UnzipDeprecation f {-# DEPRECATED "..." #-}

instance {-# OVERLAPPING #-} UnzipDeprecation NonEmpty

@tomjaguarpaw
Copy link
Contributor

instance {-# INCOHERENT #-} UnzipDeprecation NonEmpty

seems to compile fine. Is there a problem with that? I agree that uses of overlapping instances are rarely good, but this case, where we're not actually trying to pick up a type class implementation, seems fine.

@hsyl20
Copy link
Contributor

hsyl20 commented Apr 6, 2023

@int-index It looks like the proposal is ready to be submitted to the committee, isn't it?

We are interested in this feature and you may add to the implementation plan that @barci2 will implement it as part of his internship at IOG.

@int-index
Copy link
Contributor Author

@nomeata Let's submit this.

@nomeata nomeata changed the title Deprecated instances Deprecated instances (under review) Apr 6, 2023
@nomeata nomeata added the Pending shepherd recommendation The shepherd needs to evaluate the proposal and make a recommendataion label Apr 6, 2023
@angerman angerman added Pending committee review The committee needs to evaluate the proposal and make a decision and removed Pending shepherd recommendation The shepherd needs to evaluate the proposal and make a recommendataion labels May 17, 2023
@angerman
Copy link
Contributor

I'm recommending acceptance of this proposal. All concerns have been sufficiently considered as far as I can see.

@simonpj
Copy link
Contributor

simonpj commented May 24, 2023

I'm content to accept this. One niggle:

inst_decl
    : 'instance' overlap_pragma inst_type maybewarning where_inst

Why put the overlap_pragma prefix but the maybewarning (also a pragma) postfix? That seem very odd. Why not:

       : 'instance' instance_pragmas inst_type where_inst

instance_pragmas ::= empty | instance_pragma instance_pragmas
instance_pragma ::= maybewarning | overlap_pragma

Now I can put them in any order.

@barci2
Copy link

barci2 commented May 24, 2023

I agree with @simonpj, it would be better if the pragmas could be placed in any order, although I'd rather do it something like this:
instance_pragmas ::= maybewarning overlap_pragma | overlap_pragma maybewarning
which does not allow for duplicate pragmas

@int-index
Copy link
Contributor Author

It's postfix for consistency with modules where we write module M {-# DEPRECATED ... #-} where and not module {-# DEPRECATED ... #-} M where. The deprecation pragma comes after the name of the deprecated module. I applied the same principle to instances, so that the deprecation pragma comes after the instance head of the deprecated instance.

Perhaps consistency is not important here, or maybe we could allow the pragmas to come both in prefix and postfix positions. I only tried to follow the established precedent. Do we have a good reason to deviate from it?

@simonpj
Copy link
Contributor

simonpj commented May 24, 2023

Do we have a good reason to deviate from it?

The good reason is that the other pragmas are prefix. I have no objection to making all of them either prefix or postfix or both; the only thing I dilike is making one prefix and the other postfix.

I'm only one person. I'd love opinions from others .

@barci2
Copy link

barci2 commented May 24, 2023

Well now deprecated exports are prefix, so you can make an argument the other way around with those as well

@barci2
Copy link

barci2 commented May 25, 2023

@int-index what's the consensus then, do we do it like with export deprecation or like with module deprecation?
Also, since deriving instances is a thing, do we want to also allow deprecating that, since it also allows overlapping instances pragma?
Also, do we want to allow deprecating instances inside of TH?

@simonpj
Copy link
Contributor

simonpj commented May 25, 2023

Also, since deriving instances is a thing, do we want to also allow deprecating that, since it also allows overlapping instances pragma?

Aha, that is an excellent point. Easily fixed, but it would be odd not to make it consistent.

@barci2
Copy link

barci2 commented Jun 1, 2023

I've asked Eric about potential solutions to his concerns in https://mail.haskell.org/pipermail/ghc-steering-committee/2023-May/003240.html; @int-index what's your stance on including standalone deriving and changes to the placement of pragmas in the proposal?

@aspiwack
Copy link
Contributor

aspiwack commented Jun 6, 2023

On the syntax I have an opinion (these are easy to come by, of course, and hard to sort). My brain fully expects the DEPRECATED pragma to be prefix in instance declarations. I even find the post-fix proposal jarring.

@angerman
Copy link
Contributor

On the syntax I have an opinion (these are easy to come by, of course, and hard to sort). My brain fully expects the DEPRECATED pragma to be prefix in instance declarations. I even find the post-fix proposal jarring.

While I think my brain seems to work similar to yours, I value consistency more. It makes it much less surprising to find it one way here and another there. Maybe prefix deprecations should be a separate proposal? Which would then also need deprecation warnings ("move your deprecation annotation to the front please").

Lumping moving deprecations into prefix position everywhere in this proposal feel a lot like scope creep.
Having prefix and postfix deprecations doesn't seem ideal either (even though we have this with prefix and postfix qualified as well).

My stance therefore is:

  1. Prefer consistency (postfix)
  2. Allow choice (prefix + postfix), but for all deprecations including modules, ... (again at least make it consistent).

Optionally have a separate proposal for prefix deprecations.

@int-index
Copy link
Contributor Author

I updated the proposal based on the recent discussion.

  • Simplified the specification of semantics using @adamgundry's idea (thanks!). No more mention of Unsatisfiable.
  • Allowed deprecation of instances produced by standalone deriving.
  • Moved the pragma to a prefix position, as several people expressed this as their preference.

As to permitting instance pragmas in arbitrary order (i.e. swapping {-# DEPRECATED ... #-} and {-# OVERLAPPING #-}) or allowing both prefix and postfix pragmas, I think it's a fine idea but unnecessary. If someone wants to propose this, be my guest, but for now I moved this to the Alternatives section and will not discuss this any further.

@angerman I'd like to resubmit the updated proposal to the committee.

@nomeata nomeata merged commit 23642df into ghc-proposals:master Jun 28, 2023
@nomeata nomeata changed the title Deprecated instances (under review) Deprecated instances Jun 28, 2023
@nomeata nomeata added Accepted The committee has decided to accept the proposal and removed Pending committee review The committee needs to evaluate the proposal and make a decision labels Jun 28, 2023
@noughtmare
Copy link
Contributor

@nomeata this has now been implemented in master and will probably land in 9.10.

@nomeata nomeata added the Implemented The proposal has been implemented and has hit GHC master label Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted The committee has decided to accept the proposal Implemented The proposal has been implemented and has hit GHC master
Development

Successfully merging this pull request may close these issues.

None yet