Introduce StringFormattingCheck checkstyle rule

[pugnascotia]

The new rule Checks for calls to String#formatted(Object...) that
include format specifiers that are not locale-safe. This method always
uses the default Locale, and so for our purposes it is safer to use
String#format(Locale, String, Object...).

Note that this rule can currently only detect violations when calling
formatted() on a string literal or text block. In theory, it could be
extended to detect violations in local variables or statics.

[elasticmachine]

Pinging @elastic/es-delivery (Team:Delivery)

[mark-vieira]

Wouldn't it be simpler to just use forbidden apis for this? We'd just have to add the method signature here.

[pugnascotia]

It would be simpler, yes, but the .formatted() method is nice to have and in most cases won't cause a problem. I thought it would be a shame to ban it outright.

[mark-vieira]

It would be simpler, yes, but the .formatted() method is nice to have and in most cases won't cause a problem. I thought it would be a shame to ban it outright.

How would that differ to this checkstyle rule? In what scenarios are we allowing it's use? You can always use @SuppressForbidden for one-off scenarios.

[pugnascotia]

This PR arose from #80751 where we're moving to text blocks in a lot of places, and at present it uses formatted in a lots of places.

There are 2 ways of looking at this:

1. Since .formatted(...) is equivalent to String.format(String, Object...), we should just ban it, since the latter is already banned. The existing ban is reasonable since there's an alternative.
2. Alternatively, since there's no .formatted(...) that accepts a locale, permit safe uses and warn about unsafe uses.

I suppose a problem with allowing .formatted(...) is that it raises the question of whether we should also have a rule that allows String.format(String, Object...). I'm inclined to say now, but I confess that feels arbitrary.

Basically, I like the flow and succinctness of being able to do e.g. "Hello, %s!".format("world") instead of String.format(Locale.ROOT, "Hello, %s!", "world"). But I accept that this is an inconsistent position.

[mark-vieira]

permit safe uses and warn about unsafe uses

Right, how do we determine what is a "safe" usage though. Does the current checkstyle rule take that into account?

[mark-vieira]

Maybe just banning it production code but allowing it in test code is a good compromise. I suspect we intend to use it in tests much more often, and the terseness will make reading tests considerably easier so there's considerable benefit there.

[pugnascotia]

permit safe uses and warn about unsafe uses

Right, how do we determine what is a "safe" usage though. Does the current checkstyle rule take that into account?

@arteam and I both went through an exercise of using all the format specifier with a locale known to be problematic, and this checkstyle rule checks for those format specifiers that will break.

I think banning in prod and allowing in test is a good compromise, I'll adjust this PR accordingly.

[pugnascotia]

I've added a forbidden APIs signature in es-server-signatures.txt, so that it doesn't apply to test code.

Am I right in saying that checkstyle runs before forbiddenApis? I wondered about swapping them around, so that someone doesn't spent time fixing a Checkstyle violation, only to then hit a forbidden API error.

[mark-vieira]

Am I right in saying that checkstyle runs before forbiddenApis?

There's no explicit ordering between them so far as I understand. We don't want to make checkstyle run after forbiddenApis because the latter requires compiling the code and the former does not. Such an ordering rule would now make checkstyle wait on code compilation to finish which would reduce build performance.

[pugnascotia]

OK, so what do we think - OK to merge? @arteam WDYT?

[mark-vieira]

Do we still need this check if we have added the method to forbidden APIs? Isn't this redundant?

[pugnascotia]

Well I only banned it in non-test code, which is what I thought you were suggesting?

[arteam]

We still want to prevent the user to use %d or %f with formatting in test code, because it can fail on some random East Asian locale.

[arteam]

The check looks good to me!

[mark-vieira]

We still want to prevent the user to use %d or %f with formatting in test code, because it can fail on some random East Asian locale.

Ah, ok. I wasn't aware the check was that specific. Roger that.

[arteam]

Some practical examples:

|  Welcome to JShell -- Version 17.0.1
|  For an introduction type: /help intro

jshell> Arrays.stream(Locale.getAvailableLocales())
   ...>         .map(locale -> Map.entry(locale, String.format(locale, "%d", 93425)))
   ...>         .sorted(Map.Entry.<Locale, String>comparingByValue().reversed()
   ...>                 .thenComparing(e -> e.getKey().toString()))
   ...>         .forEach(e -> System.out.printf("%16s - %s%n", e.getKey(), e.getValue()));
             sat - ᱙᱓᱔᱒᱕
          sat_IN - ᱙᱓᱔᱒᱕
    sat_IN_#Olck - ᱙᱓᱔᱒᱕
      sat__#Olck - ᱙᱓᱔᱒᱕
              my - ၉၃၄၂၅
           my_MM - ၉၃၄၂၅
     my_MM_#Mymr - ၉၃၄၂၅
              dz - ༩༣༤༢༥
           dz_BT - ༩༣༤༢༥
     dz_BT_#Tibt - ༩༣༤༢༥
th_TH_TH_#u-nu-thai - ๙๓๔๒๕
              as - ৯৩৪২৫
           as_IN - ৯৩৪২৫
     as_IN_#Beng - ৯৩৪২৫
              bn - ৯৩৪২৫
           bn_BD - ৯৩৪২৫
     bn_BD_#Beng - ৯৩৪২৫
           bn_IN - ৯৩৪২৫
             mni - ৯৩৪২৫
          mni_IN - ৯৩৪২৫
    mni_IN_#Beng - ৯৩৪২৫
      mni__#Beng - ৯৩৪২৫
              mr - ९३४२५
           mr_IN - ९३४२५
     mr_IN_#Deva - ९३४२५
              ne - ९३४२५
           ne_IN - ९३४२५
           ne_NP - ९३४२५
     ne_NP_#Deva - ९३४२५
              sa - ९३४२५
           sa_IN - ९३४२५
     sa_IN_#Deva - ९३४२५
              fa - ۹۳۴۲۵
           fa_AF - ۹۳۴۲۵
           fa_IR - ۹۳۴۲۵
     fa_IR_#Arab - ۹۳۴۲۵
              ks - ۹۳۴۲۵

[arteam]

I've merged #80751, let's check this rule on the new changes!

[arteam]

@elasticmachine update branch

[pugnascotia]

The new rule found some problems, which I've addressed.

[arteam]

Thank you!

[pugnascotia]

@elasticmachine run elasticsearch-ci/eql-correctness

[pugnascotia]

Unsurprisingly, I also had to make some changes for the new forbidden API config.

@mark-vieira Back to you.

[mark-vieira]

LGTM

[jkakavas]

Can this be backported to 8.0 ? We need it to resolve #81804

[pugnascotia]

@jkakavas now backported in 4d9f96b.