Implement feature #3427
Needs RevisionPublic

Authored by nineonine on Sep 4 2018, 1:38 AM.

Details

Reviewers
bgamari
osa1
Trac Issues
#3427
Summary
  • added new reserved word ('constructor')
  • added new datatype DeprEntity to distinguish between different deprecated entities
  • WarningTxt type, namely DeprecatedTxt constructor was extended with a field of type DeprEntity
  • during the renaming phase, in warnIfDeprecated we do extra check for the deprecated entity
  • check is performed against DeprEntity and Namespace

had to make changes in submodules:

  • utils/haddock - DeprecatedTxt is used by parseWarning function
  • utils/hsc2hs - CrossCodegen.hs had some binding with a name 'constructor'

few notes:

  • right now the implementation feels a bit hacky - we are abusing the type (which is only responsible for the textual representation of deprecation) to carry some extra information around. Perhaps there is a better place/way or maybe WarningTxt needs just to be renamed?
  • I am not sure I am happy with the entitySpecified naming. It doesn't feel right.
  • if we specify deprecated entity for something non-type or non-constructor(for example, a function), ghc will ignore it and warn anyway
  • 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):

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

this will not work:

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

./validate

nineonine created this revision.Sep 4 2018, 1:38 AM
nineonine added a comment.EditedSep 4 2018, 1:41 AM

had to make changes in submodules:

  • utils/haddock - DeprecatedTxt is used by parseWarning function
  • utils/hsc2hs - CrossCodegen.hs had some binding with a name 'constructor'

few notes:

  • right now the implementation feels a bit hacky - we are abusing the type (which is only responsible for the textual representation of deprecation) to carry some extra information around. Perhaps there is a better place/way or maybe WarningTxt needs just to be renamed?
  • I am not sure I am happy with the entitySpecified naming. It doesn't feel right.
  • if we specify deprecated entity for something non-type or non-constructor(for example, a function), ghc will ignore it and warn anyway
  • 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):

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

this will not work:

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

I've read the comment by Simon about disambiguation in export lists and pattern and I am not sure I understood it - sorry about that, I am a newcomer and a total newbie in this game :-)

nineonine updated the Trac tickets for this revision.Sep 5 2018, 4:34 PM

It looks like I would need some push privileges to update submodule.

osa1 requested changes to this revision.EditedSep 6 2018, 2:07 AM
osa1 added a subscriber: osa1.

Thanks for doing this! I haven't reviewed this in detail but you can't make constructor a keyword as it's currently a valid identifier and making it a keyword breaks programs that use it as an identifier. In fact, I can't even boot GHC with this patch as some of the ghc libraries use constructor as an identifier.

compiler/parser/Lexer.x
807 ↗(On Diff #17893)

Not a good idea! See my comment.

This revision now requires changes to proceed.Sep 6 2018, 2:07 AM
nineonine added inline comments.
compiler/parser/Lexer.x
807 ↗(On Diff #17893)

Thanks for the review!

Yes, I already thought about it. Should we just change it to something else? Although my intuition is telling me that a comment by @simonpj on trac about disambiguation in export lists could be the solution here. Unfortunately, I haven't understood yet how it would work here. Some explanation would be greatly appreciated!

osa1 added inline comments.Sep 6 2018, 2:51 AM
compiler/parser/Lexer.x
807 ↗(On Diff #17893)

Right, I think we should be able to make "constructor" special only in the relevant pragma. I don't know off the top of my head how to do this though. I'll need to take a look at the lexer/parser code. IIUC Simon says we have a similar case in disambiguation in export lists (I don't know what that is though) so perhaps we can take a look there..

nineonine edited the summary of this revision. (Show Details)Sep 6 2018, 2:28 PM
nineonine added inline comments.Sep 6 2018, 5:35 PM
compiler/parser/Lexer.x
807 ↗(On Diff #17893)

@osa1 I found a proposal for disambiguation in export lists. It suggests to use the disambiguating specifier data, type, and class. Should I go ahead and change constructor to data?

osa1 added inline comments.Sep 7 2018, 1:27 AM
compiler/parser/Lexer.x
807 ↗(On Diff #17893)

I think either one is fine. Btw, unless there is already a proposal for this, this may have to go through the GHC proposal process. That's also a good way to get feedback about the syntax. This is backwards compatible and many people needed it before so I suspect it'll be an easy accept with a little bit of discussion about the syntax (constructor or data etc.).

Great stuff. See comment:22 on the ticket.

nineonine updated this revision to Diff 17947.Sep 8 2018, 1:48 PM
nineonine edited the summary of this revision. (Show Details)
  • Change qualifier name to data

This appears to be blocked on GHC Proposal #167. @nineonine, when the proposal is accepted could you move this to GitLab for review?

bgamari requested changes to this revision.Jan 20 2019, 6:17 PM
This revision now requires changes to proceed.Jan 20 2019, 6:17 PM