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: fix: Fix return type of setParent #4099

Merged
merged 5 commits into from
Aug 17, 2021

Conversation

slarse
Copy link
Collaborator

@slarse slarse commented Aug 16, 2021

Fix #4098 (see issue for description of problem)

Breaking change!

This PR implements option 2 presented in #4098. It's a purely statically breaking change, meaning that some compilation may be affected, but behavior is not. However, compilation is pretty unlikely to be affected, and it should really only be the case if the parameterization of the method is explicitly provided.

@@ -45,7 +45,7 @@ public void accept(CtVisitor visitor) {

@Override
public CtTypeReference<Void> getType() {
return (CtTypeReference<Void>) getFactory().Type().VOID_PRIMITIVE.clone().<CtTypeAccess>setParent(this);
return getFactory().Type().VOID_PRIMITIVE.clone().setParent(this);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Case and point in small amount of breakage: this was the only part of the entire Spoon codebase that needed to change, apart from the directly affected method headers.

@slarse
Copy link
Collaborator Author

slarse commented Aug 16, 2021

@I-Al-Istannen The improved Javadoc check pinpointed the correct error: https://github.com/INRIA/spoon/runs/3337956190?check_suite_focus=true#step:9:25 :)

EDIT: Almost, it was off by one method. But pretty close!

@slarse slarse changed the title wip: fix: Fix return type of setParent review: fix: Fix return type of setParent Aug 17, 2021
@monperrus
Copy link
Collaborator

Thanks for the analysis and proof that it's minor.

What about this alternative <E extends CtElement> E setParent(CtElement parent), not changing the return type and type parameters?

@slarse
Copy link
Collaborator Author

slarse commented Aug 17, 2021

Ah, yes, that should also work. Technically speaking the return type is changed, as it is no longer bound to the argument. This means that there could potentially still be static breakage, but as the return type was incorrect we probably want that breakage.

Oh, and note that this is the super sketchy use of type parameters in return types that I advocated against in #1846 (although there I talk about return-type only parameters, but it's really a problem wherever unchecked casts are used). It can cause stuff like this, where incorrect typing simply isn't detected. But it's already there so we need to roll with it.

@monperrus
Copy link
Collaborator

Simple and to the point, thanks. Will merge.

@monperrus monperrus merged commit 44e0e9a into INRIA:master Aug 17, 2021
@monperrus
Copy link
Collaborator

Thanks @slarse

@slarse slarse deleted the issue/4098-fix-setParent-return-type branch August 17, 2021 13:32
@monperrus monperrus mentioned this pull request Aug 19, 2021
@slarse
Copy link
Collaborator Author

slarse commented Aug 23, 2021

This caused a regression in Spork (and likely will for any Kotlin dependents).

For any call previously looking like so:

element.setParent(parent);

Must now be amended like so:

element.setParent<CtElement>(parent);

For the reasons outlined in #1846

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CtElement.setParent's return type is incorrect, or all implementations are incorrect
2 participants