Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Interface method handling #2023

Merged
merged 3 commits into from
Jun 5, 2018

Conversation

krakowski
Copy link
Contributor

@krakowski krakowski commented Jun 1, 2018

Description

From FunctionalInterface's JavaDoc:

If an interface declares an abstract method overriding one of the public methods of java.lang.Object, that also does not count toward the interface's abstract method count since any implementation of the interface will have an implementation from java.lang.Object or elsewhere.

CtLambdaImpl doesn't take this into account resulting in Exceptions whenever an Interface declaring one or more of java.lang.Object's methods is used in combination with a Lambda Expression.

Related Issues

@pvojtechovsky
Copy link
Collaborator

Filip, welcome! And thank you for this high quality PR 👍

@@ -124,6 +119,9 @@ public String getSimpleName() {
lambdaExecutableMethod = lambdaTypeMethods.iterator().next();
} else {
for (CtMethod<?> method : lambdaTypeMethods) {
if (getFactory().Method().OBJECT_METHODS.contains(method)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just not sure whether we can check whether method overrides Object method this way. I guess CtMethod equals compares everythink including body of method. So this fix will probably fail on the class whose sources are in spoon model. Because this method will have body and therefore will be not equal to Object method.
What is correct and fast enough? I am not sure ...
A) to compare method signatures?
B) to use isoverriding method
C) ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the incredibly fast feedback!

I just tried using CtMethod.isOverriding.

for (CtMethod<?> method : lambdaTypeMethods) {
    if (getFactory().Method().OBJECT_METHODS.stream().anyMatch(method::isOverriding)) {
        continue;
    }
...

Unfortunately the method does not return true since the Interface is no subtype of java.lang.Object (Reference).

It seems the only solution is comparing method signatures, as you said.

@spoon-bot
Copy link
Collaborator

API changes: 1 (Detected by Revapi)

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-20180601.225315-107 / New API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-SNAPSHOT

Method was added to an interface.
Old none
New method CtMethod#hasSameSignature(CtMethod)
Breaking binary: non_breaking

@pvojtechovsky
Copy link
Collaborator

pvojtechovsky commented Jun 2, 2018

CtMethod.isOverriding.
Unfortunately the method does not return true since the Interface is no subtype of java.lang.Object

I think it is a bug, which should be fixed first (in another PR). Then we can use isOverriding here too.
@monperrus WDYT?

@pvojtechovsky
Copy link
Collaborator

The comparation of signatures doesn't work with java generics

class B<T> extends A<T> {
void m(T arg){}
}
class A<U> {
void m(U arg){}
}

here B#m overrides A#m, but they have different signatures.

So do not invest more effort on signatures, because it is probably wrong way.

@monperrus monperrus changed the title Fix Interface method handling WIP: Fix Interface method handling Jun 3, 2018
@krakowski
Copy link
Contributor Author

CtLambdaImpl now checks if the interface's methods are overriding java.lang.Object methods. Once #2025 is merged this should work.

@pvojtechovsky
Copy link
Collaborator

#2025 was merged, please do some little change in your PR so the next CI build is triggered using latest sources.

@krakowski
Copy link
Contributor Author

Thanks Pavel!

I rebased on top of master and the test succeeded on my local machine.

@krakowski krakowski changed the title WIP: Fix Interface method handling Fix Interface method handling Jun 5, 2018
@@ -403,6 +404,18 @@ public void testLambdaFilter() throws Exception {
assertHasStrings(methodNames, "m2", "m4", "m7", "m8");
}

@Test
public void testInterfaceWithObjectMethods() throws Exception {
CtInterface<?> checkPersons = factory.Interface().get(Foo.CheckPersons.class);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add contract to the first line of the test body. Something like
//contract: Lambda expression works on interfaces with methods inherited from java.lang.Object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

@pvojtechovsky pvojtechovsky merged commit a3bf6f6 into INRIA:master Jun 5, 2018
@pvojtechovsky
Copy link
Collaborator

Thank You Filip 👍

@surli surli mentioned this pull request Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants