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

Review: bug(*): fix method resolution for inner interfaces #4008

Merged
merged 5 commits into from
Jun 29, 2021

Conversation

MartinWitt
Copy link
Collaborator

@MartinWitt MartinWitt commented Jun 27, 2021

Currently, spoon fails to resolve methods in a strange edge case, with inner interfaces. Using CtType#getAllExecutable on the following type produced a spoon.SpoonException: Cannot create MethodTypingContext for method declared in different ClassTypingContext

public class BarBaz implements FooBar.Crashy {

  @Override
  public String foo() {
    return "bar!";
  }

}

In java, all nested interfaces are implicitly static. This fixes the method CtType#getAllExecutables and handles now inner interfaces correctly as implicitly static. I'm unsure about the location of the test case, and it's contract. Any suggestions?

This bug and the minimal example was discovered/created by @I-Al-Istannen

@MartinWitt MartinWitt force-pushed the bug/extendStaticInnerInterface branch from 90e2818 to c4487c7 Compare June 27, 2021 05:44
This fixes the method CtType#getAllExecutables and handles inner now
interfaces correctly as implicitly static.

Co-authored-by: I-Al-Istannen <[email protected]>
@MartinWitt MartinWitt force-pushed the bug/extendStaticInnerInterface branch from c4487c7 to 86615d5 Compare June 27, 2021 06:01
@MartinWitt MartinWitt changed the title WIP: bug(*): fix method resolution for inner interfaces Review: bug(*): fix method resolution for inner interfaces Jun 27, 2021
Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

Hi @MartinWitt!

The fix LGTM, but I think the test is a bit unclear in purpose. See what you think of my comments.

As this is a test of CtType.getAllExecutables(), I would place the test in the CtTypeTest test class.

src/test/java/spoon/test/method/MethodTest.java Outdated Show resolved Hide resolved
src/test/java/spoon/test/method/MethodTest.java Outdated Show resolved Hide resolved

@Test
public void testGetAllMethodsInnerClassExtended() {
// contract: implicit static nested interfaces are correct handled in getAllExecutables and dont throw an error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Depending on whether you want the test to verify that there's no crash, or that the correct amount of executables are returned, I would update the contract to state that.

Like I mentioned, invoking a method in a test implicitly tests that it does not crash, and so if you test anything in addition to the fact that the method does not crash (e.g. that it returns the expected amount of executables), the contract does not need to state that it tests that the method doesn't crash. That was a messy sentence but I hope you understand what I mean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was a crash while parsing the JDK. I wanted to include a real assertion to guarantee that it's not only not crashing, but working correctly. But you are correct, "Does not crash" is not a useful assertion if we can check if the result is correct.

Copy link
Collaborator

@slarse slarse left a comment

Choose a reason for hiding this comment

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

Looks good!

@slarse slarse merged commit d3f21f4 into INRIA:master Jun 29, 2021
@monperrus monperrus mentioned this pull request Aug 19, 2021
woutersmeenk pushed a commit to woutersmeenk/spoon that referenced this pull request Aug 29, 2021
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.

2 participants