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

feat: Add CtExecutable.addParameterAt #3902

Merged
merged 7 commits into from
May 4, 2021

Conversation

algomaster99
Copy link
Contributor

@algomaster99 algomaster99 commented May 2, 2021

#3884

This PR provides an API to add a parameter in CtExecutables at a specified position. Since there are few classed which inherit from CtExecutableImpl, we should test this API for all those classes.

  • CtConstructor
  • CtMethod
  • CtLambda
    - CtAnonymousExecutable (cannot add parameters to an anonymous class in Java)

CtParameter<?> first = factory.createParameter();
CtTypeReference<String> firstType = factory.Type().stringType();
first.setSimpleName("x");
first.setType((CtTypeReference) firstType);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing the argument as is (without type casting) was throwing a compilation error.

incompatible types: spoon.reflect.reference.CtTypeReference<java.lang
.String> cannot be converted to spoon.reflect.reference.CtTypeReference<capture#1 of ?>

I was getting confused with the types as it wasn't even allowing it to type cast to CtTypeReference<String> so I just used a raw type for now. Need help resolving this :)

Copy link
Collaborator

@slarse slarse May 3, 2021

Choose a reason for hiding this comment

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

The type parameters must match between CtParameter and CtTypeReference.

CtParameter<String> first = factory.createParameter();
CtTypeReference<String> firstType = factory.Type().stringType();
first.setSimpleName("y");
first.setType(secondType);

The reason you can't cast to CtTypeReference<String> is that casting is a runtime thing, and the type parameter of a generic type is erased at compile time. That is to say, the JVM does not know the parameterization of a generic type. It's the same reason you can't do stuff like list instanceof List<String>. You can read about type erasure here. It's quite fascinating, and massively annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can read about type erasure here. It's quite fascinating, and massively annoying.

😂. I shall read about it. I remember you mentioned this concept in one of our meetings.

By the way, can you run the workflow for this PR? I will be able to resolve if anything breaks.

@algomaster99
Copy link
Contributor Author

CtAnonymousExecutable (cannot add parameters to an anonymous class in Java)

Do we need to write a test case for it? We can test if the unit test throws an Exception.

@algomaster99 algomaster99 force-pushed the add-parameter-at branch 2 times, most recently from be3d7ca to 002708a Compare May 2, 2021 14:25
@slarse
Copy link
Collaborator

slarse commented May 3, 2021

CtAnonymousExecutable (cannot add parameters to an anonymous class in Java)

Do we need to write a test case for it? We can test if the unit test throws an Exception.

That's an @UnsettableProperty, right? Not sure if those are typically tested, @monperrus?

Also, @monperrus approve workflow run please :)

@algomaster99
Copy link
Contributor Author

That's an @UnsettableProperty, right?

Yes. Let me know if they are still tested. Logically speaking, the test should be written (if it does not exist) for the decorator itself so we don't need to repeat it here.

@algomaster99 algomaster99 changed the title wip: feat: Add CtExecutable.addParameterAt feat: Add CtExecutable.addParameterAt May 3, 2021
@algomaster99 algomaster99 requested a review from slarse May 3, 2021 09:31
@slarse
Copy link
Collaborator

slarse commented May 3, 2021

FYI waiting for workflow run approval from @monperrus before review.

@monperrus
Copy link
Collaborator

run approved!

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.

This looks excellent to me.

The overall architecture has the same problem with code duplication as mentioned in #3896, but it's not really the place of either of these PRs to fix that. So I think we should merge this, and deal with the code duplication (which was there to begin with) separately.

@monperrus I vote for merge.

@monperrus
Copy link
Collaborator

Thanks @algomaster!

In Spoon, we have one comment line per test that states the test intention, the contract that is being tested. By convention, it starts with // contract: eg

// contract: adding an element to the list increases its size

Could you add this contract line in each new test?

@algomaster99
Copy link
Contributor Author

@monperrus I added the contract for each test case in this PR.

@monperrus
Copy link
Collaborator

Thanks a lot. LGTM, will merge.

@monperrus monperrus merged commit 3b81204 into INRIA:master May 4, 2021
@monperrus
Copy link
Collaborator

Thanks a lot for your contribution!

@slarse
Copy link
Collaborator

slarse commented May 4, 2021

@algomaster99 Congrats on your first contribution to Spoon!

@algomaster99
Copy link
Contributor Author

Thanks a lot! :)

I will move ahead and use this API in diffmin. How can I find the SNAPSHOT version?

@algomaster99 algomaster99 deleted the add-parameter-at branch May 4, 2021 06:30
@slarse
Copy link
Collaborator

slarse commented May 4, 2021

@algomaster99 The snapshot version is already used in diffmin. When you specify 9.1.0-SNAPSHOT, Maven will fetch the latest snapshot for 9.1.0. We publish a snapshot on each push to master in Spoon, so it should be up by now.

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.

3 participants