[POC] Support qualified module renamings #7303

ezyang · 2021-02-24T22:58:05Z

Summary

Qualified module renamings let you bring all modules from a library into scope, but qualified under some module prefix.

Instead of something wordy like:

mixins: pkg (A as Foo.A, B as Foo.B, C as Foo.C, D as Foo.D) requires (Sig as Foo)

You can write this compactly as:

mixins: pkg qualified Foo requires (Sig as Foo)

Partially address #7290

Description

Today, using the 'mixins' field you can rename modules that come from other packages by manually expressing a renaming one-by-one. In some Backpack use cases, you may have a lot of modules that you would like to mechanically rename into some subnamespace; today, you have manually list each renaming one by one.

This PR contains an implementation of one possible way to extend mixin syntax to support qualified renaming; the implementation is very simple. The syntax here is based off of Richard Eisenberg's local modules proposal (ghc-proposals/ghc-proposals#283) which supports the qualified keyword before module exports/imports which has the same effect (bring the module into scope under a sub-module namespace). However, the PR isn't really meant to be an end all to the discussion: it's just to show that it's pretty simple to implement this functionality.

There are two primary axes which I am looking for feedback:

Expressivity. The current PoC implementation only permits unconditionally prefix-ing all modules that would have been brought into scope by the mixin; e.g., transforming module A to Prefix.A. Edward Kmett has expressed that in some cases, he would like it if you could implement the import as a suffix. One could also imagine allowing arbitrary string transformations. Opinions on where to draw the line for expressivity are solicited.
Syntax. The current syntax is "pkgname qualified Prefix" as it is symmetric with "pkgname hiding (A, B)" and it was simple to implement. But I am not particularly attached to this syntax, and am open to other suggestions. If we permit suffixing, a wildcard based syntax like "pkgname (* as *.Suffix)" may be preferable (though modestly more complex to specify and implement; for example, is the glob recursive over dots?). Edward Kmett has offered some other possibilities at Globbing support for mixins and rexported-modules #7290 (comment)

Please include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

Qualified module renamings let you bring all modules from a library into scope, but qualified under some module prefix. If you write 'pkg qualified Prefix', then if pkg exposes A and B, you will be able to access them as Prefix.A and Prefix.B. This functionality doesn't require any GHC changes; Cabal takes care of desugaring the qualified syntax into an explicit list of renamings. Partially address #7290 Signed-off-by: Edward Z. Yang <[email protected]>

ezyang · 2021-02-24T22:58:21Z

This probably needs a version guard somewhere

ezyang · 2021-02-24T22:58:32Z

And it also needs docs

ekmett · 2021-02-24T23:09:09Z

LGTM. I'd like a more general globbing solution, but won't let the perfect be the enemy of 'hey we could ship this and improve the situation'

ezyang · 2021-02-25T04:16:22Z

@phadej I'm not exactly sure how I'm supposed to operate the Described regex machinery.

Currently, IncludeRenaming is specified as:

instance Described IncludeRenaming where
    describe _ = mr <> REOpt (RESpaces <> "requires" <> RESpaces1 <> mr)
      where
        mr = describe (Proxy :: Proxy ModuleRenaming)

In particular, the space before requires is optional. This is currently unambiguous because prior to this PR mr never produces a token that would be ambiguous if cat'ed with requires. However, with this PR, an unparenthesized module name is permitted, in which case the space is mandatory, and this regex will produce garbage test input without it (failing propParsec).

However, I can't just modify the RESpaces to be RESpaces1, because the regular expressions are also being used to recognize valid strings, and it is permitted to omit the space between a trailing parentheses and requires.

So I could fix this by sort of just reimplementing the IncludeRenaming regex from scratch, inlining ModuleRenaming into it as necessary to resolve the ambiguity, but this seems very unsatisfactory. Is there an intended recipe for resolving this situation?

phadej

This should be guarded by cabal-version checks. We cannot extend syntax of old specs after they are allowed on Hackage.

phadej · 2021-02-25T06:36:03Z

Re regexes. I'd write regex from scratch. Maybe having two braches, one with parens and once without.

phadej · 2021-02-25T06:39:04Z

Cabal/src/Distribution/Types/ModuleRenaming.hs

@@ -121,6 +130,11 @@ moduleRenamingParsec bp mn =
        P.spaces -- space isn't strictly required as next is an open paren
        hides <- bp (P.sepBy mn cma)
        return (HidingRenaming hides)
+    parseQualified = do
+        _ <- P.string "qualified"


This is wrong. It would allow qualifiedBlabla

some day I will remember that Cabal's parser doesn't actually have a lexer stage before hand 🤣

Nor parsec has auto-try. (But Described generative tests should catch that)

phadej · 2021-02-25T07:05:29Z

I'd ask you to discuss proposed syntax publicly, at least on cabal-devel list, or better some wider place. I hope there are other backpack users than @ekmett.

I'm feel uncomfortable you jumped straight into implementation. (Though it's good to try if an idea is even viable).

ezyang · 2021-02-25T15:48:41Z

@phadej Sorry, the intention wasn't to ram this PR through as is without discussion, I wanted to show ekmett that the change he wanted was easy to do (I renamed the subject to add POC to make this clearer). The cabal-devel mailing list seems pretty moribund these days but I'm happy to send an email there.

