Move language extension enumeration to `ghc-boot` and expose enabled language extensions to TH
ClosedPublic

Authored by bgamari on Sep 1 2015, 4:32 PM.

Details

Summary

This exposes template-haskell functions for querying the language extensions which are enabled when compiling a module,

  • an isExtEnabled function to check whether an extension is enabled
  • an extsEnabled function to obtain a full list of enabled extensions

To avoid code duplication this adds a GHC.LanguageExtensions module to ghc-boot and moves DynFlags.ExtensionFlag into it. A happy consequence of this is that the ungainly DynFlags lost around 500 lines. Moreover, flags corresponding to language extensions are now clearly distinguished from other flags due to the LangExt.* prefix.

This fixes Trac #10820.

Test Plan

validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
spinda retitled this revision from to Expose enabled language extensions to TH (#10820).Sep 1 2015, 4:32 PM
spinda updated this object.
spinda edited the test plan for this revision. (Show Details)
spinda added reviewers: austin, goldfire.
spinda updated the Trac tickets for this revision.
spinda added a comment.Sep 1 2015, 4:45 PM

I don't like adding another datatype for supported extensions, but it seems to be the only way to do this (see the discussion on Trac #10820). If there's another way, please say so.

This doesn't quite hew to the specification in the ticket. That spec included an extsEnabled :: Q [Extension] operation. Exposing the ability to get the full list of enabled extensions to clients is more powerful than just allowing a query interface. If a client wants to check a certain extension, it's trivial to do so. (TH could provide a function to do this directly, for convenience.)

As discussed below, I think there may be a way to avoid the new big datatype. Perhaps post to ghc-devs and see if there's a reason we can't do this? Or if anyone has a better idea.

In any case, many thanks for just jumping in here!

compiler/main/DynFlags.hs
561–562

See comments below. I think this Read instance should be removed.

compiler/typecheck/TcSplice.hs
1622

Use expectJust here.

1625

This is painful and untyped. I know it's annoying to convert between the large types manually, but it's much more robust to future changes. I'm still not 100% convinced that we need a new datatype here. Maybe Cabal and ghc can depend on template-haskell. But having The Datatype Of All Extensions in Template Haskell is terrible. But maybe it's the best that we can do.

In any case, using show and read to convert is fragile. For example, a spelling error in one of the datatypes would not be caught and would cause misbehavior. Just use an obnoxiously long and boring case.

docs/users_guide/7.12.1-notes.xml
191 ↗(On Diff #4009)

Yay. Thanks for adding a release note!

libraries/template-haskell/Language/Haskell/TH/Syntax.hs
1707

Remove this line (copy/pasted from DynFlags?).

spinda updated this revision to Diff 4012.Sep 2 2015, 2:45 AM
spinda marked 4 inline comments as done.

Add extsEnabled, get rid of Read instance

spinda added a comment.Sep 2 2015, 2:46 AM

@goldfire

Thank you for looking over this!

I initially left out extsEnabled because it turns out to be a more complicated operation than I originally thought, due to the way DynFlags is structured (storing ExtensionFlags in Int form in an IntSet). For checking if a single extension is enabled, I think it's better to expose the efficient xopt method directly than to make isExtEnabled a shortcut built on extsEnabled, as the latter requires a full conversion to list from the IntSet. But I've added in extsEnabled in my update to the patch.

re: avoiding duplicating the list of extensions all over the place, I'll make that post to ghc-devs and see what comes of it. It would be nice to have one shared data structure for this, but I don't know where one would put it, except in template-haskell, which... isn't ideal, but like you say, may be the best thing one can do here.

compiler/typecheck/TcSplice.hs
1622

Ah, thank you. I knew there had to be some function that did that somewhere, but couldn't find one.

1625

Aw, but that's another place the entire extension list has to be duplicated... Alright, I'll stop trying to be clever about it and bite the bullet here.

spinda updated this object.Sep 2 2015, 3:29 AM
goldfire added a reviewer: hvr.Sep 3 2015, 1:45 AM
goldfire added subscribers: simonpj, hvr.

@simonpj suggests (on ghc-devs) bin-package-db as a good home for the extensions type. Though we should probably rename that package. Including @hvr as he seems to know a bunch about the boot libraries and such.

spinda added a comment.Sep 3 2015, 1:58 AM

Question is, what to name it, and what should its scope be? A general package for shared code (ghc-common or ghc-shared, maybe...), or something more narrow?

I'd be happy with ghc-common or ghc-shared, with a specification something like:

"This package contains types and functions that are used both by GHC itself and by other tools in the GHC eco-system, such as cabal, ... [complete this list]".

hvr added a comment.Sep 3 2015, 11:23 AM

"This package contains types and functions that are used both by GHC itself and by other tools in the GHC eco-system, such as cabal, ... [complete this list]".

I'm not sure though if Cabal can benefit from the Extension type, as Cabal currently keeps track of a superset of language extensions (e.g. past obsolete ones as well as non-GHC extensions) as well as being compile-compatible against a large range of GHC versions.

In D1200#33691, @hvr wrote:

I'm not sure though if Cabal can benefit from the Extension type, as Cabal currently keeps track of a superset of language extensions (e.g. past obsolete ones as well as non-GHC extensions) as well as being compile-compatible against a large range of GHC versions.

Yes, good point. But one declaration shared between TH and GHC is still quite worth it.

hvr added a comment.Sep 4 2015, 8:03 AM

Yes, good point. But one declaration shared between TH and GHC is still quite worth it.

Indeed, as TH is tied to GHC versions anyway...

It looks like the plan here is to re-purpose and rename the existing bin-package-db package to put the shared datatype. I concretely propose ghc-common as the new name for the package. ghc and template-haskell can then both depend on ghc-common.

Sound like a plan? @spinda, are you still planning on working on this? Many thanks for your contributions!

bgamari requested changes to this revision.Sep 21 2015, 5:32 AM

@spinda, do you think you continue this work?

This revision now requires changes to proceed.Sep 21 2015, 5:32 AM

FYI: D1313 renamed bin-package-db to ghc-boot, which would provide an ideal location for Extension.

bgamari commandeered this revision.Dec 7 2015, 8:49 AM
bgamari edited reviewers, added: spinda; removed: bgamari.

I'm going to quickly finish this up.

bgamari updated this revision to Diff 5525.Dec 7 2015, 8:50 AM

Rebase and place Extension in ghc-boot

bgamari updated this revision to Diff 5529.Dec 7 2015, 10:33 AM

Add missing file

bgamari updated this revision to Diff 5534.Dec 7 2015, 10:59 AM

Fix module name

bgamari updated this revision to Diff 5545.Dec 7 2015, 2:01 PM

Add missing import to test

bgamari updated this revision to Diff 5560.Dec 8 2015, 4:32 AM

Rebase?

bgamari updated this revision to Diff 5634.Dec 12 2015, 9:44 AM
  • D1200
  • Replace ExtensionFlag with GHC.LanguageExtensions.Extension
bgamari retitled this revision from Expose enabled language extensions to TH (#10820) to Move language extension enumeration to `ghc-boot` and expose enabled language extensions to TH.Dec 12 2015, 9:49 AM
bgamari updated this object.
bgamari updated this object.

@goldfire, @simonpj, @austin: would any of you be opposed to moving ExtensionFlag like this?

bgamari updated this revision to Diff 5642.Dec 12 2015, 1:55 PM
bgamari marked 3 inline comments as done.

Fixes

goldfire accepted this revision.Dec 12 2015, 6:44 PM

I like it.

Thanks for picking this up.

libraries/template-haskell/Language/Haskell/TH/Syntax.hs
46

Re-export Extension here and from Language.Haskell.TH? After all, we tell people not to use ghc-boot directly.

bgamari updated this revision to Diff 5662.Dec 14 2015, 6:32 AM
  • It finally builds
bgamari added inline comments.Dec 14 2015, 6:43 AM
libraries/template-haskell/Language/Haskell/TH/Syntax.hs
46

Good call. That being said, I do worry a bit about cluttering the namespace (and Haddocks) with all of these constructors. Perhaps it would be preferable to reexport these in a dedicated module (e.g. Language.Haskell.TH.Extensions)?

@goldfire, @simonpj, @austin: would any of you be opposed to moving ExtensionFlag like this?

I'm ok with this.

Simon

goldfire added inline comments.Dec 14 2015, 8:37 AM
libraries/template-haskell/Language/Haskell/TH/Syntax.hs
46

This is a good point, but Language.Haskell.TH and .Syntax are already so cluttered. Would this make it substantively worse? I vote to keep it all bundled together, but am quite sympathetic to an opposing opinion. Do what you think is best.

bgamari updated this revision to Diff 5738.Dec 15 2015, 3:57 PM

Fix all the things

bgamari marked 3 inline comments as done.Dec 15 2015, 4:01 PM
bgamari updated this revision to Diff 5740.Dec 15 2015, 4:57 PM

It's done

This revision was automatically updated to reflect the committed changes.