-
-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
review fix: Manage generics from Interface in shadow mode #1914
Conversation
LGTM! |
Great, thank you, I was not quite confident on my changes ;) |
This wrong code is generated by this PR public abstract interface CtField<T extends java.lang.Object>
extends spoon.reflect.code.CtRHSReceiver<A> ,
spoon.reflect.declaration.CtShadowable ,
spoon.reflect.declaration.CtTypeMember ,
spoon.reflect.declaration.CtVariable<T> { ... } The problem is I have extended the test to check that situation |
There is some problem with qualified names of interfaces. I have added failing test. For example CtExpression looks like this: public abstract interface CtExpression<T extends java.lang.Object> extends spoon.reflect.code.CtCodeElement , spoon.reflect.declaration.spoon.reflect.declaration.CtTypedElement<T> , spoon.template.spoon.template.TemplateParameter<T> { where |
I have found two more problems:
|
Next problem is with bounding type of actual type arguments of references of parameter types and return types of methods. See failing test. I am not sure whether it belongs to this PR. May be we merge this one and fix the problem reproduced by last test case in next PR ... but may be it is quite related ... it is up to you, to decide ;-) By the way, I am really not sure that current behavior of non shadow type (from sources) checked by first part of the test is wanted (see comments in test). For me the shadow class behaves more correct ... but the change would probably destroy many things... hard to estimate... |
For me it should be fixed in this PR: we are trying here to align the behaviour between elements coming from sources and shadow elements, and obviously concerning the generic types, shadow classes are currently badly supported. Now I don't agree with your comment on the last test:
You were talking about the type public <C extends CtRHSReceiver<A>> C setAssignment(CtExpression<A> assignment) and public class CtAssignmentImpl<T, A extends T> extends CtStatementImpl implements CtAssignment<T, A> so for me it shoud really use the bounding type T which should itself be bounding only by object. |
@@ -43,9 +43,9 @@ public void visitPackage(Package aPackage) { | |||
clazz.getPackage(); | |||
} | |||
if (clazz.getSuperclass() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it ok that the check is not on getGenericSuperclass
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I certainly should change that one for safety.
…arameters. Avoid infinite loop when resolving them.
Thanks @pvojtechovsky to take back this one. Sorry I'm a lot busy elsewhere these days :-/ |
we have a conflict here. rebase? |
Conflicts: src/test/java/spoon/test/enums/EnumsTest.java
Merge done, but we should review it after #1998, which is contained in this PR too ... so another merge will be needed |
API changes: 19 (Detected by Revapi) Old API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-20180522.082726-80 / New API: fr.inria.gforge.spoon:spoon-core:jar:6.3.0-SNAPSHOT
|
#1998 merged. rebase here? |
It already been done. |
awesome collaborative work, thanks to all! |
This would be a nice reflection library! |
Fix #1906