Signed-off-by: Edward Z. Yang <[email protected]>

phadej · 2021-02-25T17:10:03Z

Cabal-described/src/Distribution/Described.hs

@@ -414,10 +414,28 @@ instance Described ForeignLibType where
    describe _ = REUnion ["native-shared","native-static"]

 instance Described IncludeRenaming where
-    describe _ = mr <> REOpt (RESpaces <> "requires" <> RESpaces1 <> mr)
+    -- Unfortunately, we can't directly use ModuleRenaming, as
+    -- there is some ambiguity in the grammar that we can't express


Is this an expressivity problem of a framework, or is there an actual ambiguity in the grammar? I might sound like a SPJ, but can you write down the BNF-grammar then (assuming there is munching lexer, so without need to worry about spaces?)

Note, these regexes are virtually only documentation of backpack field syntax (re: #4761).

And while mixin syntax definition in https://cabal.readthedocs.io/en/3.4/buildinfo-fields-reference.html is technically correct, it goes beyond what I'd consider "human understandable".

Is this an expressivity problem of a framework, or is there an actual ambiguity in the grammar?

Ooh, I used a bad turn of phrase here. Ambiguity isn't the right word.

First, here is the EBNF you requested:

IncludeRenaming ::= ModuleRenaming { "requires" ModuleRenaming } ModuleRenaming ::= (* empty *) | "(" Renaming ")" | "hiding" "(" ModuleList ")" | "qualified" ModuleName RenamingList ::= Renaming | Renaming "," RenamingList Renaming ::= ModuleName | ModuleName "as" ModuleName ModuleList ::= ModuleName | ModuleName "," ModuleList

OK, now to explain the problem here. The problem I am having with Distribution.Described lies solely in whitespace handling, as I mentioned in #7303 (comment) The problem is that it is only valid to omit the space before requires (RESpaces) if you didn't select the "qualified" production in ModuleRenaming. If you assume that everything is tokenized beforehand, the problem disappears because M.Nrequires will properly tokenize as a single module name, while )requires will tokenize as two tokens.

Note, these regexes are virtually only documentation of backpack field syntax (re: #4761).

Yes, no excuse for this. I guess now is a good time to nail that issue :)

@phadej Backpack docs exist and landed!

danidiaz · 2021-02-25T18:48:25Z

How would this proposed syntax work with reexported-modules and signatures?

ezyang · 2021-02-25T21:20:12Z

@danidiaz

How would this proposed syntax work with reexported-modules and signatures?

So, the second part (not included in this PR) is an extension to reexported-modules to permit globbing of reexports (this is not relevant to signatures as we don't require you to write all the requirements in a package, we'll infer them). But this is substantially more fraught because this is tantamount to doing #5343 which means that we have to shave the "update cabal file when you sdist" yak first.

Signed-off-by: Edward Z. Yang <[email protected]>

Kleidukos · 2023-05-17T06:22:02Z

Marking this PR as draft 🙂

ffaf1 · 2025-05-08T18:21:19Z

Hello, I am going through old PRs to check whether they are stale.

If this PR is still “live”, write a comment and I will remove the consider closing label. Otherwise in ≃ 2 weeks this PR will be closed.

ezyang requested review from phadej and ekmett February 24, 2021 22:58

phadej suggested changes Feb 25, 2021

View reviewed changes

phadej reviewed Feb 25, 2021

View reviewed changes

ezyang changed the title ~~Support qualified module renamings~~ [POC] Support qualified module renamings Feb 25, 2021

Fix the unit tests

23859e3

Signed-off-by: Edward Z. Yang <[email protected]>

phadej reviewed Feb 25, 2021

View reviewed changes

one more test file fix

2258fa5

Signed-off-by: Edward Z. Yang <[email protected]>

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022

ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022

Kleidukos marked this pull request as draft May 17, 2023 06:22

ffaf1 added the status: consider closing label May 8, 2025

[POC] Support qualified module renamings #7303

Are you sure you want to change the base?

[POC] Support qualified module renamings #7303

Uh oh!

Conversation

ezyang commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Description

Uh oh!

ezyang commented Feb 24, 2021

Uh oh!

ezyang commented Feb 24, 2021

Uh oh!

ekmett commented Feb 24, 2021

Uh oh!

ezyang commented Feb 25, 2021

Uh oh!

phadej left a comment

Choose a reason for hiding this comment

Uh oh!

phadej commented Feb 25, 2021

Uh oh!

phadej Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

phadej Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

phadej commented Feb 25, 2021

Uh oh!

ezyang commented Feb 25, 2021

Uh oh!

phadej Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

phadej Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang Feb 28, 2021

Choose a reason for hiding this comment

Uh oh!

danidiaz commented Feb 25, 2021

Uh oh!

ezyang commented Feb 25, 2021

Uh oh!

Kleidukos commented May 17, 2023

Uh oh!

ffaf1 commented May 8, 2025

Uh oh!

Uh oh!

ezyang commented Feb 24, 2021 •

edited

Loading

phadej Feb 25, 2021 •

edited

Loading