Implemented TH splices for validated ByteString literals #712

vdukhovni · 2025-08-16T06:53:37Z

thLiteral    :: Quote m => String -> Code m ByteString
thHexLiteral :: Quote m => String -> Code m ByteString

The former rejects inputs with non-octet code points above 0xFF. The latter rejects odd-length inputs or inputs with characters other than non-hexadecimal digits.

vdukhovni · 2025-08-16T09:13:18Z

@Bodigrim I don't understand what's going on with CI. Any help appreciated.

Bodigrim · 2025-08-16T09:31:04Z

@vdukhovni no idea tbh. Maybe something changed under the hood, either in runner images or in the haskell action.

vdukhovni · 2025-08-16T11:57:46Z

I have very little experience tuning GitHub CI. Any chance someone can help?

Bodigrim · 2025-08-16T18:14:22Z

It seems it was an intermittent failure with https://github.com/haskell-actions/setup. I don't think you need to touch CI setup in this PR.

vdukhovni · 2025-08-17T00:28:54Z

Thanks, indeed most of the problems appear to have been transient. I reverted the CI changes, and the only failure so far is with OpenBSD, which reports:
⚠️ Not enough compute credits to prioritize tasks!

Otherwise, no issues. So I think I'm done, unless you'd prefer to name the two functions differently. The names thLiteral and thHexLiteral were a best effort choice at the time, but one can probably make a case for other choices if these don't appeal.

vdukhovni · 2025-08-21T03:10:26Z

Review request: @hsyl20 @Bodigrim @clyring

hsyl20

LGTM

I would prefer more explicit names: something like literalFromAscii (or literalFromChar8) and literalFromHex

Data/ByteString/Internal/Type.hs

vdukhovni · 2025-08-21T10:43:03Z

LGTM

I would prefer more explicit names: something like literalFromAscii (or literalFromChar8) and literalFromHex

Many thanks for the prompt review! I'm about to push a fixup for all the nits, and what remains then is to reach consensus on the splice names. Of the above I prefer literalFromChar8 over literalFromAscii and have no objections to literalFromHex. I take it you don't see any benefit from including a th prefix to make it clear these are splices rather than directly usable functions?

hsyl20 · 2025-08-21T12:36:47Z

I take it you don't see any benefit from including a th prefix to make it clear these are splices rather than directly usable functions?

Yes the type and literal already convey that imo.

Data/ByteString/Internal/Type.hs

vdukhovni · 2025-08-24T14:53:17Z

@hsyl20, @Bodigrim Many thanks for the reviews, much appreciated. If at some point you find some more review cycles, I've revived, rebased and improved #569, so reviews there would also be great.

vdukhovni · 2025-09-09T14:47:01Z

@hsyl20 @clyring @Bodigrim I believe this is done. Please let me know if anything is missing.

clyring · 2025-09-14T02:35:18Z

Data/ByteString/Internal/Type.hs

-import Data.Bits                ((.&.))
+import Data.Bits                ((.|.), (.&.), complement, shiftL)
 import Data.Char                (ord)
