Skip to content

Commit

Permalink
[Chore] Update the rewrite rules for text
Browse files Browse the repository at this point in the history
Problem: after `2.0.1` version they rewrote `pack` completely, so now
our tricks with folding `pack`/`unpack`'s internals do not work.

For details, see [this commit in `text`
package](haskell/text@5a666e4).

Fortunately, annihilating entire `pack` and `unpack` calls is still an
option. Along with that `pack` rewrite they added `NOINLINE [0]` pragma
for `pack`, meaning that we have even better chances of the rewrite rule
to fire.

Solution: add a `MIN_VERSION` pragma to disable the now non-working
rewrite rule in the recent `text` versions.
Rely on the remaining `unpack (pack s) = s` and `pack (unpack s) = s`
rewrite rules to fire at stage 0.

Update maintenance notes, in the new reality verifying whether `text`
bump is correct should be simpler.
  • Loading branch information
Martoon-00 committed Jun 23, 2023
1 parent 1f840f0 commit 8049813
Showing 1 changed file with 27 additions and 18 deletions.
45 changes: 27 additions & 18 deletions src/Universum/String/Conversion.hs
Original file line number Diff line number Diff line change
Expand Up @@ -164,41 +164,49 @@ instance ToString T.Text where
instance ToString LT.Text where
toString = LT.unpack

{- [Note toString-toText-rewritting]
Note ON MAINTENANCE of rewrite rules below.
Whenever you want to allow a newer version of @text@ package, check:
* The @text conversions@ benchmark, that the numbers with and without the rules
are still the expected ones (see the comments there for what to expect).
* You may optionally check whether any changes to `pack` and `unpack` functions
and their `INLINE`/`NOINLINE` annotations took place.
The current rewrite rules match with what happens in `text` package
at the e5644b663c32c01a1de7299a5e711216755e01bc commit, and next time
you can just check `git diff <that commit>..HEAD`.
If these points hold, it should be safe to raise the upper bound on `text` version.
If not, first do the necessary changes (preserving the backward compatibility),
update the commit id above if you checked the code of the `text` package.
Then bump the version constraint.
-}

{-
@toString . toText@ pattern may occur quite often after inlining because
we tend to use 'Text' rather than 'String' in function signatures, but
there are still some libraries which use 'String's and thus make us perform
conversions back and forth.
Note that @toString . toText@ is not strictly equal to identity function, see
explanation in the comment below.
Note that, as documentation for 'T.pack' function mentions, @toString . toText@
is not strictly equal to the identity function. Thus, replacing @toString . toText@
with @id@ will result in losing transformation of surrogate code points.
But in the most cases this is what the user wants.
-}

{-# RULES "pack/unpack" [~0]
forall s. T.unpack (T.pack s) = s
#-}

{- [Note toString-toText-rewritting]
#if !MIN_VERSION_text(2,0,2)
{-
We can do even better than above if take rules defined in 'Data.Text' into
account.
Note ON MAINTENANCE: whenever you need to update the used version of @text@ package,
you have to check whether the comment below is still valid.
However, you can cut down by looking at the diff between versions of @text@, and
seeing if implementation of any of these functions have changed:
* pack
* unpack
* streamList
* unstreamList
* safe
* any RULES definition (if some is added, this counts)
Also check the @text conversions@ benchmark, that the numbers with and without
the rules are still the expected ones
If none of mentioned have changed, then it is safe to assume that everything
is still fine.
Quoting investigation of @int-index:
If we look at @unpack@ and @pack@ they are defined as
Expand Down Expand Up @@ -269,6 +277,7 @@ So, eventually, we add the following rule:
{-# RULES "pack/unpack internal" [1]
forall s. TF.unstreamList (TF.map T.safe (TF.streamList s)) = s
#-}
#endif

{- In case if GHC didn't manage to inline and rewrite everything in
the remaining phases (@Data.Text.pack@ is inlined at 1-st phase),
Expand Down

0 comments on commit 8049813

Please sign in to comment.