Most efficient way to visit references to BuildConfig.VERSION_NAME?

[BT]

Hi all,

I'm trying to write a lint rule that discourages the use of BuildConfig.VERSION_NAME in source code as it can lead to breaking changes since APK functionality/updates have no dependency on the version name...What's the best way to do this? I want to tell lint to visit all references to the static field VERSION_NAME within BuildConfig. I think that I'll want to implement `getApplicableUastTypes()` , but I'm not sure which `UElement` to pass in.

Thanks for any guidance!

[Tor Norbye]

There is special support for this scenario; simply override getApplicableReferenceNames() to return listOf("VERSION_NAME"), and then override visitReference(...) to (a) make sure the reference is within BuildConfig (use context.getEvaluator() to help with that) and (b) report the error if so. 

Here's an example from HardwareIdDetector:

    @Nullable

    @Override

    public List<String> getApplicableReferenceNames() {

        return Collections.singletonList("SERIAL");

    }

    @Override

    public void visitReference(

            @NonNull JavaContext context,

            @NonNull UReferenceExpression reference,

            @NonNull PsiElement resolved) {

        JavaEvaluator evaluator = context.getEvaluator();

        if (resolved instanceof PsiField

                && evaluator.isMemberInSubClassOf((PsiField) resolved, "android.os.Build", false)) {

            ....

            context.report(ISSUE, reference, context.getNameLocation(reference), message);

        }

    }

[Brandon]

Thanks Tor!

This is actually the first thing I tried, but for some reason visitReference(...) is not being called! Here's the code I'm working with:

public class VersionNameReferencedDetector extends Detector implements SourceCodeScanner {

    private static final String VERSION_NAME = "VERSION_NAME";

    public static final Issue VERSION_NAME_REFERENCED_ISSUE = Issue.create(
        // ID: used in @SuppressLint warnings etc
        "VersionNameReferenced",
        // Title -- shown in the IDE's preference dialog, as category headers
        // in the Analysis results window, etc
        "Referencing BuildConfig.VERSION_NAME in source code",
        // Full explanation of the issue; you can use some markdown markup
        // such as `monospace`, *italic*, and **bold**.
        "Version name is an entirely user-facing build descriptor and means " +
            "nothing with relation to the app itself. For example, while unlikely, it is " +
            "entirely possible for an app to be released with a version name of 2.0, only to " +
            "then be released with a version name of 1.0. Therefore, developers should never " +
            "reference version name for any logic, as it may lead to breaking changes. Instead, " +
            "use version code which must always increment, or find another way to achieve the " +
            "desired behavior.",
        Category.CORRECTNESS,
        // Range 1-10
        8,
        Severity.ERROR,
        // Tell lint which files to look at, and which Detector to use (this one)
        new Implementation(VersionNameReferencedDetector.class, Scope.JAVA_FILE_SCOPE));

    /**
     * Constructs a new {@link VersionNameReferencedDetector}
     */
    public VersionNameReferencedDetector() {
    }

    public static Issue[] getIssues() {
        return new Issue[]{
            VERSION_NAME_REFERENCED_ISSUE
        };

    }

    @Nullable
    @Override
    public List<String> getApplicableReferenceNames() {

        return Collections.singletonList(VERSION_NAME);
    }

    @Override
    public void visitReference(JavaContext context, UReferenceExpression reference, PsiElement referenced) {
        // Not being called!
        // ...
    }

}

And here's the test String I'm working with:

@Language("JAVA")
public static final String CODE_WITH_VERSION_NAME = "" +
    "package com.example.lint;\n" +
    "\n" +
    "import com.android.ddmlib.Log;\n" +
    "\n" +
    "public class TestClass {\n" +
    "\n" +
    "    public static void main(String[] args) {\n" +
    "        \n" +
    "        String name = BuildConfig.VERSION_NAME;\n" +
    "        Log.d(\"TAG\", name);\n" +
    "        \n" +
    "    }\n" +
    "\n" +
    "}";

Note: I'm working with lint 26.1.0

[Tor Norbye]

That looks right; there's probably something else going on. Can you verify that the detector is itself initialized and run (e.g. try setting a breakpoint in the default constructor and maybe the getApplicable* method).

Also one other thing to try is to make sure your test case setup also includes the BuildConfig class itself with the VERSION_NAME constant definition, in the same package as your test case (com.example.lint).

-- Tor

[Brandon]

So I can confirm that both the constructor and getApplicableReferenceNames() are being called. The only thing that's not being hit is visitReference(...). Regarding the BuildConfig class... Since I am literally testing with @Language("JAVA") String, does the BuildConfig class really need to exist? I could definitely see how it would need to once I actually begin checking that VERSION_NAME is being referenced from the BuildConfig class (within the visitReference(...) method), but just in my attempt to get this method to be triggered, it shouldn't matter, right? Lint doesn't care that it's not a real compiled reference, does it?

