Skip to content

Add in-house Bidi segmentation#251

Merged
andydotxyz merged 21 commits into
mainfrom
bidi
Apr 25, 2026
Merged

Add in-house Bidi segmentation#251
andydotxyz merged 21 commits into
mainfrom
bidi

Conversation

@benoitkugler

Copy link
Copy Markdown
Contributor

We currently rely on the semi standard x/text/unicode/bidi package for Bidi segmentation. It has served us quite well so far, but :

  1. The implementation does not pass the Unicode conformance tests
  2. The package seems not maintained anymore
  3. Some performance optimization regarding allocations could be made quite easily

See golang/go#71809, golang/go#69819 and golang/go#67098

I propose to include a fork of the x/text/unicode/bidi in our repo, as implemented in this branch. The API is simplified and the case of []rune as input is supported. This implementation passes the Unicode conformance tests.

Also, it exposes the Bidi level, which will probably be required to properly reorder nested bidi texts (see #249).

What do you think ?

@whereswaldon whereswaldon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is quite the PR. I see the logic of vendoring an implementation of this. I agree that upstream isn't really working on it, and you're quite right that it's impossible for us to fix our handling of embedding levels without exposing them.

Thanks for tackling this.

@benoitkugler

Copy link
Copy Markdown
Contributor Author

(Waiting for @andydotxyz review since there is a significant API addition.)

@andydotxyz

Copy link
Copy Markdown
Contributor

Apologies, I have been snowed under.
Is there anything risk of apps that might have a transitive dependency on the old bidi? (Beyond compiling two similar packages).

In some situations I have had to provide a bridge implementation so that apps can "replace" to get the new code under the old import - but that may have been CGo problem or complications we don't have to worry about here...

@benoitkugler

benoitkugler commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Apologies, I have been snowed under. Is there anything risk of apps that might have a transitive dependency on the old bidi? (Beyond compiling two similar packages).

In some situations I have had to provide a bridge implementation so that apps can "replace" to get the new code under the old import - but that may have been CGo problem or complications we don't have to worry about here...

I don't expect such limations, especially since our use of x/text/bidi was completely internal. Your are right however that there will likely be two copies of similar code in the binary, if the user performs directional string manipulations.
It is also likely that, since we do not import bidi anymore, a new version of it would be selected if a user upgrade. But because of Go compatibility promise, that should not break anything.

@andydotxyz andydotxyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thanks for that and thinking through the implications too.

@andydotxyz andydotxyz merged commit d036223 into main Apr 25, 2026
14 checks passed
@andydotxyz andydotxyz deleted the bidi branch April 25, 2026 09:58
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.

3 participants