Refaster: fix expression matching in absence of additional syntactic context

core/src/main/java/com/google/errorprone/refaster/RefasterScanner.java

@@ -28,6 +28,7 @@
 import com.sun.source.tree.DoWhileLoopTree;
 import com.sun.source.tree.IfTree;
 import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.PackageTree;
 import com.sun.source.tree.ParenthesizedTree;
 import com.sun.source.tree.SynchronizedTree;
 import com.sun.source.tree.Tree;
@@ -98,7 +99,7 @@ public Void visitVariable(VariableTree node, Context context) {
 
   @Override
   public Void scan(Tree tree, Context context) {
-    if (tree == null) {
+    if (tree == null || tree instanceof PackageTree) {
       return null;
     }
     JCCompilationUnit compilationUnit = context.get(JCCompilationUnit.class);

core/src/main/java/com/google/errorprone/refaster/UFreeIdent.java

@@ -23,6 +23,8 @@
 import com.sun.source.tree.Tree;
 import com.sun.source.util.TreeScanner;
 import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Symbol.TypeSymbol;
 import com.sun.tools.javac.tree.JCTree.JCExpression;
 import com.sun.tools.javac.util.Names;
 import javax.annotation.Nullable;
@@ -99,7 +101,14 @@ public Boolean visitIdentifier(IdentifierTree ident, Void v) {
       if (!isGood) {
         return Choice.none();
       } else if (currentBinding == null) {
+        Symbol currentExprSymbol = ASTHelpers.getSymbol(expression);
+        if (currentExprSymbol instanceof TypeSymbol
+            || (currentExprSymbol instanceof MethodSymbol && target instanceof IdentifierTree)) {
+          // The `JCExpression` by itself does not represent a valid Java expression.
+          return Choice.none();
+        }
         unifier.putBinding(key(), expression);
+
         return Choice.of(unifier);
       } else if (currentBinding.toString().equals(expression.toString())) {
         // TODO(lowasser): try checking types here in a way that doesn't reject

core/src/test/java/com/google/errorprone/refaster/TemplateIntegrationTest.java

@@ -374,4 +374,9 @@ public void suppressWarnings() throws IOException {
   public void typeArgumentsMethodInvocation() throws IOException {
     runTest("TypeArgumentsMethodInvocationTemplate");
   }
+
+  @Test
+  public void onlyBeforeSimpleReturnTemplate() throws IOException {
+    runTest("OnlyBeforeSimpleReturnTemplate");
+  }
 }

core/src/test/java/com/google/errorprone/refaster/UFreeIdentTest.java

@@ -17,10 +17,18 @@
 package com.google.errorprone.refaster;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
 
 import com.google.common.testing.EqualsTester;
 import com.google.common.testing.SerializableTester;
-import com.sun.tools.javac.tree.JCTree.JCExpression;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.CompilationTestHelper;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.tools.javac.util.Context;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -41,10 +49,33 @@ public void inlinesExpression() {
 
   @Test
   public void binds() {
-    JCExpression expr = parseExpression("\"abcdefg\".charAt(x + 1)");
-    UFreeIdent ident = UFreeIdent.create("foo");
-    assertThat(ident.unify(expr, unifier)).isNotNull();
-    assertThat(unifier.getBindings()).containsExactly(new UFreeIdent.Key("foo"), expr);
+    CompilationTestHelper.newInstance(UnificationChecker.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "class A {",
+            "  public void bar() {",
+            "    int x = 0;",
+            "    \"abcdefg\".charAt(x + 1);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @BugPattern(
+      summary = "Verify that unifying the expression results in the correct binding",
+      explanation = "For test purposes only",
+      severity = SUGGESTION)
+  public static class UnificationChecker extends BugChecker implements MethodInvocationTreeMatcher {
+    @Override
+    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+      Unifier unifier = new Unifier(new Context());
+      UFreeIdent ident = UFreeIdent.create("foo");
+
+      assertThat(ident.unify(tree, unifier)).isNotNull();
+      assertThat(unifier.getBindings()).containsExactly(new UFreeIdent.Key("foo"), tree);
+
+      return Description.NO_MATCH;
+    }
   }
 
   @Test

core/src/test/java/com/google/errorprone/refaster/URepeatedTest.java

@@ -17,10 +17,18 @@
 package com.google.errorprone.refaster;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
 
 import com.google.common.testing.EqualsTester;
 import com.google.common.testing.SerializableTester;
-import com.sun.tools.javac.tree.JCTree.JCExpression;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.CompilationTestHelper;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.tools.javac.util.Context;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -31,10 +39,33 @@ public class URepeatedTest extends AbstractUTreeTest {
 
   @Test
   public void unifies() {
-    JCExpression expr = parseExpression("\"abcdefg\".charAt(x + 1)");
-    URepeated ident = URepeated.create("foo", UFreeIdent.create("foo"));
-    assertThat(ident.unify(expr, unifier)).isNotNull();
-    assertThat(unifier.getBindings()).containsExactly(new UFreeIdent.Key("foo"), expr);
+    CompilationTestHelper.newInstance(UnificationChecker.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "class A {",
+            "  public void bar() {",
+            "    int x = 0;",
+            "    \"abcdefg\".charAt(x + 1);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @BugPattern(
+      summary = "Verify that unifying the expression results in the correct binding",
+      explanation = "For test purposes only",
+      severity = SUGGESTION)
+  public static class UnificationChecker extends BugChecker implements MethodInvocationTreeMatcher {
+    @Override
+    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+      Unifier unifier = new Unifier(new Context());
+      URepeated ident = URepeated.create("foo", UFreeIdent.create("foo"));
+
+      assertThat(ident.unify(tree, unifier)).isNotNull();
+      assertThat(unifier.getBindings()).containsExactly(new UFreeIdent.Key("foo"), tree);
+
+      return Description.NO_MATCH;
+    }
   }
 
   @Test

core/src/test/java/com/google/errorprone/refaster/testdata/input/OnlyBeforeSimpleReturnTemplateExample.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright 2023 The Error Prone Authors.
+ *
+ * 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.google.errorprone.refaster.testdata;
+
+/** Test data for {@code OnlyBeforeSimpleReturnTemplate}. */
+public class OnlyBeforeSimpleReturnTemplateExample {
+  public String foo(String s) {
+    return s;
+  }
+}

core/src/test/java/com/google/errorprone/refaster/testdata/output/OnlyBeforeSimpleReturnTemplateExample.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright 2023 The Error Prone Authors.
+ *
+ * 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.google.errorprone.refaster.testdata;
+
+/** Test data for {@code OnlyBeforeSimpleReturnTemplate}. */
+public class OnlyBeforeSimpleReturnTemplateExample {
+  public String foo(String s) {
+    return /* match found */ s;
+  }
+}

core/src/test/java/com/google/errorprone/refaster/testdata/template/OnlyBeforeSimpleReturnTemplate.java

@@ -0,0 +1,30 @@
+/*
+ * Copyright 2023 The Error Prone Authors.
+ *
+ * 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.google.errorprone.refaster.testdata.template;
+
+import com.google.errorprone.refaster.annotation.BeforeTemplate;
+
+/**
+ * Template that flags all string-typed expressions, irrespective of the syntactic context in which
+ * they occur.
+ */
+public class OnlyBeforeSimpleReturnTemplate {
+  @BeforeTemplate
+  String before(String s) {
+    return s;
+  }
+}