ICU-23431 Add link detection/formatting tools#4048
Conversation
Very rough first cut
|
All, I think it is ready for review; it passes the UTS58 test data (with 3 cases noted in the description). I plan on further cleanup (mostly unused code), but suggest doing than in a subsequent PR. |
|
Also, I wasn't quite sure about the conventions for where to put files nowadays (It's be a long time since I contributed to ICU!) |
richgillam
left a comment
richgillam
left a comment
There was a problem hiding this comment.
I got to about line 200 of LinkHandlingUtilities before I gave up. My comments may or may not be helpful, but I don't think I ever really got into the meat of this class. Maybe I'm not a good reviewer for this stuff because all my Java experience is woefully out of date and I'm completely unfamiliar with more modern facilities and idioms. Maybe it's also that I chose to read this PR from top to bottom, so I was looking at utility classes before I ever got to the stuff that actually uses those utility classes. All I know is that I fairly quickly had no idea what was going on.
I think you probably want at least one reviewer more conversant in Java than I am at this point, such as @yumaoka , @echeran , or @mihnita .
| PATH(QUERY, "[?\\^`\\{\\}]"), | ||
|
|
||
| /** | ||
| * path and<br> |
There was a problem hiding this comment.
These are pasted in from the documentation in https://url.spec.whatwg.org/#percent-encoded-bytes just as a guide to the meaning.
The javadoc reads:
path and
U+002F (/), U+003A (:), U+003B (;), U+003D (=), U+0040 (@), U+005B ([) to U+005D (]), inclusive, and U+007C (|).
it is referring to the two arguments of the enum constructor, the path and a bunch of characters (in a UnicodeSet, so requiring \ quotes in Java).
USERINFO(PATH, "[/:;=@\[-\]|]"),
| Hard("[\\p{whitespace}\\p{NChar}[\\p{C}-\\p{Cf}]\\p{deprecated}]"), | ||
| Soft("[\\p{Term}\\p{lb=qu}-\\p{deprecated}]"), | ||
| Close("[\\p{Bidi_Paired_Bracket_Type=Close}[>]-\\p{deprecated}]"), | ||
| Open("[\\p{Bidi_Paired_Bracket_Type=Open}[<]-\\p{deprecated}]"), |
There was a problem hiding this comment.
Maybe it's just me, but I had a hard time following all the brackets and subtractions in the Unicode set descriptions above. I think the addition of the ASCII < and > signs kind of exacerbated that. I wonder if parentheses can be used to clarify this, or constants that add the < and > symbols to the paired-bracket stuff they're always used with, or (failing all that) textual descriptions can be added?
There was a problem hiding this comment.
that has a comment /** Defines the LinkTermination property TODO use real property */
It will replaces by calls on real ICU properties, once they are there.
The UnicodeSet syntax does get a bit ugly in Java because of the double escapes.
"[\\p{Bidi_Paired_Bracket_Type=Close}[>]-\\p{deprecated}]" is
\p{Bidi_Paired_Bracket_Type=Close}
union
[>]
set-difference
\p{deprecated}
That copies the spec for the derivation, but will be moot once we have real properties.
There was a problem hiding this comment.
This might be easier to read (opinion), and allows comments:
String s2 = "["
+ "\\p{Bidi_Paired_Bracket_Type=Close}"
+ "[>]"
+ "-" // set difference
+ "\\p{deprecated}"
+ "]"
;| return cp == '>' ? '<' : UCharacter.getBidiPairedBracket(cp); | ||
| } | ||
|
|
||
| /** Defines the LinkTermination property TODO use real property */ |
There was a problem hiding this comment.
Are the properties you refer to in the TODOs real Unicode properties? Does Markus's recent Unicode 18 update add them?
There was a problem hiding this comment.
They are added to the extended UCD in version 18, but are not yet supported in ICU
| Include(null), // all else | ||
| ; | ||
|
|
||
| public final UnicodeSet base; |
There was a problem hiding this comment.
This is always going to be the empty set, right? What does this declaration get you?
There was a problem hiding this comment.
The base is set by the constructor, just below.
Enums in java have more features than in many other languages, with methods and fields.
| } else { | ||
| this.base = new UnicodeSet(uset).freeze(); | ||
| SOFAR.addAll(this.base); | ||
| } |
There was a problem hiding this comment.
If I'm reading this class right, there's an undocumented order dependency here, where this constructor will only do the right thing if Include is the last constant in the enum. Is this okay, and should it at least be documented? And is SOFAR used anywhere else, or is it just an implementation detail of the construction of LinkTermination? If it's just used here, why is it defined so far away? And should the name be in all caps when it doesn't appear to actually be a constant?
There was a problem hiding this comment.
This is a big ugly, ’tis true. Will disappear with the real properties.
It is basically to all the 'base' to be final in the enum. The code accumulates all the other values and then complements then for the final INCLUDE - which is basically "every other code point"
| PROPERTY_MAP.putAll(lt.base, lt); | ||
| } | ||
| PROPERTY_MAP.freeze(); | ||
| } |
There was a problem hiding this comment.
LinkTermination is hard-coded and should always construct the same thing, right? Is this consistency check something you want to be doing at run time?
There was a problem hiding this comment.
This is left over from the unicodetools code that has to derive the values. It will be replaced once we have a real property, but is a nice check to have in the meantime.
| new UnicodeSet("[[a-zA-Z][0-9][_ \\- ! ? ' \\{ \\} * / \\& # % ` \\^ + = | ~ \\$]]") | ||
| .add('.') | ||
| .freeze(); | ||
| static final UnicodeSet validEmailLocalPart = |
There was a problem hiding this comment.
Shouldn't both this variable and the one right above it have all-caps names to indicate they're constants? Is EMAIL_ASCII_INCLUDES used in other places, or is it only part of the process of building validEmailLocalPart? If the latter, maybe consolidate them?
There was a problem hiding this comment.
Yes, I changed the second to be all upper.
I hadn't marked these as temporary; added a comment.
/**
* These will be replaced once we have a real property value in ICU.
*/
There was a problem hiding this comment.
I hadn't marked these as temporary; added a comment.
I've seen in other comments "will replace when we have real properties"
What do you think about adding some TODOs, that are easier to grep?
|
|
||
| static final UnicodeSet EMAIL_ASCII_INCLUDES = | ||
| new UnicodeSet("[[a-zA-Z][0-9][_ \\- ! ? ' \\{ \\} * / \\& # % ` \\^ + = | ~ \\$]]") | ||
| .add('.') |
There was a problem hiding this comment.
Why is the period being added separately?
There was a problem hiding this comment.
Just to be very obvious. I have the comment above.
// We add dot (ascii '.'), and then check after for the special dot constraints.
| } | ||
|
|
||
| Structure(String sub, String prefix) { | ||
| this(sub, prefix, null, null); |
There was a problem hiding this comment.
So you're putting prefix in the sub2 spot? That feels weird...
There was a problem hiding this comment.
changed the fields to be:
public final String sub;
public final String prefix;
public final String sub2; // only used for Query, with 2 values
public final String prefix2;
Is that clearer?
| single, | ||
| listP("/", "𝑷"), | ||
| listF(":~:", "𝑭"), // directive | ||
| listQ2("&", "𝑸", "=", "𝑽"); |
There was a problem hiding this comment.
Do the math alphanumeric characters in the constants above have some kind of special meaning?
There was a problem hiding this comment.
Yes, they are used in the test data, to stand out clearly.
|
Thanks for your feedback.
I'll review on more detail tomorrow, but a quick note:
The property construction stuff is temporary, until Markus can add access
to the real properties. He is stretched right now and may not be able to
handle that before release.
That being said, I can move all of that into a separate file to make it
clearer -- and do some other reorganization and commenting to make it all
easier to read.
…On Wed, Jul 8, 2026, 03:56 Rich Gillam ***@***.***> wrote:
***@***.**** commented on this pull request.
I got to about line 200 of LinkHandlingUtilities before I gave up. My
comments may or may not be helpful, but I don't think I ever really got
into the meat of this class. Maybe I'm not a good reviewer for this stuff
because all my Java experience is woefully out of date and I'm completely
unfamiliar with more modern facilities and idioms. Maybe it's also that I
chose to read this PR from top to bottom, so I was looking at utility
classes before I ever got to the stuff that actually uses those utility
classes. All I know is that I fairly quickly had no idea what was going on.
I think you probably want at least one reviewer more conversant in Java
than I am at this point, such as @yumaoka <https://github.com/yumaoka> ,
@echeran <https://github.com/echeran> , or @mihnita
<https://github.com/mihnita> .
------------------------------
In
icu4j/main/core/src/main/java/com/ibm/icu/impl/links/LinkHandlingUtilities.java
<#4048 (comment)>:
> + FRAGMENT(C0, "[\\ \"<>`]"),
+
+ /** C0 and U+0020 SPACE, U+0022 ("), U+0023 (#), U+003C (<), and U+003E (>) */
+ QUERY(C0, "[\\ \"<>]"),
+
+ /** query percent-encode set and U+0027 ('). Used for http://... */
+ SPECIAL_QUERY(QUERY, "[']"),
+
+ /**
+ * query percent-encode set and<br>
+ * U+003F (?), U+005E (^), U+0060 (`), U+007B ({), and U+007D (}).
+ */
+ PATH(QUERY, "[?\\^`\\{\\}]"),
+
+ /**
+ * path and<br>
Missing space?
------------------------------
In
icu4j/main/core/src/main/java/com/ibm/icu/impl/links/LinkHandlingUtilities.java
<#4048 (comment)>:
> + private WHATWG_PERCENT_ENCODED(WHATWG_PERCENT_ENCODED base, String uset) {
+ set = new UnicodeSet(uset).addAll(base == null ? UnicodeSet.EMPTY : base.set).freeze();
+ }
+ }
+
+ /** TODO use real property */
+ private static int getOpening(int cp) {
+ return cp == '>' ? '<' : UCharacter.getBidiPairedBracket(cp);
+ }
+
+ /** Defines the LinkTermination property TODO use real property */
+ private enum LinkTermination {
+ Hard("[\\p{whitespace}\\p{NChar}[\\p{C}-\\p{Cf}]\\p{deprecated}]"),
+ Soft("[\\p{Term}\\p{lb=qu}-\\p{deprecated}]"),
+ Close("[\\p{Bidi_Paired_Bracket_Type=Close}[>]-\\p{deprecated}]"),
+ Open("[\\p{Bidi_Paired_Bracket_Type=Open}[<]-\\p{deprecated}]"),
Maybe it's just me, but I had a hard time following all the brackets and
subtractions in the Unicode set descriptions above. I think the addition of
the ASCII < and > signs kind of exacerbated that. I wonder if parentheses
can be used to clarify this, or constants that add the < and > symbols to
the paired-bracket stuff they're always used with, or (failing all that)
textual descriptions can be added?
------------------------------
In
icu4j/main/core/src/main/java/com/ibm/icu/impl/links/LinkHandlingUtilities.java
<#4048 (comment)>:
> + */
+ COMPONENT(USERINFO, "[\\$-\\&+,]");
+
+ public final UnicodeSet set;
+
+ private WHATWG_PERCENT_ENCODED(WHATWG_PERCENT_ENCODED base, String uset) {
+ set = new UnicodeSet(uset).addAll(base == null ? UnicodeSet.EMPTY : base.set).freeze();
+ }
+ }
+
+ /** TODO use real property */
+ private static int getOpening(int cp) {
+ return cp == '>' ? '<' : UCharacter.getBidiPairedBracket(cp);
+ }
+
+ /** Defines the LinkTermination property TODO use real property */
Are the properties you refer to in the TODOs real Unicode properties? Does
Markus's recent Unicode 18 update add them?
------------------------------
In
icu4j/main/core/src/main/java/com/ibm/icu/impl/links/LinkHandlingUtilities.java
<#4048 (comment)>:
> +
+ /** TODO use real property */
+ private static int getOpening(int cp) {
+ return cp == '>' ? '<' : UCharacter.getBidiPairedBracket(cp);
+ }
+
+ /** Defines the LinkTermination property TODO use real property */
+ private enum LinkTermination {
+ Hard("[\\p{whitespace}\\p{NChar}[\\p{C}-\\p{Cf}]\\p{deprecated}]"),
+ Soft("[\\p{Term}\\p{lb=qu}-\\p{deprecated}]"),
+ Close("[\\p{Bidi_Paired_Bracket_Type=Close}[>]-\\p{deprecated}]"),
+ Open("[\\p{Bidi_Paired_Bracket_Type=Open}[<]-\\p{deprecated}]"),
+ Include(null), // all else
+ ;
+
+ public final UnicodeSet base;
This is always going to be the empty set, right? What does this
declaration get you?
------------------------------
In
icu4j/main/core/src/main/java/com/ibm/icu/impl/links/LinkHandlingUtilities.java
<#4048 (comment)>:
> + Hard("[\\p{whitespace}\\p{NChar}[\\p{C}-\\p{Cf}]\\p{deprecated}]"),
+ Soft("[\\p{Term}\\p{lb=qu}-\\p{deprecated}]"),
+ Close("[\\p{Bidi_Paired_Bracket_Type=Close}[>]-\\p{deprecated}]"),
+ Open("[\\p{Bidi_Paired_Bracket_Type=Open}[<]-\\p{deprecated}]"),
+ Include(null), // all else
+ ;
+
+ public final UnicodeSet base;
+
+ private LinkTermination(String uset) {
+ if (uset == null) { // only called with Include, the "none of the above" option
+ this.base = SOFAR.complement().freeze();
+ } else {
+ this.base = new UnicodeSet(uset).freeze();
+ SOFAR.addAll(this.base);
+ }
If I'm reading this class right, there's an undocumented order dependency
here, where this constructor will only do the right thing if Include is
the last constant in the enum. Is this okay, and should it at least be
documented? And is SOFAR used anywhere else, or is it just an
implementation detail of the construction of LinkTermination? If it's
just used here, why is it defined so far away? And should the name be in
all caps when it doesn't appear to actually be a constant?
------------------------------
In
icu4j/main/core/src/main/java/com/ibm/icu/impl/links/LinkHandlingUtilities.java
<#4048 (comment)>:
> + if (pv1.base.containsSome(pv2.base)) {
+ throw new IllegalArgumentException(
+ "Values in LinkTermination overlap! "
+ + pv1
+ + ", "
+ + pv2
+ + ": "
+ + new UnicodeSet(pv1.base).retainAll(pv2.base));
+ }
+ }
+ }
+ for (LinkTermination lt : values()) {
+ PROPERTY_MAP.putAll(lt.base, lt);
+ }
+ PROPERTY_MAP.freeze();
+ }
LinkTermination is hard-coded and should always construct the same thing,
right? Is this consistency check something you want to be doing at run time?
------------------------------
In
icu4j/main/core/src/main/java/com/ibm/icu/impl/links/LinkHandlingUtilities.java
<#4048 (comment)>:
> + for (LinkTermination lt : values()) {
+ PROPERTY_MAP.putAll(lt.base, lt);
+ }
+ PROPERTY_MAP.freeze();
+ }
+ }
+
+ // https://www.rfc-editor.org/rfc/rfc5322.html#section-3.2.3 has the full list for ASCII part
+ // See also https://en.wikipedia.org/wiki/Email_address#Local-part
+ // We add dot (ascii '.'), and then check after for the special dot constraints.
+
+ static final UnicodeSet EMAIL_ASCII_INCLUDES =
+ new UnicodeSet("[[a-zA-Z][0-9][_ \\- ! ? ' \\{ \\} * / \\& # % ` \\^ + = | ~ \\$]]")
+ .add('.')
+ .freeze();
+ static final UnicodeSet validEmailLocalPart =
Shouldn't both this variable and the one right above it have all-caps
names to indicate they're constants? Is EMAIL_ASCII_INCLUDES used in
other places, or is it only part of the process of building
validEmailLocalPart? If the latter, maybe consolidate them?
------------------------------
In
icu4j/main/core/src/main/java/com/ibm/icu/impl/links/LinkHandlingUtilities.java
<#4048 (comment)>:
> + }
+ }
+ for (LinkTermination lt : values()) {
+ PROPERTY_MAP.putAll(lt.base, lt);
+ }
+ PROPERTY_MAP.freeze();
+ }
+ }
+
+ // https://www.rfc-editor.org/rfc/rfc5322.html#section-3.2.3 has the full list for ASCII part
+ // See also https://en.wikipedia.org/wiki/Email_address#Local-part
+ // We add dot (ascii '.'), and then check after for the special dot constraints.
+
+ static final UnicodeSet EMAIL_ASCII_INCLUDES =
+ new UnicodeSet("[[a-zA-Z][0-9][_ \\- ! ? ' \\{ \\} * / \\& # % ` \\^ + = | ~ \\$]]")
+ .add('.')
Why is the period being added separately?
------------------------------
In
icu4j/main/core/src/main/java/com/ibm/icu/impl/links/LinkHandlingUtilities.java
<#4048 (comment)>:
> + private enum Structure {
+ single,
+ listP("/", "𝑷"),
+ listF(":~:", "𝑭"), // directive
+ listQ2("&", "𝑸", "=", "𝑽");
+ public final String sub;
+ public final String sub2;
+ public final String prefix;
+ public final String prefix2;
+
+ Structure() {
+ this(null, null, null, null);
+ }
+
+ Structure(String sub, String prefix) {
+ this(sub, prefix, null, null);
So you're putting prefix in the sub2 spot? That feels weird...
------------------------------
In
icu4j/main/core/src/main/java/com/ibm/icu/impl/links/LinkHandlingUtilities.java
<#4048 (comment)>:
> + }
+ String localPart = source.subSequence(result, domainStart - 1).toString();
+ if (localPart.startsWith(".") || localPart.endsWith(".") || localPart.contains("..")) {
+ return domainStart;
+ }
+ if (source.toString().substring(0, result).endsWith("mailto:")) {
+ result -= "mailto:".length();
+ }
+ return result;
+ }
+
+ private enum Structure {
+ single,
+ listP("/", "𝑷"),
+ listF(":~:", "𝑭"), // directive
+ listQ2("&", "𝑸", "=", "𝑽");
Do the math alphanumeric characters in the constants above have some kind
of special meaning?
—
Reply to this email directly, view it on GitHub
<#4048?email_source=notifications&email_token=ACJLEMAZGOV3RR5AORYCAY35DWL5TA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRUHE4TCMBXGU32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4649910757>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACJLEMC5R6RNX3IXCPGDVK35DWL5TAVCNFSNUABEKJSXA33TNF2G64TZHM2DSMRUGQ3TMNR3JFZXG5LFHM2DONJXHA4DIMJTGCQXMAQ>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/ACJLEMANB7YQTQOXHPQFVVT5DWL5TA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRUHE4TCMBXGU32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/ACJLEMABVTZ4EBI4OFFDDJT5DWL5TA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRUHE4TCMBXGU32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Maybe it's enough to just say that in a TODO comment (or maybe you already did)... |
| */ | ||
| COMPONENT(USERINFO, "[\\$-\\&+,]"); | ||
|
|
||
| public final UnicodeSet set; |
|
|
||
| /** Defines the LinkTermination property TODO use real property */ | ||
| private enum LinkTermination { | ||
| Hard("[\\p{whitespace}\\p{NChar}[\\p{C}-\\p{Cf}]\\p{deprecated}]"), |
There was a problem hiding this comment.
Uppercase (HARD, SOFT, ...)
There was a problem hiding this comment.
not really important, since this will be replaced by real properties. but did it anyway
| * Based on https://url.spec.whatwg.org/#percent-encoded-bytes. Is the default for maximal | ||
| * encoding, but can be customized | ||
| */ | ||
| private enum WHATWG_PERCENT_ENCODED { |
There was a problem hiding this comment.
Camel case
WhatwgPercentEncoded or WhatWgPercentEncoded
| Include(null), // all else | ||
| ; | ||
|
|
||
| public final UnicodeSet base; |
| toAppendTo.append(fixed); | ||
| } | ||
|
|
||
| /** We are guaranteed that string is all percent escaped utf8, %a3%c0 ... */ |
There was a problem hiding this comment.
Is everything escaped, including the ASCII range?
Because if not (for example "%a3%c0A%a3%c1") then I can see problems.
There was a problem hiding this comment.
It is used in the following:
int start = toEscape.span(source, lastEnd, SpanCondition.NOT_CONTAINED);
appendCheckingPercent(result, source, lastEnd, start);
if (start == source.length()) {
break;
}
So it is checking for just certain characters.
| @@ -179,6 +192,7 @@ public Entry<K, V> next() { | |||
| Entry<K, Set<V>> e = it1.next(); | |||
| entry.key = e.getKey(); | |||
| it2 = e.getValue().iterator(); | |||
| entry.value = it2.next(); | |||
| } | |||
| return entry; | |||
| } | |||
| @@ -195,8 +209,8 @@ public void remove() { | |||
| } | |||
|
|
|||
| private static class ReusableEntry<K, V> implements Entry<K, V> { | |||
| K key; | |||
| V value; | |||
| private K key; | |||
| private V value; | |||
|
|
|||
| @Override | |||
| public K getKey() { | |||
| @@ -314,6 +328,10 @@ public Splitter(char c) { | |||
| this(Pattern.compile("\\Q" + c + "\\E")); | |||
| } | |||
|
|
|||
| public Splitter(String s) { | |||
| this(Pattern.compile("\\Q" + s + "\\E")); | |||
| } | |||
|
|
|||
| public Splitter(Pattern p) { | |||
| pattern = p; | |||
| } | |||
| @@ -322,6 +340,10 @@ public static Splitter on(char c) { | |||
| return new Splitter(c); | |||
| } | |||
|
|
|||
| public static Splitter on(String sub) { | |||
| return new Splitter(sub); | |||
| } | |||
|
|
|||
| public static Splitter on(Pattern p) { | |||
| return new Splitter(p); | |||
| } | |||
| @@ -336,6 +358,10 @@ public List<String> splitToList(String input) { | |||
| return Arrays.asList(items); | |||
| } | |||
|
|
|||
| public Stream<String> splitToStream(String input) { | |||
| return splitToList(input).stream(); | |||
There was a problem hiding this comment.
splitToList creates an array, and from that it creates a list, to create a stream here.
We can create a stream directly:
return pattern.splitAsStream(input);There was a problem hiding this comment.
This is old code, and I don't want to touch it except for errors.
There was a problem hiding this comment.
I'm talking about public Stream<String> splitToStream(String input) { ... }, which GitHub shows as new method.
It calls splitToList which does a pattern.split to an array, uses that to build a List, and finally we make a stream from it.
pattern.splitAsStream(input) makes a stream directly.
| } | ||
|
|
||
| /** Utilities because regular java doesn't have some nice Guava features */ | ||
| public static <T> Set<T> SetofOrdered(T... source) { |
There was a problem hiding this comment.
| public static <T> Set<T> SetofOrdered(T... source) { | |
| public static <T> Set<T> setOfOrdered(T... source) { |
| /** Not as good as Guava, since it makes a new set instead of a view */ | ||
| public static <T> Set<T> setsDifference(Set<T> set1, Set<T> set2) { | ||
| return set1.stream() | ||
| .filter(element -> !set2.contains(element)) |
There was a problem hiding this comment.
Optional:
| .filter(element -> !set2.contains(element)) | |
| .filter(Predicate.not(set2::contains)) |
|
I'd like to get this merged. |
mihnita
left a comment
mihnita
left a comment
There was a problem hiding this comment.
There are a few nitpicks left, but if there is a rush then it's good enough.
3 participants
ICU-23431
Notes
john..smith.comdoes not detect as john.smith.comjohn..smith@example.comis not detected as john..smith@example.comChecklist