Refaster: fix expression matching in absence of additional syntactic context

[rickie]

@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]

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]

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]

IIUC we can also use an interface here:

Suggested change

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

[Stephan202]

Suggested change

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

[Stephan202]

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]

We can now use 2022.

[rickie]

Upstream PR: google#2805