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

[java] New rule: Format strings must match the arguments passed and their count #2187

Open
linusjf opened this issue Dec 25, 2019 · 12 comments
Labels
a:new-rule Proposal to add a new built-in rule

Comments

@linusjf
Copy link

linusjf commented Dec 25, 2019

Affects PMD Version:
6.20.0
Rule:
New rule.

Description

SonarSource has an existing rule for Java that verifies argument types and their count for formatted string specifiers lest a runtime-error occurs when incorrectly coded.

https://rules.sonarsource.com/java/RSPEC-3457

I've had this occur a couple of times while coding.
This rule, however, will not cover instances when the format specifiers are dynamically created or loaded from an input file such as a ResourceBundle.

Note: AFAIK, this rule exists neither in Checkstyle nor Spotbugs.

Note: A similar rule for log messages is InvalidLogMessageFormat.

Code Sample demonstrating the issue:
See link above.

Java-API-Methods to be checked:

  • java.lang.String.format
  • java.io.PrintWriter.format
  • java.io.PrintStream.format
  • java.io.PrintWriter.printf
  • java.io.PrintStream.printf

Similar to InvalidLogMessageFormat:

  • java.util.Formatter.format
  • java.text.MessageFormat.format

Running PMD through: [CLI | Ant | Maven | Gradle | Designer | Other]

@linusjf linusjf changed the title [Java] Format strings must match the arguments passed and their count. [Java] New rule: Format strings must match the arguments passed and their count. Dec 25, 2019
@adangel adangel changed the title [Java] New rule: Format strings must match the arguments passed and their count. [java] New rule: Format strings must match the arguments passed and their count Jan 6, 2020
@adangel adangel added the a:new-rule Proposal to add a new built-in rule label Jan 6, 2020
@jscancella
Copy link

This also happens for me in the following code logger.debug(messages.getString("fetch_allowed"), builder.isFetchAllowed()); where messages is the ResourceBundle. This needs to be smart enough to fetch the correct string from the resource bundle and then evaluate the formatting.

@linusjf
Copy link
Author

linusjf commented Jul 23, 2020

@jscancella As far as I know, this can only be done on a best efforts basis with the resource bundles available in the classpath or auxiliary classpath. I presume you wish to read in all the language resource bundles available.

Frankly, I don't think this additional requirement can be accommodated.

@jscancella
Copy link

@Fernal73 well in my case I only have 1 language currently, but even with that it fails to find it because I am getting this error.

@linusjf
Copy link
Author

linusjf commented Jul 24, 2020

See above edit.

@jscancella
Copy link

jscancella commented Jul 24, 2020

Frankly, I don't think this additional requirement can be accommodated.

But this is the standard way of dealing with internationalization https://docs.oracle.com/en/java/javase/14/intl/internationalization-overview.html#GUID-75FA808F-CA61-4C31-87C5-BD5C12DA5945
Could you explain why it can't be accommodated? There is a standard naming convention, and the file is just a list of key value pairs or comments.
If you aren't going to support this standard Java feature, it should at least only check when there is inline String.

@linusjf
Copy link
Author

linusjf commented Jul 24, 2020 via email

@jscancella
Copy link

jscancella commented Jul 24, 2020

I also don't know the PMD API, but how I access it in my project is java.util.ResourceBundle.getBundle("MessageBundle"), also the standard would be to put it in src/main/resources/MessageBundle.properties and to put the language specific identifier as defined in https://docs.oracle.com/javase/7/docs/api/java/util/Locale.html
So for example MessageBundle_en_GB.properties would be english for great briton

Being able to list the resource bundle files would be a good start and would allow for others that are not following the default standards of naming convention.

@linusjf
Copy link
Author

linusjf commented Jul 24, 2020

https://dzone.com/articles/resource-bundle-tricks-and

This will need another rule to reinforce it; one that enforces unique keys across families of ResourceBundles.

And another to ensure that keys are the same within a family of ResourceBundles.

@linusjf
Copy link
Author

linusjf commented Jul 24, 2020

If you aren't going to support this standard Java feature, it should at least only check when there is inline String.

This is for the PMD team of maintainers to decide. I do not arbite which issues will be addressed and when.

@linusjf
Copy link
Author

linusjf commented Jul 30, 2020

https://checkerframework.org/manual/#formatter-checker

https://checkerframework.org/manual/#i18n-formatter-checker

The Checker framework provides the desired functionality at the byte code level.

Personally , I prefer not to add dependencies in the source code unless absolutely necessary i.e., unnecessary imports. Test classes, not so much.

I don't mind hearing arguments---pro and against.

@linusjf
Copy link
Author

linusjf commented Jun 22, 2021

A simpler version of this rule could be implemented where the string format is always expected to be a string literal much like
format-security warning in C/C++.

@linusjf
Copy link
Author

linusjf commented Jun 23, 2021

Another feature that could be added while supporting multiple languages via resource bundles is to validate that resource keys match in value and number across bundles and that format specifier(s) count for each matching resource string is the same across bundles. This, of course, implies that a resource bundle (usually English) is the master reference and validation flows from this. Do I need to illustrate this or is my language clear enough to comprehend?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:new-rule Proposal to add a new built-in rule
Projects
None yet
Development

No branches or pull requests

3 participants