Support for deprecating exports
Needs ReviewPublic

Authored by alanasp on Jul 10 2018, 12:22 PM.

Details

Summary

The proposed change is discussed here
https://github.com/ghc-proposals/ghc-proposals/pull/134

You can find the proposal here
https://github.com/alanasp/ghc-proposals/blob/patch-2/proposals/deprecating_exports_proposal.rst

Signed-off-by: Alanas Plascinskas <alanas.pla@gmail.com>

There are a very large number of changes, so older changes are hidden. Show Older Changes
alanasp added inline comments.Aug 22 2018, 5:32 AM
compiler/typecheck/TcRnExports.hs
261

This was possibly done before you submitted the comments

alanasp updated this revision to Diff 17781.Aug 24 2018, 9:00 AM
alanasp marked an inline comment as done.

Fixes to testsuite and parser

alanasp updated this revision to Diff 17804.Aug 25 2018, 2:25 PM

swap args in ie constructor, update dependencies

alanasp updated this revision to Diff 17810.Aug 26 2018, 4:02 AM

T14189.stderr updated to reflect ie constructor changes

@bgamari I've adressed all the issues that you raised by now except for the rebasing. There were some issues when I tried to rebase onto the tip of ghc master and I am wondering if that is necessary as the only thing this was necessary in the first place are the release notes and it's just the addition of "- Export deprecation is now supported. Please refer to GHC proposal #28 <https://github.com/ghc-proposals/ghc-proposals/blob/master/proposals/0028-deprecating-exports-proposal.rst>__ and the updated user guide <https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/glasgow_exts.html#warning-and-deprecated-pragmas>__" under Language.

