Handle retaining explicit formatting characters + other fixes #92

Manishearth · 2022-12-20T01:51:08Z

Built on top of #91. PR starts at "Add all fixups for retaining explicits".

Fixes #89 by doing the implementation hacks specified in section 5.2 Retaining BNs and Explicit Formatting Characters.

This fixes all of the character tests! (fixes #90). ~~We still have to fix some of the "basic" tests, which are unfortunately not as nicely labeled as the character tests.~~ This also fixes the basic tests (fixes #8 )

This got mixed in with a bunch of other fixes since a lot of the code is nearby, a bunch of them fix things from N0. The main Big Fix in here besides the one around retaining explicit formatting is that the code now always handles lookahead/lookbehind by iterating within the run sequence only.

All in all, this PR contains:

Fixes for Retaining BNs and Explicit Formatting Characters (Handle removed explicit formatting characters #89)
A fix for a spec bug in BD16
A typo/indexing fix from my work in Implement N0 (bracket matching) #85 (oops)
A sequence of changes so that all lookahead/lookbehind works within the run sequence only.
A bunch of rule ordering fixes in the W rules (Rules W1–W6 incorrectly applied in a single pass #8)

This handles all of the fixups from https://www.unicode.org/reports/tr9/#Retaining_Explicit_Formatting_Characters

This uses a BD16 that does *not* match the spec but does match conformance tests, the reference implementation, and ICU4C. BD16 does not explicitly state this, but it intends to ignore overridden brackets.

whoops, got the indices wrong here

All of the N and W algorithms apply within an isolating run sequence, which may have gaps that contain other meaningful characters. We shoudl skip these.

These helpers make it easy to do lookaround within an isolating run sequence.

The N0 check for enclosed strong characters should only check within the run sequence.

Manishearth · 2022-12-21T04:43:42Z

r? @eggrobin or @sffc

Note for reviewers: Should be reviewed commit-by-commit. If you review the first two commits, you need not review #91, just let me know and I can merge that.

Looking for spec review from @eggrobin, Rust code review from @sffc , and ideally "is this the best way to implement this" from both.

Manishearth · 2022-12-21T04:48:35Z

We also probably should fuzz this crate to catch panics, and maybe also fuzz it against the reference implementation once all tests pass.

sffc

seems plausible; what type of review are you looking for?

Manishearth · 2022-12-21T17:38:46Z

@sffc I'm hoping @eggrobin can do spec review, and you can do general Rusty code review as well as "is this the best way to implement this step" review.

FWIW If you review everything here including the first two commits I don't need to get review on #91.

(expanded my r? comment to say so)

We use lookaround in a bunch of other places. Use the new helpers there too

sffc

My reaction is that iter_forwards_from and iter_backwards_from seem to be good abstractions, but they still give you UTF-8 indices, yes? So if you want to "flip" a whole code point from one level or property to another, you need to overwrite all the bytes for that code point, right?

sffc · 2022-12-22T16:21:09Z

src/explicit.rs

@@ -47,13 +47,17 @@ pub fn compute(
            RLE | LRE | RLO | LRO | RLI | LRI | FSI => {
                let last_level = stack.last().level;

+                // <https://www.unicode.org/reports/tr9/#Retaining_Explicit_Formatting_Characters>
+                levels[i] = last_level;


I'm not super happy that all this code uses [] but you have another issue to follow up on that (fuzzing this crate to make sure it doesn't panic).

Yeah the whole crate does this

sffc · 2022-12-22T16:39:52Z

src/implicit.rs

+                if *class != BN {
+                    break;
+                }
+                *class = class_to_set;


You're only setting one element; is this intentional, or do you need to set multiple elements? Is processing_classes in UTF-8 or UTF-32 indices?

The loop makes this unnecessary; we are maintaining the property of every byte having its classes set so we don't have to worry about this. The existing code does this in many spots. I do want to clean this up by aggressively commenting or by building better abstractions. (Or potentially storing Option<BidiClass> or something)

Everything is in UTF-8 indices.

… applied

Fixes servo#8

Manishearth · 2022-12-28T02:15:36Z

And all tests pass, now. Bonus fix for #8

Manishearth · 2022-12-29T19:42:42Z

kick CI

src/explicit.rs

src/implicit.rs

Forgot to include variable renaming

Manishearth force-pushed the retain-explicit branch 5 times, most recently from 52a4aac to 98c05a8 Compare December 20, 2022 06:22

This was referenced Dec 20, 2022

Analysis of failing character tests (after #85) #90

Closed

Implement N0 (bracket matching) #85

Merged

Manishearth force-pushed the retain-explicit branch 4 times, most recently from 756acdf to 7fdfacb Compare December 21, 2022 04:23

Manishearth marked this pull request as ready for review December 21, 2022 04:24

Manishearth added 6 commits December 20, 2022 20:28

Add all fixups for retaining explicits

3f9abfe

This handles all of the fixups from https://www.unicode.org/reports/tr9/#Retaining_Explicit_Formatting_Characters

Use fixed BD16 where overridden brackets are ignored

c2ea9e5

This uses a BD16 that does *not* match the spec but does match conformance tests, the reference implementation, and ICU4C. BD16 does not explicitly state this, but it intends to ignore overridden brackets.

Fix identify_bracket_pairs bug

aeacc02

whoops, got the indices wrong here

Only look for brackets in run sequence

e48010a

All of the N and W algorithms apply within an isolating run sequence, which may have gaps that contain other meaningful characters. We shoudl skip these.

Add run sequence iteration helpers

1b6b668

These helpers make it easy to do lookaround within an isolating run sequence.

Use run sequence iteration for N0 enclosed check

ad09eef

The N0 check for enclosed strong characters should only check within the run sequence.

Manishearth force-pushed the retain-explicit branch from 7fdfacb to e4213aa Compare December 21, 2022 04:34

Manishearth changed the title ~~Handle retaining explicit formatting characters~~ Handle retaining explicit formatting characters + other fixes Dec 21, 2022

sffc reviewed Dec 21, 2022

View reviewed changes

Manishearth added 2 commits December 21, 2022 18:53

use sequence iteration for everything else

66872e3

We use lookaround in a bunch of other places. Use the new helpers there too

Handle retaining explicit formatting characters L1 step

a20bc1c

Manishearth force-pushed the retain-explicit branch from ae876ce to a20bc1c Compare December 22, 2022 02:53

sffc reviewed Dec 22, 2022

View reviewed changes

Manishearth added 2 commits December 27, 2022 14:55

Address spec bug around updating the class of BNs

d9fa9fc

Address fixup for X10

d07f173

Manishearth force-pushed the retain-explicit branch from ed6b0eb to d07f173 Compare December 27, 2022 23:00

Fix BN skipping in N1/N2, and end-of-sequence BNs in W6

eb21078

Manishearth force-pushed the retain-explicit branch from 13a694b to eb21078 Compare December 27, 2022 23:35

Manishearth added 3 commits December 27, 2022 15:43

Handle NSMs in ET runs

b85ba08

Make sure W1 is using the previous class from before other rules were…

4bc16af

… applied

Break up weak processing into multiple steps

f7cfe98

Fixes servo#8

Manishearth closed this Dec 29, 2022

Manishearth reopened this Dec 29, 2022

Manishearth mentioned this pull request Jan 4, 2023

Fix W2 rule ordering; behavior of multibyte characters in W4 #91

Merged

eggrobin reviewed Jan 10, 2023

View reviewed changes

src/explicit.rs Outdated Show resolved Hide resolved

Manishearth added 6 commits January 10, 2023 09:36

fix comments

ebdfc77

link to l2

ef6b74c

Fix up X7

4f5dd11

comment cleanup

1c21d4c

fmt

065f7a1

cmt

ab1b26a

Manishearth force-pushed the retain-explicit branch from 5f00e3f to ab1b26a Compare January 17, 2023 17:04

eggrobin approved these changes Jan 18, 2023

View reviewed changes

src/implicit.rs Outdated Show resolved Hide resolved

src/implicit.rs Show resolved Hide resolved

src/implicit.rs Outdated Show resolved Hide resolved

better names

c9487b5

Manishearth merged commit 64611a5 into servo:master Jan 18, 2023

Manishearth deleted the retain-explicit branch January 18, 2023 20:10

Manishearth added a commit that referenced this pull request Jan 18, 2023

Uncommitted change from #92

74f7f13

Forgot to include variable renaming

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle retaining explicit formatting characters + other fixes #92

Handle retaining explicit formatting characters + other fixes #92

Manishearth commented Dec 20, 2022 •

edited

Loading

Manishearth commented Dec 21, 2022 •

edited

Loading

Manishearth commented Dec 21, 2022

sffc left a comment

Manishearth commented Dec 21, 2022 •

edited

Loading

sffc left a comment

sffc Dec 22, 2022

Manishearth Dec 22, 2022

sffc Dec 22, 2022

Manishearth Dec 22, 2022

Manishearth commented Dec 28, 2022

Manishearth commented Dec 29, 2022

Handle retaining explicit formatting characters + other fixes #92

Handle retaining explicit formatting characters + other fixes #92

Conversation

Manishearth commented Dec 20, 2022 • edited Loading

Manishearth commented Dec 21, 2022 • edited Loading

Manishearth commented Dec 21, 2022

sffc left a comment

Choose a reason for hiding this comment

Manishearth commented Dec 21, 2022 • edited Loading

sffc left a comment

Choose a reason for hiding this comment

sffc Dec 22, 2022

Choose a reason for hiding this comment

Manishearth Dec 22, 2022

Choose a reason for hiding this comment

sffc Dec 22, 2022

Choose a reason for hiding this comment

Manishearth Dec 22, 2022

Choose a reason for hiding this comment

Manishearth commented Dec 28, 2022

Manishearth commented Dec 29, 2022

Manishearth commented Dec 20, 2022 •

edited

Loading

Manishearth commented Dec 21, 2022 •

edited

Loading

Manishearth commented Dec 21, 2022 •

edited

Loading