Here's my test:

public void testCodeWithVersionName() {
    lint()
        .sdkHome(sdkHome)
        .files(java(TestStrings.CODE_WITH_VERSION_NAME))
        .run()
        .expect("Warning here");
}

This approach has worked for all my other lint rules (both Java and XML)

[Brandon]

It appears that things are falling apart in UElementVisitor in visitSimpleNameReferenceExpression(USimpleNameReferenceExpression):

if (mVisitReferences) {
    val list = referenceDetectors[node.identifier]
    if (list != null) {
        val referenced = node.resolve() // <-- This returns null
        if (referenced != null) { // So the check does not pass here
            for (v in list) {
                val uastScanner = v.uastScanner
                uastScanner.visitReference(mContext, node, referenced)
            }
        }
    }
}

Even though I do indeed have a Detector, it seems I have no visitor which I would expect to also be a problem...

[Tor Norbye]

I tried implementing the same check here and it works for me; the test passes. Here's the full code for my detector, and for the unit test (there's also a registration of the issue in the issue registry).

VersionNameDetector.kt:

/*
 * Copyright (C) 2018 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.android.tools.lint.checks

import com.android.tools.lint.detector.api.Category
import com.android.tools.lint.detector.api.Implementation
import com.android.tools.lint.detector.api.Issue
import com.android.tools.lint.detector.api.JavaContext
import com.android.tools.lint.detector.api.Scope
import com.android.tools.lint.detector.api.Severity
import com.android.tools.lint.detector.api.SourceCodeScanner
import com.intellij.psi.PsiElement
import com.intellij.psi.PsiField
import org.jetbrains.uast.UReferenceExpression

class VersionNameDetector : SourceCodeScanner {
    companion object Issues {
        private val IMPLEMENTATION = Implementation(
            VersionNameDetector::class.java,
            Scope.JAVA_FILE_SCOPE
        )

        @JvmField
        val ISSUE = Issue.create(
            id = "VersionNameReferenced",
            briefDescription = "Referencing BuildConfig.VERSION_NAME in source code",
            explanation = """
                Version name is an entirely user-facing build descriptor and means nothing with \
                relation to the app itself. For example, while unlikely, it is entirely possible \
                for an app to be released with a version name of 2.0, only to then be released \
                with a version name of 1.0. Therefore, developers should never reference version \
                name for any logic, as it may lead to breaking changes. Instead, use version code \
                which must always increment, or find another way to achieve the desired behavior.""",
            category = Category.CORRECTNESS,
            priority = 8,
            severity = Severity.ERROR,
            implementation = IMPLEMENTATION
        )
    }

    override fun getApplicableReferenceNames(): List<String>? = listOf("VERSION_NAME")

    override fun visitReference(
        context: JavaContext,
        reference: UReferenceExpression,
        referenced: PsiElement
    ) {
        if (referenced is PsiField &&
            referenced.containingClass?.name == "BuildConfig"
        ) {
            // TODO: Check surrounding code in AST to see if it looks like it's being used
            // for something other than display purposes

            context.report(
                ISSUE,
                reference,
                context.getNameLocation(reference),
                "Don't reference `BuildConfig.VERSION_NAME`; use `BuildConfig.VERSION_CODE` instead"
            )
        }
    }
}

VersionNameDetectorTest.kt:

/*
 * Copyright (C) 2018 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.android.tools.lint.checks

import com.android.tools.lint.detector.api.Detector

class VersionNameDetectorTest : AbstractCheckTest() {
    override fun getDetector(): Detector {
        return VersionNameDetector()
    }

    fun testConnectivityChange() {
        lint().files(
            java(
                """
                package com.example.lint;

                import com.android.ddmlib.Log;

                public class TestClass {

                    public static void main(String[] args) {

                        String name = BuildConfig.VERSION_NAME;
                        Log.d("TAG", name);

                    }

                }"""
            ).indented(),
            java(
                """
                package com.example.lint;
                public final class BuildConfig {
                  public static final String VERSION_NAME = "1.0";
                }
            """
            ).indented()
        ).run().expect(
            """
            src/com/example/lint/TestClass.java:8: Error: Don't reference BuildConfig.VERSION_NAME; use BuildConfig.VERSION_CODE instead [VersionNameReferenced]
                    String name = BuildConfig.VERSION_NAME;
                                              ~~~~~~~~~~~~
            1 errors, 0 warnings
            """
        )
    }
}

--
You received this message because you are subscribed to the Google Groups "lint-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to lint-dev+u...@googlegroups.com.
To post to this group, send email to lint...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/lint-dev/80ca29be-216f-44b9-8a80-1f10104686ce%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Brandon]

Tor,

Thank you SO much for your consistent follow-ups. The answer was all in the tests. I did not know you could pass multiple files to the test, and once I did as per your recommendation everything worked, which makes perfect sense.

I learned a lot about lint testing today :)

Thanks again!