tdammers requested changes to this revision.Aug 27 2018, 9:41 AM
tdammers added a subscriber: tdammers.
tdammers added inline comments.
.gitmodules
3 ↗(On Diff #17810)

This entire file probably shouldn't be touched in this commit.

_config.yml
1 ↗(On Diff #17810)

This file doesn't seem to belong in this patch either.

compiler/hsSyn/HsImpExp.hs
180

Please do wrap this line to <= 80 chars. It's just a comment, so it should be trivial.

208–209

I suggest aligning the second argument with the others.

compiler/rename/RnNames.hs
392

This can easily be broken up into shorter lines.

docs/users_guide/glasgow_exts.rst
14463

This paragraph here should probably be a third bullet point in the list of possible ways of using DEPRECATED.

14476

AFAICT, the word "differences" here is a bit misleading - it's more that deprecating exports has a few natural consequences that are addressed specially, which do not apply to regular deprecation. Specifically, export deprecations mean that the same identifier can be in scope via multiple ways, and unlike regular deprecations, export deprecations make it possible for some, but not all, of the identical versions of such an identifier to be deprecated, because now the deprecation is not attached to the identifier itself, but to the route via which it is imported.

I would call the logic that deals with all this "additional" rather than "different".

14477

Typo: s/hanndled/handled/

14493

This bullet point seems excessively technical; can you give a more functional description from a GHC user's point of view here?

This revision now requires changes to proceed.Aug 27 2018, 9:41 AM
alanasp updated this revision to Diff 17885.Sep 2 2018, 3:11 PM
alanasp marked 11 inline comments as done.

lint and other small fixes

@tdammers Thanks for your suggestions. I think I've addressed most of them. Let me know what you think about some of the issues that I asked about in the comments.

.gitmodules
3 ↗(On Diff #17810)

I initially changed this because the relative links didn't work with my mirror of GHC. I can undo this change but some submodules (bytestring, nofib and haddock) required changes to build properly and those absolute links are pointing to my versions of these submodules, so they could be merged together.

_config.yml
1 ↗(On Diff #17810)

That's true, got here by mistake.

docs/users_guide/glasgow_exts.rst
14463

I thought so as well but as it currently stands, you can only use DEPRECATED for module reexports but extension to WARNING is rather trivial, so I can fix this if necessary?

14476

Thanks, yeah, I guess this would be mostly what I intended to say.

14493

Perhaps, this is more clear?

DavidEichmann added inline comments.Sep 24 2018, 12:15 PM
.gitmodules
3 ↗(On Diff #17810)

I ran into the same problem recently. I cloned from git@git.haskell.org:ghc and added my own fork as a new remote. That solved the issue.

compiler/rename/RnEnv.hs
1252

Please excuse me if I'm misunderstanding, but there seems to be an issue here (also see my comment in tryPickNonDeprecImp). We need to be clear when to throw exceptions and when to use Maybe or MaybeErr. In this case we assume that imported modules can be loaded and should otherwise throw an exception (hence the use of loadInterfaceForName which throws an exception if the interface file is not found). In effect:

  • Replace loadSrcInterface_maybe with loadSrcInterface
  • Update tryPickNonDeprecImp as per my comment on that function
1292

If this returns Nothing, it is not clear if that means an interface file failed to load or if all ImportSpecs are deprecated. As per my comment on warnIfDeprecated you should throw an exception if the interface file fails to load:

  • replace loadSrcInterface_maybe with loadSrcInterface
compiler/rename/RnNames.hs
395

Minor suggestion: You could manually inline this function if you want e.g.:

case imp_details of
    Just (False, L _ imps) -> mapM_ (flip addWarnIfDeprecatedImport (WarnSome warns)) imps
    _ -> return ()
405

Minor suggestion: I think you're looking for the lookup :: Eq a => a -> [(a, b)] -> Maybe b function from prelude. You can remove occNameMention and replace this with:

.. = case lookup (rdrNameOcc $ ieName ie) warns of
    Just wtxt -> addWarn (Reason Opt_WarnWarningsDeprecations) (pprWarningTxtForMsg wtxt)
    Nothing -> return ()
testsuite/tests/rename/should_compile/ExportDeprecation002.stderr
4

In this case it is clear from the warning that func1 is deprecated, but only because the reason string is mention it. If we e.g. got a warning like:

Example.hs:7:1: warning: [-Wdeprecations (in -Wdefault)]
    Deprecated: "Moved to module X"

where Example.hs:7 has many imports

import Example (a,b,c,d,e,f)

It is not clear which import is deprecated. Can we augment the warning message to always mention which export is deprecated?

DavidEichmann added inline comments.Sep 25 2018, 11:18 AM
compiler/rename/RnNames.hs
395

On second thought, I think it's ok not to inline, but you can still:

  • replace addWarnsIfDeprecatedImports imps exps with mapM_ (flip addWarnIfDeprecatedImport exps) imps.
    • then you can remove addWarnsIfDeprecatedImports
    • You can also manually filp the arguments of maybeAddWarnsIfDeprecatedImports and addWarnIfDeprecatedImport to make this a bit nicer.
  • And maybe rename exps to warns

Sorry a lot of comments about minor issues. All this is up to you.

405

As per my comment about the warning message, I think you would need to add this here. So something like:

addWarnIfDeprecatedImport :: LIE GhcPs -> Warnings -> TcRn ()
addWarnIfDeprecatedImport (L _ ie) (WarnSome warns) = case lookup occ warns of
    Just txt -> addWarn (Reason Opt_WarnWarningsDeprecations) (mkMsg txt)
    Nothing -> return ()
    where
        occ = rdrNameOcc (ieName ie)
        mkMsg txt = ... -- Something like: sep ["In import of " <+> quotes (ppr occ), pprWarningTxtForMsg txt]
addWarnIfDeprecatedImport _ _ = return ()
compiler/typecheck/TcRnExports.hs
236

Again here I suspect you should throw exceptions here by using loadSrcInterface instead of loadSrcInterface_maybe.
I think the code can be cleaned up a bit here is a sketch:

import Data.List (foldl')
import Data.Maybe (mapMaybe)

...

let reexp_module_names_wtxts = maybe [] get_reexp_module_names_wtxts exports
reexp_module_ifaces_wtxts <- get_reexp_module_ifaces_wtxts
                                reexp_module_names_wtxts
let mod_reexp_warns = get_reexp_warns reexp_module_ifaces_wtxts
let export_deprecation_warns = get_export_depr_warns exports

...

-- Returns Warnings structure from every export in each interface
get_reexp_warns :: [(ModIface, WarningTxt)] -> Warnings
get_reexp_warns ((iface, wtxt):xs)
    = foldl' plusWarns NoWarnings
    . map (\(iface, wtxt) -> WarnSome (map (exp_to_warn wtxt)
                                            (mi_exports iface)))
  where
    exp_to_warn wtxt export = ((nameOccName . availName) export, wtxt)

-- Returns interfaces of deprecated reexported modules with their warning
get_reexp_module_ifaces_wtxts :: [(ModuleName, WarningTxt)] ->
                                  RnM [(ModIface, WarningTxt)]
get_reexp_module_ifaces_wtxts = mapM $ \(mod_name, wtxt) -> do
    iface <- loadSrcInterface (text "load reexp modules")
                        mod_name False Nothing)
    return (iface, wtxt)

-- Returns a list of reexported module names and warnings
get_reexp_module_names_wtxts :: Located [LIE GhcPs] ->
                              [(ModuleName, WarningTxt)]
get_reexp_module_names_wtxts (L x lies)
    = mapMaybe get_mod_name_if_reexp lies

get_mod_name_if_reexp :: LIE GhcPs -> Maybe (ModuleName, WarningTxt)
get_mod_name_if_reexp (L _ (IEModuleContents _ (Just wtxt) loc_mod_name))
  = Just (unLoc loc_mod_name, wtxt)
get_mod_name_if_reexp _    = Nothing
tdammers added inline comments.Sep 27 2018, 4:48 AM
.gitmodules
3 ↗(On Diff #17810)

Well, I'm assuming here that the relative-path thing was done on purpose: this way, you can clone the entire thing recursively, and develop GHC and boot libs in concert on your fork, and the submodules will always refer to the same server as the main repo, without having to touch .gitmodules.

Whether this is the best approach stands to reason, but I'm not a fan of casually changing it without good reason - especially not the ones that you *don't* divert to your fork.

docs/users_guide/glasgow_exts.rst
14493

Works for me!

alanasp updated this revision to Diff 18488.Oct 27 2018, 2:34 PM
alanasp marked 18 inline comments as done.
  • Occurence name added to warning messages for deprecated imports

I've addressed and/or fixed the issues raised, please let me know if there's anything else you'd like changing.

.gitmodules
3 ↗(On Diff #17810)

I only intended the absolute paths as a temporary workaround. Wouldn't it be best if my changes to the submodules that actually required changing be accepted first? Then I could change everything back to relative in one step as for now the differential wouldn't build with all relative links.

compiler/rename/RnEnv.hs
1252

There is an issue if we try loading a self import with loadSrcInterface as this is an error. I thought loadSrcInterface_maybe is a suitable workaround.

1292

As above, I did this to avoid issues with self imports. Do you think there is a more suitable workaround?

compiler/rename/RnNames.hs
395

Thanks, this is helpful

405

Thanks, this certainly helps with the clarity issue.

compiler/typecheck/TcRnExports.hs
236

Thanks, I've mostly cleaned up what was suggested here, except for the loadSrcInterface issue as explained above.

Harbormaster edited this Differential Revision.Oct 28 2018, 1:29 PM

Regarding loadSrcInterface vs loadSrcInterface_maybe , it is hard to see what the correct solution is here. What exactly do you mean by "self import"? something like module A where; import module A?

@DavidEichmann Yeah, the self-referencing import is used in some GHC native modules.
Also, I am getting this error when Phab attempts Linux build:
configure: error: GHC 8.2.1 is known to be buggy and cannot bootstrap this GHC release (See Trac 15281); please use GHC 8.2.2 or later.
I am not getting it on my own machine and the other builds seem to succeed. Is it something to do with Phab or something that I'd have to look into?

DavidEichmann added a comment.EditedOct 30 2018, 4:30 AM

@alanasp I wasn't aware that ghc supported self imports. The concept sounds a bit absurd to me (why would a module ever import itself?). I know GHC 8.4.3 would complain about circular imports, which is what I expect. I would expect a similar error to be raised by the code in this patch, but perhaps I'm missing something. Can you give a concrete example? How did you run into this use case?

P.S It looks to me like the build system is using the wrong stage0 ghc version, which would be unrelated to this patch.

@DavidEichmann Ok, it's actually self-export rather than self import. The example is here /libraries/base/GHC/Num.hs
I agree that the self-import wouldn't make sense but the self-export here is useful and to me using loadSrcInterface_maybe seemed like the most straightforward if not the best solution.

@DavidEichmann Ok, it's actually self-export rather than self import. The example is here /libraries/base/GHC/Num.hs

@alanasp Ah, thank you! That makes a lot more sense now.

I agree that the self-import wouldn't make sense but the self-export here is useful and to me using loadSrcInterface_maybe seemed like the most straightforward if not the best solution.

It's definitely the most straightforward solution, though I think it may cause some problems. Stepping back a bit, you are taking the "use a default-value" approach to deal with errors here. In general, this approach is easy to implement, but can be dangerous as it can (and I generally expect it to) silently introduce subtle bugs. So in this case your "default value" is to just ignore the file that fails to load. I expect this will lead to bugs e.g.:

-- Main.hs --
module Main where
import A
main = print b   -- b is deprecated

-- A.hs --
module A (
    module A  -- Self export
    {-# DEPRECATED module B "Import B directly" #-}
    module B
  ) where
import B


-- B.hs --
module B where
b = 2

The above is supposed to give a warning, but I suspect with your patch, it will silently omit the warning because it fails to load A due to a self export. As another example, if at least 1 import of 'b' is NOT deprecated then you should omit the warning, but I think in the following case it will still show a warning saying "Import B directly" even though B is imported directly!

-- Main.hs --
module Main where
import A -- b from here is deprecated
import B -- b from here is NOT deprecated
main = print b

-- A.hs --
module A (
    -- no self export
    {-# DEPRECATED module B "Import B directly" #-}
    module B
  ) where
import B


-- B.hs --
module B (
    module B  -- Self export
  ) where
b = 2

P.S I haven't actually tried this, this is just my guess as to what will happen.
What are your thoughts? If you try this and find the above cases helpful, then I suggest adding them or similar as tests.

alanasp updated this revision to Diff 18573.Nov 2 2018, 12:51 PM

Additional test cases added as per @DavidEichmann request

@DavidEichmann The case that you pointed out indeed works as intended. I've made sure that if there is a non-deprecating way a symbol can be brought into context then that way will be used.

However, in the odd case, when the self export itself is deprecated, GHC would fail to display a deprecation warning. This is because I have to load the source interface to be able to access the export list but I only need to load it for the deprecated module reexports.
Do you think this is something that would need fixing?

-- Main.hs --
module Main where
import A
main = print b   -- b is deprecated but no warning because we were unable to load module A

-- A.hs --
module A (
    {-# DEPRECATED "A self export is deprecated" #-}
    module A  -- Self export
    {-# DEPRECATED "Import B directly" #-}
    module B
  ) where
import B
  

-- B.hs --
module B where
b = 2

@DavidEichmann The case that you pointed out indeed works as intended. I've made sure that if there is a non-deprecating way a symbol can be brought into context then that way will be used.

Thanks for looking further into this @alanasp.

However, in the odd case, when the self export itself is deprecated, GHC would fail to display a deprecation warning. This is because I have to load the source interface to be able to access the export list but I only need to load it for the deprecated module reexports.
Do you think this is something that would need fixing?

Absolutely! A warning is omitted. This doesn't fit the specification so I'd consider it a bug.
If I understand you correctly, in your example, loadSrcInterface_maybe for module A returns an error? This is essentially what I was trying to get at in my previous comment: if there is a way to make loadSrcInterface_maybe return an error (for an otherwise valid module), then there will be bugs like the case you've found.

So what should you do? I would investigate why loadSrcInterface_maybe returns an error. This seems like a bug in loadSrcInterface_maybe: it should not return an error. If it turns out that this is an unrelated bug then great: just make a new trac ticket. Though the problem may be in this patch since it happens when a "self export itself is deprecated" (that's functionality added in this patch). I appreciate that a lot of time and effort has already gone into this patch, I'd like to help more though I'm having issue getting this to build and am fairly busy at the moment.

Hi @alanasp, Have you managed to have a look at the issue since we last spoke? It would be great to see this to completion. I appreciate that a significant amount of effort has already gone into this patch. If you're short on time/resources, please let me know and I will investigate this further.

Hi @DavidEichmann
I should have some time available to look into this next week. Currently, this is a very busy period at the university.

alanasp updated this revision to Diff 18882.Nov 25 2018, 12:47 PM

Resolved issue with deprecated self import not raising warning

@DavidEichmann I don't think there's a problem with loadSrcInterface_maybe as the interface of the module, which I try to load does not exist yet. The fix doesn't involve loading the current source interface and so avoids this issue.

Hello, thanks for fixing that, great work! I see you also added a test which is always appreciated. Before I accept:

  • I still think throwing an exception is more valid. I.e. replace loadSrcInterface_maybe with loadSrcInterface (this change would also make the code a bit simpler). If you're not sure why, do ask. If you're convinced otherwise, I'm not gonna fight you on it.
  • The changes in .gitmodules should be reverted (as a rule of thumb, I'll only accept if I think it's ready to merge as is).
    • You can easily fix this without breaking your build by, first pushing your changes git push, then re-cloning ghc using git clone git://git.haskell.org/ghc --recurse-submodules (if you need help with this feel free to contact me on IRC at freenode #haskell my handle is DavidEichmann).
compiler/typecheck/TcRnExports.hs
180

Look like you can remove warn_all_if_deprec_reexp and just use

maybe NoWarnings WarnAll (lookup (moduleName this_mod) reexp_module_names_wtxts)
alanasp updated this revision to Diff 18974.Dec 2 2018, 4:25 PM
  • loadSrcInterface instead of loadSrcInterface_maybe and some minor code fixes
  • .gitmodule links changed to relative
alanasp updated this revision to Diff 18975.Dec 2 2018, 4:32 PM
  • revert .gitmodules to parent commit

Hi @DavidEichmann,

I've fixed the issues you pointed out and removed the .gitmodules from the revision entirely.

Let me know how it looks.

alanasp updated this revision to Diff 18976.Dec 2 2018, 4:55 PM
  • All lint warnings fixed

Hello, sorry for the delay here. I can't seem to build this patch and am seeing merge conflicts. Can you please rebase on master, and resolve any merge conflicts.

@DavidEichmann I think it will be much easier if you rebase the patch? This review cycle has been going on for a very long time now.

@mpickering Ok, I'll do the rebase now.

Rebase, resolve conflicts, and fix build.

  • Included all changes.

@DavidEichmann Thanks. Also, let me know if there's anything else that needs fixing.