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

Control what sort of entity a deprecated pragma applies to #167

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@nineonine
Copy link

nineonine commented Sep 11, 2018

This proposal would allow to specify which entity (type or data constructor) you want to deprecate.

rendered
Trac ticket


This will not work (parse error): ::

{-# DEPRECATED type Qux, constructor Quux "Don't use this" #-}

This comment has been minimized.

@gridaphobe

gridaphobe Sep 11, 2018

Contributor

Should the constructor here be data instead?

This comment has been minimized.

@nineonine

nineonine Sep 11, 2018

Author

That is correct. Thank you! Fixed.


This will not work (parse error): ::

{-# DEPRECATED type Qux, constructor Quux "Don't use this" #-}

This comment has been minimized.

@gridaphobe

gridaphobe Sep 11, 2018

Contributor

I think it would be nicer to have the type/data qualifier apply only to the immediately-following name, such that the second example would be accepted. But I also haven't had occasion to put more than one name in a DEPRECATE pragma yet, so I might be wrong about the common case.

@osa1

This comment has been minimized.

Copy link
Contributor

osa1 commented Sep 11, 2018

I think data is better than constructor for the reason mentioned in the proposal, and it's also shorter.

Not against this proposal, but I'm wondering if we could reuse haddock syntax for this. For example, in Haddock syntax we can annotate constructor or the type without ambiguity:

-- | Type documentation
data Foo =
    -- | Constructor documentation
    Foo x

so why not something like

-- | DEPRECATE: This type is deprecated
data Foo =
    -- | DEPRECATE: This constructor is deprecated
    Foo x

Perhaps as a solution to this problem we can ask people to use haddock syntax, and then deprecate the {-# DEPRECATED ... #-} syntax.

Just an idea.

@simonpj

This comment has been minimized.

Copy link
Contributor

simonpj commented Sep 11, 2018

I note that a module deprecation is positional. One says (I think)

module M {-# DEPRECATED "blah" #-} where ...

One could do the same for data constructors, thus

data Baz = Baz {-# DEPRECATED "blah" #-}
               | Boo
               | Bim {-# DEPRECATED "blah" #-}
                        Int

or in GADT syntax

data Baz where
   Baz :: Int -> Baz
   Boo :: Baz
   Bim :: Baz
   {-# DEPRECATED Bim, Baz "blah" #-}

This positional story works well when the deprecation is attached to the definition of the thing. If you want to import something, deprecate it, and re-export it, it would not work so well. But (I think) we can't do that anyway today.

@RyanGlScott

This comment has been minimized.

Copy link
Contributor

RyanGlScott commented Sep 13, 2018

After reading this proposal, it seems like this is limited in scope to disambiguating prefix type constructors from prefix data constructors. But what about the following scenario?

module A where

type a <+> b = ...

(<+>) :: Int -> Int -> Int
a <+> b = ...

{-# DEPRECATED (<+>) "Don't use (<+>)" #-}

It's unclear which (<+>) I'm referring to, so I want to pick the value-level (<+>) in particular. But how do I say so with this proposal? Certainly not with the type specifier. That would suggest that data is the specifier to use, but that also feels a little awkward, since (<+>) isn't a data constructor.

The reason I'm picking this nit is because there is a separate, previously accepted proposal (Require namespacing fixity declarations for type names and WARNING/DEPRECATED pragmas) that also adds specifiers in a similar fashion to this proposal. However, it proposes adding specifiers named type and value (instead of type and data), since value is a more inclusive description that can apply to more entities than data.

Sorry for continuing to bikeshed about this name even more, but at the very least I'd like this proposal to be consistent with the other proposal (even if we end up changing the other proposal to use data over value).

@nineonine

This comment has been minimized.

Copy link
Author

nineonine commented Sep 14, 2018

Thank you for all your comments!
@RyanGlScott this is a great example! value keyword specifier definitely sounds more inclusive than data. I tried to think about an edge case where we would need to distinguish between data constructor and a function (with something like PatternSynonyms perhaps) but wasn't able to come up with example - it looks like it is restricted at syntax level so it is probably safe to have 1 disambiguating specifier for both these namespaces. Unless someone has something else in mind, I am going to update the proposal accordingly.

@nineonine nineonine force-pushed the nineonine:depr-entities branch from 8140791 to 1948441 Sep 17, 2018

@nineonine

This comment has been minimized.

Copy link
Author

nineonine commented Sep 24, 2018

pinging @nomeata - I think this is ready to be reviewed.

@nomeata

This comment has been minimized.

Copy link
Contributor

nomeata commented Sep 30, 2018

Sorry for the delay, ICFP…

Before brining this before the committee, I’d like to raise one point: We already added a way to distinguish between the type constructor and the data constructor in export lists, and the ExplicitNamespaces language extensions uses type and pattern (also see the section on PatternSynonyms.

While it is a bit strange to call a data constructor pattern, I believe we should be consistent here.

In fact, I would be open to just extending ExplicitNamspaces with this feature, rather than adding a new language extension here.

When specifying several entities in one pragma, the sort of deprecated entity we've specified will apply to all listed entities. This will work - warn when these types are used(but not their constructors):

What is the rationale for this? I would expect that type Foo, Bar is the same as Bar, type Foo. Also, again the similarity to export lists calls for this.

@nineonine

This comment has been minimized.

Copy link
Author

nineonine commented Oct 2, 2018

@nomeata thanks for the comments.

I do agree that pattern looks somewhat strange in that case. But as long as everyone agrees on it, I will gladly change it.

When specifying several entities in one pragma, the sort of deprecated entity we've specified will apply to all listed entities. This will work - warn when these types are used(but not their constructors):

This is coming from the implementation that I already have in mind. I just added that bit to the proposal trying to cover all the possible corner cases. Frankly speaking, I am not sure whether this feature is even widely used (specifying several entities in one pragma). So this is just an implementation side-effect, so to speak, which I included in the proposal for the sake of completeness.

@nomeata

This comment has been minimized.

Copy link
Contributor

nomeata commented Oct 3, 2018

So this is just an implementation side-effect…

Even if that was the easiest way to implement it, and may few people use this feature, we should still strive for consistency with the rest of the language. And if in export lists (where we already have such qualifiers) they bind more tightly than commas, then I think they should do that here as well.

@nineonine

This comment has been minimized.

Copy link
Author

nineonine commented Oct 12, 2018

we should still strive for consistency with the rest of the language

@nomeata for sure, this is a good point and I wholeheartedly agree!
Updating the proposal accordingly.

@bravit bravit added the Proposal label Nov 11, 2018

@bravit bravit removed the Proposal label Dec 3, 2018

@bgamari

This comment has been minimized.

Copy link
Contributor

bgamari commented Jan 21, 2019

What is the status of this?

@nineonine

This comment has been minimized.

Copy link
Author

nineonine commented Jan 21, 2019

@bgamari proposal is updated according to the comments. I guess it is waiting to be reviewed by the committee ..

@bgamari

This comment has been minimized.

Copy link
Contributor

bgamari commented Jan 21, 2019

Cool, @nomeata, it sounds like this needs a shepard.

nomeata added some commits Jan 21, 2019

@bravit

This comment has been minimized.

Copy link
Contributor

bravit commented Jan 22, 2019

@nineonine, could you please mention the idea of positional DEPRECATED pragmas as described here by Simon in the list of alternatives?

@bravit

This comment has been minimized.

Copy link
Contributor

bravit commented Jan 22, 2019

Before I am ready to make a recommendation, are there any thoughts (pro or contra) about moving to positional DEPRECATED pragmas instead? In fact, I think that this approach is better as it enables more fine-grained control over deprecated entities and we don't have to use pattern and other words. By the way, what about deprecating instances (see also this feature request and this note on deprecation mechanisms)?

@nineonine

This comment has been minimized.

Copy link
Author

nineonine commented Jan 26, 2019

Added positional deprecation idea.
I think this idea would work equally well and I cannot think of any bad consequences of going that way right now.
Deprecating class instances sounds like a great idea too, I added this to "unresolved questions". I wonder what others think about that..

@bravit

This comment has been minimized.

Copy link
Contributor

bravit commented Jan 26, 2019

@nineonine will you be able to rewrite/reimplement the proposal if the Committee decides in favor of positional pragmas?

@nineonine

This comment has been minimized.

Copy link
Author

nineonine commented Jan 26, 2019

@bravit sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment