search

haskell / filepath

Haskell FilePath core library
BSD 3-Clause "New" or "Revised" License
66 stars 33 forks source link

OsString deprecation applies to all its exports #209

Closed tbidne closed 7 months ago

tbidne commented 7 months ago

Hello.

First, thanks for all your hard work on AFP. I'm a big fan of the new OsPath functionality.

It appears that the deprecations in https://github.com/haskell/filepath/pull/206 caused all downstream symbols to be deprecated, not just the module names. Was that intended? For instance, I would expect the following to succeed, since it is not using a deprecated module or symbol that was itself deprecated:

import System.OsPath.Encoding (EncodingException)

foo :: Either EncodingException ()
foo = Right ()

Yet:

 src/Lib.hs:7:15: error: [GHC-68441] [-Wdeprecations, Werror=deprecations]
Error:     In the use of type constructor or class ‘EncodingException’
    (imported from System.OsPath.Encoding, but defined in System.OsPath.Encoding.Internal):
    Deprecated: "Use System.OsString.Encoding.Internal from os-string >= 2.0.0 package instead. This module will be removed in filepath >= 1.5."
  |
7 | foo :: Either EncodingException ()
  |               ^^^^^^^^^^^^^^^^^

This failure can be seen here: https://github.com/tbidne/osstr-deprecated.

I didn't know module deprecations worked this way, so I wonder if this was unintended. PVP considers deprecations breaking changes (see 7), so it seems like this probably qualifies.

Additional thoughts

I'm not sure of a good fix here. You can work around this sometimes by creating new entities e.g.

{-# OPTIONS_GHC -Wno-deprecations #-}

module LibGood (foo) where

import qualified Deprecated

-- Deprecated.foo is deprecated, but the export is fine because it is a new, undeprecated symbol.
foo :: String
foo = Deprecated.foo ++ " LibGood"

But I doubt this can work well when the deprecations are on the types themselves. Moreover, this seems like a lot of work for little gain, when you consider people will have to move to the new types in os-string anyway.

Perhaps the best solution is to revise 1.4.200.0 to an impossible build plan, so the solver doesn't pick it for everyone with a <1.5 bound.

Finally, this feels like a flaw with module deprecations and could be reported on ghc's issue tracker, but I wanted to get other opinions first.

Thanks!

Edit: I should have noted, this is really only a problem for -Werror users. PVP does explicitly point to this use-case, but on the other hand, it has been debated elsewhere that -Werror should be excluded. Perhaps PVP should be updated.

hasufell commented 7 months ago

That is unfortunate.

So a re-export of a deprecated module propagates.

We want to signal that importing System.OsPath.Encoding.Internal at all (even with no import list) will be a failure soon.

@Bodigrim ideas?

hasufell commented 7 months ago

Maybe we can silence deprecation warnings for System.OsPath.Encoding by adding {-# OPTIONS_GHC -Wno-deprecations #-} to its modules.

Can you test that @tbidne ?

tbidne commented 7 months ago

Unfortunately I think {-# OPTIONS_GHC -Wno-deprecations #-} applies only to the module itself. Entities retain warnings/deprecations. I experimented with this here: https://github.com/tbidne/module-deprecate/tree/main.

I also tried adding {-# OPTIONS_GHC -Wno-deprecations #-} to the Deprecated.hs module itself in that repo: no luck.

I fear there is no way to export an "unwarned" version of a symbol.

Edit: Incidentally, there are some GHC links for possible designs on silencing warnings on an individual basis:

I attempted to implement this a bit ago but didn't get too far.

hasufell commented 7 months ago

Also discussed here: https://gitlab.haskell.org/ghc/ghc/-/issues/24017#note_536724

hasufell commented 7 months ago

I think the only solution is to:

  1. keep System.OsPath.Encoding.Internal in filepath for now (e.g. 1.4.200.1) without deprecation warning
  2. make it a re-export shim in e.g. filepath-1.5.0.1 of System.OsString.Encoding.Internal
  3. Have another deprecation cycle now for System.OsPath.Encoding.Internal

This has two major downsides:

  1. we probably have to mark filepath 1.4.200.0 and 1.5.0.0 as broken (base < 0)
  2. another deprecation cycle means we have yet another major version bump just for deprecation purposes (e.g. filepath-1.6)
tbidne commented 7 months ago

To be fully compliant with PVP as currently written, yes, I think you are right. It's a shame as it is a significant amount of work primarily due to module deprecations having surprising behavior. 1.5 becomes the deprecation warning, and 1.6 becomes the new version with os-string.

For what it's worth, SPJ argues in this PR that deprecations (and warnings, more generally) should not affect "stability", as defined there. If that is the consensus, then PVP's current stance on deprecations should probably be reconsidered.

tbidne commented 7 months ago

There is in fact a workaround for the deprecated module issue, though it's grim and requires GHC 9.8+. Using the new deprecated exports feature, you can instead deprecate every export individually. This has the expected effect.

I did this on my filepath fork here, and my branch built successfully. On the other hand, changing the import to use the "deprecated module" produces the error, as desired.

diff --git a/src/Lib.hs b/src/Lib.hs
index cf05c87..2fbae8b 100644
--- a/src/Lib.hs
+++ b/src/Lib.hs
@@ -2,7 +2,8 @@

 module Lib (foo) where

-import System.OsPath.Encoding (EncodingException)
+--import System.OsPath.Encoding (EncodingException)
+import System.OsPath.Encoding.Internal (EncodingException)

 foo :: Either EncodingException ()
 foo = Right ()
src/Lib.hs:6:41: error: [GHC-68441] [-Wdeprecations, -Werror=deprecations]
    In the use of type constructor or class ‘EncodingException’
    (imported from System.OsPath.Encoding.Internal):
    Deprecated: "Use System.OsString.Encoding.Internal from os-string >= 2.0.0 package instead. This module will be removed in filepath >= 1.5."
  |
6 | import System.OsPath.Encoding.Internal (EncodingException)
  |                                         ^^^^^^^^^^^^^^^^^

src/Lib.hs:8:15: error: [GHC-68441] [-Wdeprecations, -Werror=deprecations]
    In the use of type constructor or class ‘EncodingException’
    (imported from System.OsPath.Encoding.Internal):
    Deprecated: "Use System.OsString.Encoding.Internal from os-string >= 2.0.0 package instead. This module will be removed in filepath >= 1.5."
  |
8 | foo :: Either EncodingException ()
  |               ^^^^^^^^^^^^^^^^^
Error: cabal: Failed to build osstr-deprecated-0.1.

Of course this doesn't help here since it requires GHC 9.8, and attaching the deprecation to every symbol is pretty unfortunate. But, happily, it does work.

Bodigrim commented 7 months ago

How about doing https://github.com/tbidne/filepath/commit/f9214950845a1afcb48324ef120fa147f99bcadf under CPP, for GHC 9.8+ only? The new release of filepath is not to hit casual users before GHC 9.10 anyway.

hasufell commented 7 months ago

How about doing https://github.com/tbidne/filepath/commit/f9214950845a1afcb48324ef120fa147f99bcadf under CPP, for GHC 9.8+ only? The new release of filepath is not to hit casual users before GHC 9.10 anyway.

So we do this in 1.4.200.1 and then set base < 0 for 1.4.200.0 in a revision?

Bodigrim commented 7 months ago

For what it's worth, SPJ argues in https://github.com/ghc-proposals/ghc-proposals/pull/625 that deprecations (and warnings, more generally) should not affect "stability", as defined there. If that is the consensus, then PVP's current stance on deprecations should probably be reconsidered.

See the upcoming PVP 1.1 which no longer recommends major version bumps because of deprecations.

There is in fact a workaround for the deprecated module issue, though it's grim and requires GHC 9.8+.

Could we put the actual content of a public module System.OsString into a non-exposed System.OsString.Hidden (without deprecation pragma) and make other modules export stuff from System.OsString.Hidden instead of System.OsString? Then System.OsString can re-export entire System.OsString.Hidden but slap a module-wide deprecation pragma atop. Would this have a desired effect?

tbidne commented 7 months ago

Could we put the actual content of a public module System.OsString into a non-exposed System.OsString.Hidden (without deprecation pragma) and make other modules export stuff from System.OsString.Hidden instead of System.OsString? Then System.OsString can re-export entire System.OsString.Hidden but slap a module-wide deprecation pragma atop. Would this have a desired effect?

Clever! I tried this here and it seems to work.

The commit message goes into more detail, but I essentially used your strategy. For a deprecated module X that had new definitions, copy to non-deprecated X.Hidden, and have X re-export X.Hidden. There are 9 deprecated modules:

Of these, System/OsString.hs and System/OsString/Types.hs are just re-exports, so there is no need to create shims for them AFAICT. That leaves the following 7:

Sadly, shuffling the deprecation message means github's diff algorithm isn't smart enough to know the files were mostly renamed, so the diff is pretty intimidating.

To be clear, I am not asking for this PR, as I am satisfied with the PVP resolution and module deprecation explanation. But if you feel it's worth it, I can open one.

hasufell commented 7 months ago

https://hackage.haskell.org/package/filepath-1.4.200.1