Update ChangeType and ChangePackage to work with SourceFileWithReference

[Laurens-W]

What's changed?

Added implementation for SourceFileWithTypeReference in ChangeType and ChangePackage recipes

Anything in particular you'd like reviewers to focus on?

Test coverage (edge cases)

Anyone you would like to review specifically?

@knutwannheden @timtebeek

Any additional context

• Add TypeReference trait to identify Java type references in XML #4587

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[knutwannheden]

Looking good already!

I added some more review comments. Most of them probably apply to both recipes.

[knutwannheden]

I think we should consider renaming this to something more generic like SymbolReference or just Reference and then have a "kind" property, which would probably be an enum like Kind with constants for Type and Namespace (or Package) for now.

That "name" property will probably also have to be complemented by a structured "symbol" property at some point (so that the caller can query a method reference for name, declaring type, and parameters). But this can wait, I think.

Thoughts?

[Laurens-W]

I agree but I also feel like this is a creep of the scope, could we log it somewhere as future improvement?

[knutwannheden]

Agreed. It would just be good if we could get the API finalized before people try to start using it.

[knutwannheden]

Ideally I think we should only be using the matches() and renameTo() methods instead of the getName() method, but coming up with a good API for those two methods feels pretty difficult, so maybe this will have to wait. I was thinking something along the lines of Java's Matcher for regex:

[knutwannheden]

One example I keep thinking about here is when we have META-INF/services files as we have them for the TypeReference.Provider interface. In this case the name of a nested type looks like foo.Bar$Baz. Now ChangeType can't possibly know that it needs to call renameTo("foo.Bar$Baz") and not renameTo("foo.Bar.Baz"). The latter is probably correct for some other config files. Currently, the ChangeType visitor would have to make sure to always pass in names like "foo.Bar$Baz" and then the TypeReference implementation would have to know if it should apply a heuristic (although Bar$Baz could even be the name of the top-level class, but that would be highly unlikely). The question is just what should the API look like to cater for this?

[knutwannheden]

I think we might as well simplify the API by removing this method. As the caller supplies the matcher that matcher can filter on the kind.

Suggested change

	public Collection<Reference> findMatches(Reference.Matcher matcher, Reference.Kind kind) {
	return findMatchesInternal(matcher, kind);
	}

Instead, we might want to provide something like a containsMatches() returning a boolean. But this isn't high priority.

[knutwannheden]

I might have missed some, but I added a comment regarding the preVisit() methods. I suggest you take a look at TreeVisitor#visit() so you understand how this mechanism works. When all you want to do is extract some information from the top-level node (or also some other node) using the preVisit() method, you should call stopAfterPreVisit() before returning.

[knutwannheden]

If we can go through all visitors and make sure preVisit() is implemented as appropriate, I think we should merge the PR.