Preserve MustImplementMethodsVisitor method type ancestors
#329
Conversation
…stract-method-types
There was a problem hiding this comment.
There are two failing integration tests:
- CF-6060 appears to be failing because exception types thrown by preserved interface methods aren't themselves preserved. I think this is an easy fix (see comment).
- CF-6030 is failing because a meta-annotation on a preserved annotation isn't being preserved. I'm not sure what that has to do with this PR, so you'll have to investigate @theron-wang
| || (overridden == null && overridesAnInterfaceMethod(method))) { | ||
| ResolvedMethodDeclaration resolvedMethod = method.resolve(); | ||
| Set<String> returnAndParamTypes = new HashSet<>(); | ||
| Map<String, ResolvedType> returnAndParamTypes = new HashMap<>(); |
There was a problem hiding this comment.
I think the values in this map also need to include thrown exceptional types. The new code exposes a problem caused by not doing that in CF-6060, which it looks like is why that integration test fails.
Yep, I believe this was a side-effect of the approach I took with this PR; basically, a previously-unincluded method which had a parent abstract method which had another parent interface method in the JDK (it's strange, sorry) was now marked to be preserved, along with its annotation. The annotation definition referenced an inner annotation which was not included due to an issue with #302 which I fixed in this PR. |
| @@ -184,6 +265,7 @@ private boolean overridesAnInterfaceMethodImpl( | |||
| for (String name : typeParametersMap.getNames()) { | |||
| String interfaceViewpointName = name.substring(name.lastIndexOf('.') + 1); | |||
| String localViewpointName = typeParametersMap.getValueBySignature(name).get().describe(); | |||
| localViewpointName = Pattern.quote(localViewpointName); | |||
There was a problem hiding this comment.
A comment justifying why quoting this is necessary will be helpful later, so that we don't accidentally delete it.
This PR addresses an issue I found in NullAway issue #103, where compile-time errors were caused by missing methods and types. The missing methods were caused by the following:
1) Methods not preserved
Foo extends (abstract) Bar extends [Some JDK interface]
If Bar contains an abstract method also present in the JDK interface, those implementations in Foo were removed while the abstract definition was maintained in Bar, causing compile-time errors. A check was added to ensure that all super abstract methods did not contain anything that needed to be preserved (i.e. when in Foo, instead of just checking Bar, we check Bar and the JDK interface).
2. Classes not preserved
If a non-target class needs its method implementations to be preserved, and some of its return/parameter types are solvable but were untracked so far by
usedTypeElements, their ancestors were not preserved. For example, if a method visited byMustImplementMethodsVisitoris of typePeekingIterator<E>which extendsIterator<E>, theextends Iterator<E>portion is kept in the original definition but its import statement was removed, leading to compile-time errors. The new approach to this is, if a type element was not previously inusedTypeElements, all of its ancestors (interfaces and super classes) would also be marked for preservation.A test case can be found in
Issue103Test.Thanks!