Opened 4 years ago

Last modified 3 years ago

#10071 new feature request

Implement deprecation-warnings for class-methods to non-method transitions

Reported by: hvr Owned by:
Priority: normal Milestone:
Component: Compiler Version: 7.8.4
Keywords: AMP MonadFail deprecate warning Cc: ekmett, thoughtpolice, simonpj, oerjan, alanz
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect warning at compile-time Test Case:
Blocked By: Blocking:
Related Tickets: #8004, #4834, #4879, #2119 Differential Rev(s):
Wiki Page:

Change History (18)

comment:1 Changed 4 years ago by simonpj

What on earth is this ticket about? The first word is "This", presumably referring to something, but with no clue about what it is referring to. I am utterly lost.

A feature request (which I assume this is) needs a specification!

Simon

comment:2 Changed 4 years ago by hvr

Description: modified (diff)
Summary: Implement warnings for class-method overridingImplement deprecation-warnings for class-methods to non-method transitions

I hope this makes it more clear what I'm proposing. When I started writing the ticket I wasn't sure if I wanted a general facility, or something more adhoc for the specific AMP use-case, and so ended up with the incomplete description...

comment:3 Changed 4 years ago by simonpj

Better. Syntax: why not just do this for a nested DEPRECATED pragma, thus

class C a where
  foo :: a

  bar :: a -> a
  bar x = x
  {-# DEPRECATED bar "burble wurble" #-}

  doo :: a

comment:4 in reply to:  3 Changed 4 years ago by hvr

Replying to simonpj:

Better. Syntax: why not just do this for a nested DEPRECATED pragma, thus

Works too (I'm only slightly worried that it might be confusing for users that the indentation-level of the {-# DEPRECATED #-} changes its semantics, but I have no strong preference here)

comment:5 Changed 4 years ago by ekmett

The main purpose of such a feature is to give us the ability to smoothly remove redundant class members over a deprecation cycle. (Admittedly, perhaps, quite a long deprecation cycle in the case of something like return!)

e.g. (>>) could be reduced to a top level binding to (*>) after a period with a warning making it clear that this was coming.

Other examples of candidates are Traversable's sequence and sequenceA, although there is currently a technical barrier to removing mapM entirely. This would enable us to eventually reclaim sequence for use on Applicative data types.

Having the ability to do these things means we can put together future proposals for how to move AMP (or FTP) along with reduced breakage.

I confess to not being terribly concerned about the choice of syntax, though. =)

comment:6 Changed 3 years ago by oerjan

Cc: oerjan added

Just found this via a libraries archive discussion. I think there are subtle cases here, if a .. wildcard is used (possibly a fishy thing in itself). With the return example, this is fine:

import Control.Monad (Monad(..), return)

This is fine provided return is not used or re-exported, including implicitly:

import Control.Monad (Monad(..))

This is fine if the corresponding import is fine:

module Test (Monad(..), return) where

This is definitely wrong:

module Test (Monad(..)) where

comment:7 Changed 3 years ago by alanz

Cc: alanz added

comment:8 Changed 3 years ago by simonpj

Notes from conversation with Herbert and Simon PJ. Proposed data types:

data Warnings = ... | WarnSome [WarnItem]
data WarnItem = WI OccName WarnInfo (Located SourceText) [Located (SourceText,FastString)]
data WarnInfo = Warning | DeprecTop | DeprecMeth

The new bit is the DeprecMeth.

Need updated ticket description, or wiki page, to give examples of the different behaviour.

comment:9 Changed 3 years ago by hvr

Description: modified (diff)
Keywords: MonadFail added

comment:10 Changed 3 years ago by simonpj

I have attempted a precise specification, in Design/MethodDeprecations. See if you agree with it.

What about this? (Using the main example in Design/MethodDeprecations

module M where
import X
import M1 ( C(..) )
x = bar ()

Do we get a warning here? You might say that since X doesn't export bar (which is not obvious) that the only way it can be in scope is via C(..).

I say yes, and I have articulated that in the wiki page. There is a design alternative described there too; I'm not sure what you want.

comment:11 in reply to:  10 Changed 3 years ago by hvr

Replying to simonpj:

I have attempted a precise specification, in Design/MethodDeprecations. See if you agree with it.

I think so. However, you added this code example:

module Use where
  import Def( C( bar ) )
  import Def( bar )
  x = bar ()

which should definitely be complained about (if bar is deprecated-as-method-of-C), as this code would break as soon as bar moved out of C for real.

What about this? (Using the main example in Design/MethodDeprecations

module M where
import X
import M1 ( C(..) )
x = bar ()

Do we get a warning here? You might say that since X doesn't export bar (which is not obvious) that the only way it can be in scope is via C(..).

I say yes, and I have articulated that in the wiki page. There is a design alternative described there too; I'm not sure what you want.

That's indeed a not so clear case, but I'd consider this a case where we need to warn, as the assumed use case for this new deprecation is that such methods will eventually be *moved* outside the class, but with the intent to remain exported (but not belonging to C anymore) from that very module that currently exports bar as method of C.

PS: ...and I forgot to mention that this feature should be implemented (and data types refactored) while keeping the related #4879 feature-request in mind

comment:12 Changed 3 years ago by simonpj

Ok, well, please go to the specification and modify it to your satisfaction.

Remember that the existing behaviour is that import and export lists never generate deprecations. I think you want them to do so. That's ok but the specification should say so, esp as it's different to the current behaviour.

comment:13 Changed 3 years ago by hvr

Keywords: deprecate warning added

(turns out there's yet another deprecation/warning related ticket...)

comment:14 Changed 3 years ago by oerjan

(fix typo in related ticket number)

comment:15 Changed 3 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:16 Changed 3 years ago by bgamari

See Design/DeprecationMechanisms for a summary of this and related deprecation proposals.

comment:17 Changed 3 years ago by thomie

Type of failure: None/UnknownIncorrect warning at compile-time

comment:18 Changed 3 years ago by thomie

Milestone: 8.0.1
Note: See TracTickets for help on using tickets.