Skip to content

Handle ignored byte-order marks at end of input - #166

Merged
svenvc merged 2 commits into
svenvc:masterfrom
panalcloud:fix-encoder-issue
Nov 11, 2026
Merged

svenvc merged 2 commits into
svenvc:masterfrom
panalcloud:fix-encoder-issue

Conversation

@lopezca

@lopezca lopezca commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Hi @svenvc hope all is well.

Here's a PR regarding that mythical U+FEFF point code. I read the old discussion #134 and agree that it should be dropped. this code is just to handle the removal and the option to keep it if desired. It comes up when using P3 to read data and "test" data may contain it so we get 2 issues, one the character count is off (the server sends 3 characters, one of these being the dropped code point) and we only see 2 on the string, so I want to write a test that gets that and can say that got 3 characters. the other is that in some locations, the encoder might give an error instead of just dropping the character, this is also changed here. please take a look if possible.
thanks.

here's the llm summary:

Handle ignored byte-order marks at end of input

This PR fixes UTF-8, UTF-16, and UTF-32 decoding when an ignored U+FEFF byte-order mark ends the input. Previously, skipping it attempted to read another character and incorrectly raised ZnIncomplete; consecutive marks were also not fully skipped in UTF-16/32. Although this code point is very rare in that position and the intention is to drop it, consuming it must still leave the encoders and underlying streams in the correct state, respecting end-of-input and bounded-stream limits.

The first commit (35da8db4) adds six regression tests covering trailing, consecutive, and BOM-only inputs, bounded streams, direct reads, and preservation when ignoreByteOrderMark: false; it also changes the existing lone-BOM test to expect empty text rather than an incomplete-input error. The second commit (d2ae55cd) stops low-level decoding from reading beyond a trailing mark, returning the mark so whole-input decoders and character/code-point stream readers can discard it through isIgnoredCodePoint:. It also makes UTF-16/32 skip consecutive marks, updates bulk reads and stream collection methods, and documents using ZnLimitedReadStream when decoding only part of a larger stream.

UTF-8, UTF-16 and UTF-32 encoders skip U+FEFF when ignoreByteOrderMark
is true. A mark that ends the input must be dropped like any other:
decodeBytes:, decodeBytesIntoWideString:, decodeAsCodePoints:,
readInto:startingAt:count:fromStream:, ZnCharacterReadStream and
ZnCodePointReadStream answer the text without it, and marks alone are
an empty text. Consecutive marks are all skipped in every encoder,
also after wide characters and in runs of 4,096 marks. A mark at the
end of a ZnLimitedReadStream does not read past the limit.
nextFromStream: answers a mark that ends the input, since there is no
character after it to answer instead.

An ignored mark at the end does not turn a ByteString result into a
WideString, for decodeBytes: and for ZnCharacterReadStream>>upToEnd.
testUpToEndAfterPeek covers a peeked character followed by upToEnd.

testUTF8ByteOrderMark pinned that a lone mark is ZnIncomplete; it now
expects an empty text, the mark being complete UTF-8.
UTF encoders skipped an ignored U+FEFF by reading the character after
it, so a mark that ended the input failed with ZnIncomplete. UTF-16
and UTF-32 skipped only one mark in a row, and UTF-8 skipped marks by
recursion, one stack frame per mark.

nextCodePointFromStream: now skips marks in a loop, in every encoder,
only while more input follows, and answers a mark that ends the input.
ZnCharacterEncoder>>isIgnoredCodePoint: tells readers of whole inputs
to drop it:
- decodeBytes:, decodeBytesIntoWideString: and decodeAsCodePoints:
  check once after their loop. decodeBytes: does not widen its
  ByteString for a final mark.
- readInto:startingAt:count:fromStream: (ZnUTF8Encoder, and a new one
  in ZnUTFEncoder for UTF-16/32) stops there.
- ZnCharacterReadStream and ZnCodePointReadStream answer nil from
  nextElement, which upToEnd and upTo: skip.
- ZnCharacterReadStream>>upToEnd reads from the encoder directly
  instead of through next and nextElement, and does not write the
  final mark.

A mark followed by more bytes still takes the next character, since an
encoder cannot know where the caller's text ends. The comment of
ignoreByteOrderMark: says so and points to ZnLimitedReadStream or
ignoreByteOrderMark: false.

Cost against master, Pharo 13, 100,000 characters, median of 9 to 15
interleaved samples, same image:
- Allocation is unchanged.
- decodeBytes: and a loop of nextCodePointFromStream: are within 4%.
- decodeBytesIntoWideString: is 5-10% faster, and
  ZnCharacterReadStream>>next: on UTF-16/32 is 6-9% faster.
- A loop of ZnCharacterReadStream>>next is within 1%, 3-5% slower
  for UTF-8 Latin-1 text.
- ZnCharacterReadStream>>upToEnd is 20-39% faster for UTF-8, UTF-16
  and cp1252. FileReference>>contents reads through it.
- With a final mark, where master fails, decodeBytes: and upToEnd
  answer a ByteString and allocate as much as without the mark.

Zinc-Character-Encoding-Tests 139/139, Zinc-Tests 254/254 (1
skipped), Zinc-Resource-Meta-Tests 100/100, Zinc-WebSocket-Tests
17/17, Zinc-Zodiac-Tests 9/9 (2 skipped), FileSystem tests 477/477
(4 skipped).
@lopezca lopezca changed the title Fix encoder issue Handle ignored byte-order marks at end of input Nov 7, 2026
@svenvc
svenvc merged commit a0b5da5 into svenvc:master Nov 11, 2026
6 checks passed
@svenvc

svenvc commented Nov 11, 2026

Copy link
Copy Markdown
Owner

Thx.

It is quite an edge case, but it looks OK.
I hope the weights did not make any mistakes ;-)

The P3 reference is intriguing: did it also come up with artificial test data ?

@lopezca

lopezca commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for taking the time!
I'm not sure i got your question, did the llm come up with the test data? I don't think so, It came up when porting the https://github.com/psycopg/psycopg2 tests into pharo and implementing them to run vs P3, they have a case about this unicode point.

@svenvc

svenvc commented Nov 11, 2026

Copy link
Copy Markdown
Owner

Ah, OK, good to know.

Are you planning on contributing those tests to P3, or keep them in a separate project.
Either way, I would like to see them.
This is a good initiative that can only help.

@lopezca

lopezca commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

I'm planning on creating a separate repo for those tests only since they are not MIT. Right now they are not very useful because I have heavily modified P3 (https://github.com/panalcloud/P3/tree/local-main). If you agree, i would like to get your help on getting that into your repository, you can tell me what you're interested in and I can send you the PR's. We can chat on discord on the database channel to coordinate, take a look and let me know. I appreciate your time.

@svenvc

svenvc commented Nov 11, 2026

Copy link
Copy Markdown
Owner

That repo seems to contain a lot of changes, too much to go over for me ;-)
It makes sense to create a separate project.
If you want to propose changes for P3, I am willing to consider them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants