Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

URL: Update IdnaTestV2 to UTS46 16.0.0 #48301

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rmisev
Copy link
Member

@rmisev rmisev commented Sep 21, 2024

Updates IdnaTestV2-parser.py for version 16.0.0 of Unicode IDNA Compatibility Processing [UTS46] and fixes other issues:

  • Adds the encoding="utf-8" parameter to file open calls to run correctly if the default system encoding is not utf-8
  • Fixes the parameter that is written to the output file: --exclude_bidi changed to --exclude-bidi

@@ -92,13 +96,11 @@ def parse(lines, exclude_ipv4_like, exclude_std3, exclude_bidi):
continue

if exclude_std3:
if re.search(r"\u2260|\u226E|\u226F|\<|\>|\$|,", to_unicode):
if re.search(r"\<|\>|\$|\?|,", to_unicode):
Copy link
Member Author

@rmisev rmisev Sep 22, 2024

Choose a reason for hiding this comment

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

  • I removed \u2260, \u226E and \u226F. because they are labeled valid starting with UTS46 15.1.0.
  • I added ? because IdnaTestV2.txt contains tests with it and it is the forbidden domain code point.
  • I am not sure if $ and , are still needed here?

Copy link
Member

@annevk annevk Nov 28, 2024

Choose a reason for hiding this comment

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

Filtering ? is the cause for many removed tests. I think I'd rather keep them. Though if they now have a non-null expected output there's a problem (note that my script currently does not show from non-parsing to parsing as changed because I've assumed that to be compatible, but here it would not be).

Copy link
Member Author

@rmisev rmisev Nov 28, 2024

Choose a reason for hiding this comment

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

IdnaTestV2.txt contains many tests labeled only with [U1] and containing ?, e.g.

xn--88e732c.xn--1-?-pzc; ≮ᡬ.σ1-?; [U1]; xn--88e732c.xn--1-?-pzc; ; ;  # ≮ᡬ.σ1-?

So they will have non-null expected output because we are ignoring U1,

Copy link
Member Author

Choose a reason for hiding this comment

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

My tests show that we can safely remove $ and , from here. Tests that contain them now have correct labels. This adds 47 new test cases.

Copy link
Member

Choose a reason for hiding this comment

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

I should have documented rationale here. I suspect , is disallowed because whatwg/url#390 is unresolved. I can't find anything for $ though. (whatwg/url#159 seems too old.)

Anyway, I would be okay with allowing them. We can always update the tests later.

For ? I guess we have to disallow it then, but that still leaves a number of removed tests. I guess we can just add a parallel file for that and we just run the compare script each time we want to update this file to see if we need to add more "discarded" tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I found a mention of the $ in whatwg/url#744

The test for ToASCII("$") is marked P1 and V6, not U1. ...

But now in UTS46 16.0.0 it is correctly marked as U1.

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@annevk
Copy link
Member

annevk commented Sep 23, 2024

Thank you for working on this, but I'm not sure this can be merged as-is.

This results in failures for both Firefox and Safari, both thought to have compliant implementations. I'm not sure I fully trust the updated tests. Especially given that we still have some open issues against the URL Standard about sorting out some IDNA issues with UTS 46.

Did you investigate the failures?

@rmisev
Copy link
Member Author

rmisev commented Sep 23, 2024

I think it's normal that about 70 tests fail with Firefox and Safari. About the same number of tests failed when I tested with my UTS46 15.1.0 implementation. There are no failed tests on my UTS46 16.0.0 implementation. I think there may be similar results with browsers when they switch to UTS46 16.0.0

@annevk
Copy link
Member

annevk commented Sep 23, 2024

That's fair, I guess the problem is that I'm not entirely convinced that the UTS 46 update is fault-free given the response we've been given to some of the issues we've raised.

@annevk
Copy link
Member

annevk commented Nov 28, 2024

This is what I'd like to do to get more clarity on the changes here (help welcome, but I suspect I can get to it):

  • Create a list of all inputs whose output changed.
  • Create a list of all removed inputs. (If there are any I think I would consider this an error and we should put those in a separate test.)
  • Create a list of all new inputs and outputs. (And then ideally cross-reference those against browser results.)

Also given Interop 2024 we won't be able to land this until somewhere early next year, probably February.

In IdnaTestV2.txt of UTS46 16.0.0 they are correctly labeled as U1.
@hsivonen
Copy link
Member

hsivonen commented Nov 28, 2024

FWIW, I have not tested this PR, but I have tested the upstream IdnaTestV2.txt in combination of IDNA back end that Firefox uses (but testing in isolation of Firefox) and ICU4X trunk, which has Unicode 16.0 data, and the upstream test passes that way.

(I had to change the expectation for the root label dot handling, but since the URL Standard does not expose verifyDnsLength to via a browser API, that difference shouldn't be visible via WPT.)

@annevk
Copy link
Member

annevk commented Nov 28, 2024

I wrote the comparison script: https://gist.github.com/annevk/189bcb339d0f5dffbd79b44bacfa7e33

There's 66 tests removed here. That seems problematic. We should keep them. Probably in a separate file. But I'm not sure how we can verify that their results are still accurate. Perhaps we can assume Henri's implementation is correct for now?

There's one input that now fails to parse whereas it used to succeed: xn--xn--a--gua.pt. That's discussed in whatwg/url#803 and seems like an improvement, although notably no browser fails on this today.

There's also lots of added tests. I guess we can assume those are correct.

Then separately we need to figure out if we can enable more tests now, e.g., BiDi. Though it wasn't quite clear if those issues were resolved now. That's probably best tracked separately though as this issue already covers a lot.

@rmisev
Copy link
Member Author

rmisev commented Nov 28, 2024

Then separately we need to figure out if we can enable more tests now, e.g., BiDi. Though it wasn't quite clear if those issues were resolved now.

According to the UTC #179 PAG report (see: "6.1 Proposals to modify CheckBidi handling (UTS 46)"), Bidi changes are planned for Unicode 17.0.

@annevk
Copy link
Member

annevk commented Nov 29, 2024

I think the changes made here make it so that encodeHostEndingCodePoints() in IdnaTestV2.window.js is no longer needed. We should instead assert/filter :, /, ?, #, and \ in the parser.

@annevk
Copy link
Member

annevk commented Nov 29, 2024

I created #49443 as a sibling PR to cover the removed tests (whose input does not contain ?). Review appreciated. Again, given Interop 2024 we'll have to wait a bit with landing this.

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

Successfully merging this pull request may close these issues.

5 participants