Refaster: fix expression matching in absence of additional syntactic context #4

rickie · 2021-09-18T07:03:46Z

@BeforeTemplate implementations of the form return someParam; are valid
constructs. With these changes such templates behave as expected, such that
someParam only matches expressions of the appropriate type, not references to
the type itself.

Stephan202

I forgot the use case we had for this; can you refresh my memory? I think it'll be important to provide context when we open a PR upstream.

rickie · 2021-10-10T13:39:47Z

Yes of course. The example that is provided in the OnlyBeforeSimpleReturnTemplate is considered a valid case Refaster template that one could write. However, without our changes the example would not run successfully.

This problem came up when we were defining the MigrationTemplates. A migration template consists of one expression in the @BeforeTemplate like in the example. Without this change, the template would not correctly match occurrences in the code.

In the end, this seemed to be the minimal example for a reproduction.

The JCTree.JCPackageDecl change prevents Refaster from going over the package statement (obviously haha). For this specific Refaster template that solves part of the problem because there were some errors when Refaster was scanning that statement (I'm not sure what the exact error was though).

The other changes in visitIdentifier prevent Refaster from thinking that String as ClassSymbol is a "normal" expression that should be matched. That should not be the case, therefore we aim to filter out JCExpressions that do not represent valid Java expressions.

Stephan202

Rebased and added one commit. Suggested commit message:

Refaster: fix expression matching in absence of additional syntactic context

`@BeforeTemplate` implementations of the form `return someParam;` are valid
constructs. With these changes such templates behave as expected, such that
`someParam` only matches expressions of the appropriate type, not references to
the type itself.

Stephan202 · 2021-12-30T10:46:22Z

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

@@ -101,7 +101,7 @@ public Void visitVariable(VariableTree node, Context context) {

  @Override
  public Void scan(Tree tree, Context context) {
-    if (tree == null) {
+    if (tree == null || tree instanceof JCTree.JCPackageDecl) {


Ideally we'd do this:

Suggested change

if (tree == null || tree instanceof JCTree.JCPackageDecl) {

if (tree == null || tree instanceof PackageTree) {

But PackageTree is marked @since 9. Perhaps we should wait for (or contribute) the changes necessary to target JDK 11, so that we can make this nice. See google#2783 (comment).

Edit: it's just IDEA flagging this; the build does pass with this change. In light of the above I think it's fine to go ahead with this then.

Stephan202 · 2021-12-30T10:48:32Z

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

+        Symbol currentExprSymbol = ASTHelpers.getSymbol(expression);
+        if (currentExprSymbol instanceof TypeSymbol
+            || (currentExprSymbol instanceof Symbol.MethodSymbol
+                && target instanceof JCTree.JCIdent)) {


IIUC we can also use an interface here:

Suggested change

&& target instanceof JCTree.JCIdent)) {

&& target instanceof IdentifierTree)) {

Stephan202 · 2021-12-30T10:49:07Z

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

@@ -365,4 +365,9 @@ public void staticImportClassToken() throws IOException {
  public void suppressWarnings() throws IOException {
    runTest("SuppressWarningsTemplate");
  }
+
+  @Test
+  public void onlyBeforeTemplate() throws IOException {


Suggested change

public void onlyBeforeTemplate() throws IOException {

public void onlyBeforeSimpleReturnTemplate() throws IOException {

Stephan202 · 2022-01-02T10:22:58Z

...st/java/com/google/errorprone/refaster/testdata/template/OnlyBeforeSimpleReturnTemplate.java

+/**
+ * Template for handling the case where only the {@link BeforeTemplate} is specified with a simple return
+ * statement.
+ */


Suggested change

/**

* Template for handling the case where only the {@link BeforeTemplate} is specified with a simple return

* statement.

*/

/**

* Template that flags all string-typed expressions, irrespective of the syntactic context in which

* they occur.

*/

Stephan202 · 2022-01-02T10:23:08Z

...ava/com/google/errorprone/refaster/testdata/input/OnlyBeforeSimpleReturnTemplateExample.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright 2021 The Error Prone Authors.


We can now use 2022.

rickie · 2022-01-05T10:14:18Z

Upstream PR: google#2805

…context

rickie force-pushed the rossendrijver/refaster_simple_return branch from c779e6f to 11ebd54 Compare September 20, 2021 05:54

Stephan202 reviewed Oct 10, 2021

View reviewed changes

rickie requested a review from Stephan202 October 10, 2021 13:39

Stephan202 force-pushed the rossendrijver/refaster_simple_return branch from 11ebd54 to 581e166 Compare January 2, 2022 10:27

Stephan202 approved these changes Jan 2, 2022

View reviewed changes

rickie changed the title ~~Add support for simple return statement with only a BeforeTemplate~~ Refaster: fix expression matching in absence of additional syntactic context Jan 2, 2022

rickie force-pushed the rossendrijver/refaster_simple_return branch 2 times, most recently from b1adbf3 to 21bc6e2 Compare January 5, 2022 09:18

rickie mentioned this pull request Jun 7, 2022

Unify real expression in U{FreeIdent,Repeated}Test #32

Merged

rickie force-pushed the rossendrijver/refaster_simple_return branch from 3b86713 to a1fd29b Compare July 4, 2022 10:54

rickie force-pushed the rossendrijver/refaster_simple_return branch from a1fd29b to e3d86cf Compare August 8, 2022 07:07

rickie force-pushed the rossendrijver/refaster_simple_return branch from e3d86cf to 29b528a Compare September 2, 2022 08:03

rickie force-pushed the rossendrijver/refaster_simple_return branch from 29b528a to e5a7be0 Compare January 9, 2023 07:52

rickie force-pushed the rossendrijver/refaster_simple_return branch from 79366f8 to 8bab354 Compare January 16, 2023 08:04

rickie force-pushed the rossendrijver/refaster_simple_return branch from 8bab354 to d334c28 Compare January 23, 2023 08:32

rickie force-pushed the rossendrijver/refaster_simple_return branch from d334c28 to fdc96e3 Compare June 8, 2023 07:48

rickie and others added 6 commits June 19, 2023 11:56

Refaster: fix expression matching in absence of additional syntactic …

3d7acf6

…context

Unify real expression in U{FreeIdent,Repeated}Test tests

bc7f2ec

Rename and improve BugCheckers used for testing

e0cd928

Suggestions

2d859ec

Remove name field from @BugPattern

b019a44

Update copyright year in new files

d3d6b1d

rickie force-pushed the rossendrijver/refaster_simple_return branch from fdc96e3 to d3d6b1d Compare June 19, 2023 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refaster: fix expression matching in absence of additional syntactic context #4

Refaster: fix expression matching in absence of additional syntactic context #4

rickie commented Sep 18, 2021 •

edited

Stephan202 left a comment

rickie commented Oct 10, 2021

Stephan202 left a comment

Stephan202 Dec 30, 2021

Stephan202 Dec 30, 2021

Stephan202 Dec 30, 2021

Stephan202 Jan 2, 2022

Stephan202 Jan 2, 2022

rickie commented Jan 5, 2022

	if (tree == null \|\| tree instanceof JCTree.JCPackageDecl) {
	if (tree == null \|\| tree instanceof PackageTree) {

	&& target instanceof JCTree.JCIdent)) {
	&& target instanceof IdentifierTree)) {

	public void onlyBeforeTemplate() throws IOException {
	public void onlyBeforeSimpleReturnTemplate() throws IOException {

Refaster: fix expression matching in absence of additional syntactic context #4

Are you sure you want to change the base?

Refaster: fix expression matching in absence of additional syntactic context #4

Conversation

rickie commented Sep 18, 2021 • edited

Stephan202 left a comment

Choose a reason for hiding this comment

rickie commented Oct 10, 2021

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Dec 30, 2021

Choose a reason for hiding this comment

Stephan202 Dec 30, 2021

Choose a reason for hiding this comment

Stephan202 Dec 30, 2021

Choose a reason for hiding this comment

Stephan202 Jan 2, 2022

Choose a reason for hiding this comment

Stephan202 Jan 2, 2022

Choose a reason for hiding this comment

rickie commented Jan 5, 2022

rickie commented Sep 18, 2021 •

edited