+import Data.Foldable            (foldr')


The unqualified foldr' briefly confused me. (Actually, why are these quote-generators defined in D.B.Internal.Type instead of the exposed Data.ByteString?)

Data/ByteString/Internal/Type.hs

clyring · 2025-09-14T02:45:05Z

Data/ByteString/Internal/Type.hs

+literalFromChar8 "" = [||empty||]
+literalFromChar8 s = case foldr' op (Octets 0 []) s of
+    Octets n ws -> liftTyped (unsafePackLenBytes n ws)
+    Hichar i w  -> liftCode $ fail $ "non-octet character '\\" ++


@TeofilC Would this liftCode $ fail $ ... stuff require any adjustments to your template-haskell-lift plans?

Thanks for the headsup. This should be fine.

Data/ByteString/Internal/Type.hs

vdukhovni · 2025-10-03T01:06:20Z

Anything still to do on my end?

clyring

LGTM, but:

I would want to pluralize the name literalFromChar8 (which looks singular). Perhaps just literalFromChar8s or literalFromString8. But there probably isn't much room for confusion anyway.
I suspect you will want to make a clean commit message.

Data/ByteString/Internal/Type.hs

vdukhovni · 2025-10-13T07:43:34Z

LGTM, but:

I would want to pluralize the name literalFromChar8 (which looks singular). Perhaps just literalFromChar8s or literalFromString8. But there probably isn't much room for confusion anyway.

I ultimately don't have strong views about the naming, so willing to make changes, but my intuition is that Char8 suffix is fairly natural, given the existing Data.ByteString.Char8. Do others also prefer avoiding Char8 here?
If change is needed, I'd propose literalFromOctetString.

I suspect you will want to make a clean commit message.

Sure, I'll squash and update the commit message.

clyring · 2025-10-16T02:57:21Z

I ultimately don't have strong views about the naming, so willing to make changes, but my intuition is that Char8 suffix is fairly natural, given the existing Data.ByteString.Char8. Do others also prefer avoiding Char8 here?
If change is needed, I'd propose literalFromOctetString.

The Char8 aspect is indeed very natural. My concern is just that literalFromChar8 reads a bit like a function that takes only one Char8. literalFromOctetString sounds good.

literalFromOctetString :: Quote m => String -> Code m ByteString literalFromHex :: Quote m => String -> Code m ByteString The former rejects inputs with non-octet code points above 0xFF. The latter rejects odd-length inputs or inputs with characters other than non-hexadecimal digits.

vdukhovni · 2025-10-16T04:13:20Z

Sadly, not enough info about why the OpenBSD CI run failed, looks unrelated to this PR, as various other *BSD jobs have been failing in other PRs recently.

,,,
[16 of 16] Linking dist-newstyle/build/x86_64-openbsd/ghc-9.8.3/bytestring-0.13.0.0/t/bytestring-tests/build/bytestring-tests/bytestring-tests
ld.lld: warning: OSThreads.c(OSThreads.thr_o:(createAttachedOSThread) in archive /usr/local/lib/ghc-9.8.3/lib/../lib/x86_64-openbsd-ghc-9.8.3/rts-1.0.2/libHSrts-1.0.2_thr.a): warning: strcpy() is almost always misused, please use strlcpy()
ld.lld: warning: EventLogWriter.c(EventLogWriter.thr_o:(initEventLogFileWriter) in archive /usr/local/lib/ghc-9.8.3/lib/../lib/x86_64-openbsd-ghc-9.8.3/rts-1.0.2/libHSrts-1.0.2_thr.a): warning: sprintf() is often misused, please use snprintf()
Running 1 test suites...
Test suite bytestring-tests: RUNNING...
Test suite bytestring-tests: FAIL
Test suite logged to:
/tmp/cirrus-ci-build/./dist-newstyle/build/x86_64-openbsd/ghc-9.8.3/bytestring-0.13.0.0/t/bytestring-tests/test/bytestring-0.13.0.0-bytestring-tests.log
0 of 1 test suites (0 of 1 test cases) passed.
Error: [Cabal-7125]
Tests failed for test:bytestring-tests from bytestring-0.13.0.0.

vdukhovni force-pushed the th-splices branch 2 times, most recently from 70f1a6a to b53c921 Compare August 16, 2025 08:26

vdukhovni force-pushed the th-splices branch from b53c921 to a6b5c82 Compare August 17, 2025 00:13

hsyl20 approved these changes Aug 21, 2025

View reviewed changes

Data/ByteString/Internal/Type.hs Outdated Show resolved Hide resolved

Data/ByteString/Internal/Type.hs Outdated Show resolved Hide resolved

Data/ByteString/Internal/Type.hs Outdated Show resolved Hide resolved

vdukhovni force-pushed the th-splices branch 2 times, most recently from c5906d9 to 28a0cb6 Compare August 21, 2025 14:09

Bodigrim reviewed Aug 21, 2025

View reviewed changes

Data/ByteString/Internal/Type.hs Show resolved Hide resolved

Data/ByteString/Internal/Type.hs Outdated Show resolved Hide resolved

Bodigrim reviewed Aug 22, 2025

View reviewed changes

Data/ByteString/Internal/Type.hs Outdated Show resolved Hide resolved

Data/ByteString/Internal/Type.hs Outdated Show resolved Hide resolved

Data/ByteString/Internal/Type.hs Outdated Show resolved Hide resolved

vdukhovni force-pushed the th-splices branch 2 times, most recently from 83cf073 to 2f5671a Compare August 23, 2025 03:11

Bodigrim approved these changes Aug 24, 2025

View reviewed changes

Bodigrim requested a review from clyring August 24, 2025 10:28

clyring reviewed Sep 14, 2025

View reviewed changes

vdukhovni force-pushed the th-splices branch 2 times, most recently from 8b3189d to d8c2d02 Compare September 14, 2025 10:53

clyring approved these changes Oct 13, 2025

View reviewed changes

Data/ByteString/Internal/Type.hs Show resolved Hide resolved

vdukhovni force-pushed the th-splices branch from d8c2d02 to 264ce7f Compare October 13, 2025 07:48

vdukhovni force-pushed the th-splices branch from 264ce7f to 489d8db Compare October 16, 2025 03:35

Implemented TH splices for validated ByteString literals #712

Are you sure you want to change the base?

Implemented TH splices for validated ByteString literals #712

Uh oh!

Conversation

vdukhovni commented Aug 16, 2025

Uh oh!

vdukhovni commented Aug 16, 2025

Uh oh!

Bodigrim commented Aug 16, 2025

Uh oh!

vdukhovni commented Aug 16, 2025

Uh oh!

Bodigrim commented Aug 16, 2025

Uh oh!

vdukhovni commented Aug 17, 2025

Uh oh!

vdukhovni commented Aug 21, 2025

Uh oh!

hsyl20 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vdukhovni commented Aug 21, 2025

Uh oh!

hsyl20 commented Aug 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vdukhovni commented Aug 24, 2025

Uh oh!

vdukhovni commented Sep 9, 2025

Uh oh!

clyring Sep 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clyring Sep 14, 2025

Choose a reason for hiding this comment

Uh oh!

TeofilC Sep 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vdukhovni commented Oct 3, 2025

Uh oh!

clyring left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vdukhovni commented Oct 13, 2025

Uh oh!

clyring commented Oct 16, 2025

Uh oh!

vdukhovni commented Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants