[java] Future-proof DontImportJavaLangRule

[mpellegrini]

Please, prefix the PR title with the language it applies to within brackets, such as [java] or [apex]. If not specific to a language, you can use [core]

Before submitting a PR, please check that:

• The PR is submitted against master. The PMD team will merge back to support branches as needed.
• mvn test passes.
• mvn checkstyle:check passes. Check this for more info

PR Description:
In Java 7+ there was a new class added under the java.lang package. Specifically java.lang.invoke.MethodHandles.

MethodHandles provides a handy utility in which to obtain the current class name amongst other things. A technique that is gaining in popularity is to use MethodHandles to obtain the class name when getting a logger, for example

private final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

However, using this triggers a PMD violation

<?xml version="1.0" encoding="UTF-8"?>
<pmd version="5.5.1" timestamp="2017-04-07T07:19:26.058">
<file name="...">
<violation beginline="18" endline="18" begincolumn="1" endcolumn="38" rule="DontImportJavaLang" ruleset="Import Statements" package="..." externalInfoUrl="https://pmd.github.io/pmd-5.5.1/pmd-java/rules/java/imports.html#DontImportJavaLang" priority="4">
Avoid importing anything from the package java.lang
</violation>
</file>
</pmd>

This pull request adds java.lang.invoke. to the existing exceptions to this rule to when using Java 7+.

Resolves #338
Resolves #339

[jsotuyod]

Thanks for the PR, you are absolutely right!

We should probably just rework this to be more future-proof... maybe checking how many dots are on the import starting with java.lang. 2 is bad, 3 or more is ok (either an inner package, inner class, or static member import).

[mpellegrini]

I completely agree with making this more future-proof. I was initially just trying to fit into the current approach. I was testing out a different approach that seems to incorporate the rules you mentioned:

If this looks more in line with what you were thinking I can update this pull request again.

public class DontImportJavaLangRule extends AbstractJavaRule {
    private static final String IMPORT_JAVA_LANG = "java.lang.";

    @Override
    public Object visit(ASTImportDeclaration node, Object data) {

        if (node.isStatic()) {
            return data;
        }

        String img = node.jjtGetChild(0).getImage();
        if (img.startsWith(IMPORT_JAVA_LANG)) {
            String substring = img.substring(IMPORT_JAVA_LANG.length());

            boolean isClass = Character.isUpperCase(substring.charAt(0));

            if (isClass && substring.contains(".")) {
                return data;
            } else if (!substring.equals("*") && substring.contains(".")) {
                return data;
            }
            addViolation(data, node);

        }
        return data;
    }
}

[jsotuyod]

Amazing! I would do it slightly different, as to avoid string allocations.

    if (img.startsWith(IMPORT_JAVA_LANG)) {
            if (img.indexOf('.', IMPORT_JAVA_LANG.length() + 1) != -1) {
                // Importing from a subpackage / inner class
                return data;
            }
            addViolation(data, node);
        }

I would greatly appreciate if you checked this passes all tests and update the PR.

[mpellegrini]

Nice improvement of the code. Will get this updated pull request out shortly. Thanks!!

[jsotuyod]

this extra dot at the end is causing a couple tests to fail on Travis

[jsotuyod]

Looks good, just one more small fix before we get a green light and merge it :)

[mpellegrini]

There were actually two bugs, I had already fixed locally the change from "java.lang." -> "java.lang" but the logic is still failing on the following type of import:

import java.lang.ref.*

This is because the evaluation of the img var is seeing 'java.lang.ref'. So it looks like some further work is needed to determine if this is a class or package as just looking for the '.' is enough. Will investigate further and do another push with a fix.

[jsotuyod]

I see, yes, for those cases the image is java.lang.ref, but the import's isImportOnDemand() is set to true, just have to make sure not to mix it with java.lang.* which would still be a violation.

If you need any assistance on this just let me know.

[mpellegrini]

Awesome, the isImportOnDemand() was quite helpful. One question for you the original code would fail the following (note: there is not a test case for this in master)

'import java.lang.ProcessBuilder.*;'

But I am think this should be ok and the new code I have does not flag a violation. What do you think, should this be a violation or not?

[jsotuyod]

@mpellegrini yes, now you mention it, the rule reporting on import java.lang.ProcessBuilder.*; would be a False Positive! Make sure to include a test for that scenario.

[jsotuyod]

@mpellegrini beautiful! Got rid of two separate false positives and made this future proof all in one PR. Thanks for the amazing job!

I'll try to get this merged ASAP. Will definitely make it into PMD 5.6.0

[jsotuyod]

Merged! Thanks for the amazing PR! Expect this in PMD 5.6.0

[mpellegrini]

Thanks @jsotuyod. Glad I was able to contribute!!