OPENNLP-1935: Regex removal (10/10): Forbid java.util.regex in production code via forbiddenapis

[krickert]

Part 10 of the regex removal series. This is the last of them and will remain in draft (this comment will be edited before we say it's ready as well).

This ticket provides a forbiddenapis execution checks the production classes of every module against dev/forbidden-regex.txt: java.util.regex, String.split, String.matches, String.replaceAll, String.replaceFirst and Scanner.useDelimiter. It runs at process-classes, before the tests, and its message names the StringUtil replacements. Tests are not checked, so test code can keep using regex.

Exceptions: a member with a documented public contract that is a user-supplied regular expression is marked with @SuppressForbidden(reason). Only RegexNameFinder and RegexNameFinderFactory have it. A private pattern is converted, not annotated, so the check's report is the to-fix list for the rest of the series. CONTRIBUTING.md has the rule.

OPENNLP-1935

[rzo1]

Little time, so here is a GPT 5.6-sol review instead for now

No additional blocking findings. The guard configuration passes targeted validation. Merge after the implementation PRs and their fixes.

Validation across the combined stack: 1,856 targeted tests, zero failures, one skipped.

[rzo1]

Here are some additional comments. I think forbiddenapis would be the bigger gain here than checkstyle. It is already configured in the parent pom, it checks compiled bytecode instead of source text, and it closes every gap listed in item 1.

Blocking

1. checkstyle.xml:83-88: use forbiddenapis instead of a textual checkstyle rule. The NoRegexStringCall pattern works on source text, so it misses and wrongly flags calls. Tested on a probe production file:

   • Missed (these do use a regex): split(" +"), split("[,;]"), split(SEP_CONSTANT), s.matches(regexVar), a .matches( wrapped onto the next line, fully qualified java.util.regex.Pattern.compile(..), and Scanner.useDelimiter("\\s*,\\s*"). The old BasicContextGenerator line o.split(separator) was not reported either.
   • Flagged by mistake: List/Map.replaceAll(String::trim) (not regex), split("\\.") (the JDK splits it without a Pattern, so "compile a Pattern on each call" is wrong), and a string literal containing .replaceAll(.

   forbiddenapis (de.thetaphi:forbiddenapis, pom.xml:398-418, already bound to validate with check and testCheck) avoids all of that:

   • It resolves method references in bytecode, so constants, variables, fully qualified names and line wraps are all covered, and List.replaceAll is not flagged.
   • Exceptions are marked per member with a @SuppressForbidden annotation (any annotation with that simple name works), not per file. A new regex added later to Parse or NameSample still fails the build, and checkstyle-suppressions.xml plus the ${checkstyle.suppressions.file} property go away.
   • The error names the exact signature and the reason text from the signatures file.

   Sketch: split the existing execution so the new signatures apply to production classes only.

   <execution>
     <id>forbidden-regex-main</id>
     <phase>validate</phase>
     <goals><goal>check</goal></goals>
     <configuration>
       <signaturesFiles>
         <signaturesFile>${maven.multiModuleProjectDirectory}/dev/forbidden-regex.txt</signaturesFile>
       </signaturesFiles>
     </configuration>
   </execution>

   @defaultMessage Use an explicit scan or a StringUtil helper instead of java.util.regex (OPENNLP-1935)
   java.util.regex.**
   java.lang.String#split(java.lang.String)
   java.lang.String#split(java.lang.String,int)
   java.lang.String#matches(java.lang.String)
   java.lang.String#replaceAll(java.lang.String,java.lang.String)
   java.lang.String#replaceFirst(java.lang.String,java.lang.String)
   java.util.Scanner#useDelimiter(java.lang.String)
   

   Trade-off: bytecode can't see the argument, so one-character splits like split(",") (which the JDK handles without a Pattern) are flagged too. Either convert them to the StringUtil helpers this series adds, or annotate them. For the classes whose contract is a user-supplied regex (RegexNameFinder, the tokenizer factories, the deprecated finder shim), put @SuppressForbidden on those members.

2. checkstyle-suppressions.xml:57-58. TokenizerME is a stale entry: it has no java.util.regex import on this head. Remove it.

3. checkstyle-suppressions.xml:28-29 vs :40-50. The header says a class is listed "only when regular expressions are its documented contract". That holds for RegexNameFinder/RegexNameFinderFactory, the tokenizer factory classes and the deprecated AbstractClassPathModelFinder shim, but not for four classes whose patterns are private and hard-coded:

   • Parse (typePattern, funTypePattern, tokenPattern; ^([^ =-]+) is a trivial scan on the treebank read path)
   • NameSample (START_TAG_PATTERN)
   • SpellCheckingCharSequenceNormalizer (URL_LIKE)
   • AncoraSpanishHeadRules (fixed tags like "AQA.*", "GRUP\\.A", i.e. prefix and literal tests)

   Convert them (sibling or follow-up JIRA), or change the header and give a real reason for each; "Penn Treebank constituent markup grammar" explains nothing.

4. checkstyle-suppressions.xml, AbstractClassPathModelFinder comment. "subclasses written for them see no change" is false: the deprecated asRegex shim in OPENNLP-1932: Regex removal (4/10): Match model finder wildcards without regular expressions #1278 returns a different regex than 2.x (details in the OPENNLP-1932: Regex removal (4/10): Match model finder wildcards without regular expressions #1278 review). Correct the comment.

5. Stacking. This branch carries cherry-picked copies of OPENNLP-1930: Regex removal (2/10): Parse Arvores Deitadas markup with cursor scans #1276–OPENNLP-1934: Add explicit tokenizer character policies #1281 (52 commits over OPENNLP-1928: Regex removal (1/8): Replace trivial patterns with character scans in StringUtil #1275), not the sibling commits. Keep it draft, rebase it onto main once the siblings are merged so the diff is only checkstyle.xml, checkstyle-suppressions.xml and pom.xml, and re-copy any change requested on a sibling until then.

Minor

• checkstyle.xml:70-73 + pom.xml:306. ${checkstyle.suppressions.file} with optional=false only resolves when the Maven plugin sets it, so loading checkstyle.xml directly (e.g. CheckStyle-IDEA) now fails. Inline the exceptions as SuppressionSingleFilter modules or document the property. Goes away with forbiddenapis (item 1).
• checkstyle-suppressions.xml:37-65. Whole-file suppressions let any new regex added later to Parse, NameSample, TokenizerFactory etc. pass unnoticed. Also solved by per-member @SuppressForbidden (item 1).
• checkstyle-suppressions.xml:61-66. When asRegex/matchesPattern are removed, this entry goes stale silently. Reference the removal JIRA.
• checkstyle.xml:80-81, :87-88. The messages don't point to the policy or JIRA and give no route for a justified exception. Name the concrete StringUtil helpers instead of "the StringUtil helpers", and drop "compile a Pattern on each call" for single-character splits.
• Conventions. Once the dev@ thread concludes, add the rule to the Code Conventions page or CONTRIBUTING.md: why, how to write a scan instead, and how to get an exception.
• AncoraSpanishHeadRules:121-124. The comment "instead of via String.matches(), which would otherwise recompile..." is design history. Trim it in whichever sibling it comes from.

Verified:

• In a worktree of this head, mvn -o -q checkstyle:check@validate exits 0. The same config on OPENNLP-1928: Regex removal (1/8): Replace trivial patterns with character scans in StringUtil #1275 reports 23 sites (11 imports, 12 string calls), all of them fixed by the siblings, which backs up the description as far as those files go.
• The test exclusion regex works, src/jmh is not scanned, and every build module is covered.
• The remaining unlisted production uses are split with a one-character literal or constant, which the JDK handles without a Pattern.

[krickert]

Dependency note: #1302 (OPENNLP-1955) supplies the sanctioned replacement for literal String.split. Once it and #1279 are in, a forbiddenapis signature for java.lang.String#split can be added here.

[rzo1]

Needs rebase and PR/issue re-runging (working).

[rzo1]

This isn't a stacked PR and has > 100 commits + 9k additions. It is totally unclear to me what this is about.

[krickert]

This isn't a stacked PR and has > 100 commits + 9k additions. It is totally unclear to me what this is about.

The JIRA was confusing, I made it normal. The title is right - 3 files to prevent regex in the future from sneaking in. I think it's a good idea. This is the last of the regex stack, don't want to cause a false positive on the errors so it was stacked.

Once they come in it'll look normal. I'll send an email to explain

[krickert]

We're getting closer to 10, so I got around to cleaning this up. It'll be red by design. We can do a last merge once all of 'em are rung out

[rzo1]

@krickert Please rebase onto main. #1302 is in now, so the java.lang.String#split signatures can be added here and the remaining split cases moved to the StringUtil helpers.

While at it: EventFields.split, MascIdentifiers.splitOnXmlWhitespace, MecabDictionary.splitWhitespace and HunspellDictionary.split are the same "split on runs of separator characters, no empty fields" loop with a different character test. Please move that into StringUtil once instead of four copies.

And again, please push your branches to apache/opennlp instead of the ai-pipestream fork. That fork is really bad for collaboration: maintainer edits don't work on organization-owned forks, so we can't push even a trivial fix or a rebase. Every small change turns into a patch in a comment and another round trip. With the branch on apache/opennlp I'd have just done this rebase myself.

[krickert]

Rebased onto main. StringUtil now has one non-empty split loop behind splitNonEmpty, splitOnUnicodeWhitespace and a new mode-aware splitOnWhitespace; EventFields, MascIdentifiers, MecabDictionary and HunspellDictionary call those instead of their own copies. The String#split signatures were already in the list. The three remaining calls in ConlluStream, TokenSampleStream and DownloadUtil use the helpers now; the one in BasicContextGenerator goes with #1280.

With that, the guard on main reports the classes still to do: the tokenizer factories and TokenizerME, the parser family (Parse, AncoraSpanishHeadRules, ParserTool), NameSample, FeatureGeneratorUtil, the two normalizers, plus the rest of ConlluStream, DownloadUtil and TokenSampleStream. Those are the next PRs in the series, so this stays a draft until they land.

On the fork: understood. This branch is on apache/opennlp from here on.

Continued in #1348.