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 issue with isOverriding #1407 #1411

Merged
merged 12 commits into from
Jun 28, 2017

Conversation

pvojtechovsky
Copy link
Collaborator

This PR is based on #1408 and should fix the problem of #1407.

@pvojtechovsky
Copy link
Collaborator Author

There were two problems

  1. java reflection did not created CtTypeParameterReference correctly. It made CtTypeReference instead
  2. CtTypeParameterReference.getBoundingType() was adapted in wrong way...

@pvojtechovsky
Copy link
Collaborator Author

There is another problem ... I have added test GenericsTest#testGetExecDeclarationOfEnumSetOf

<E extends Enum> EnumSet#of(E)

has different depth of boundingTypes in their CtTypeParameterReference ...

Please have a look at this problem. I have no idea how to solve it...
I think it is not correct to have boundingType for CtTypeParameterReferences which mirrors superType of it's TypeParameter declaration.

surli added 2 commits June 26, 2017 16:23
…dd a new convention in ReferenceBuilder to only check 2 levels in case of reflexive type parameter like E extends Enum<E>: it will only get E -> Enum -> E.
@surli
Copy link
Collaborator

surli commented Jun 26, 2017

I think it is not correct to have boundingType for CtTypeParameterReferences which mirrors superType of it's TypeParameter declaration.

I agree, I changed the convention to solve problems like Enum<E extends Enum<E>> to solve them keeping only E -> Enum -> E as typeParameterReference.

However to fix your test I also had to change the CtExecutableReference#getDeclaration to use getTypeDeclaration. It changes some tests, but it also brings more information...

if (typeRefAdapted instanceof CtTypeParameterReference) {
return adaptTypeParameterReferenceBoundingType((CtTypeParameterReference) typeRefAdapted, typeParamRef.getBoundingType());
}
//Do not adapt bounding type of TypeParameterReference. That bounding type is already adapted as part of typeParamRef adaptation
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented code

@surli surli changed the title Fix issue with isOverriding #1407 review: Fix issue with isOverriding #1407 Jun 27, 2017
@surli
Copy link
Collaborator

surli commented Jun 27, 2017

it's ok for you @pvojtechovsky ?

@surli surli closed this Jun 27, 2017
@surli surli reopened this Jun 27, 2017
@pvojtechovsky
Copy link
Collaborator Author

I will check it in the evening

@@ -145,7 +146,16 @@ public boolean isConstructor() {
@SuppressWarnings("unchecked")
public CtExecutable<T> getDeclaration() {
final CtTypeReference<?> typeRef = getDeclaringType();
return typeRef == null ? null : getCtExecutable(typeRef.getDeclaration());
if (typeRef == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a test typeRef.getDeclaration() == null (not in source path)


CtInvocation invocation = ctClass.getMethodsByName("m").get(0).getBody().getStatement(0);
CtExecutable<?> decl = invocation.getExecutable().getDeclaration();
assertNotNull(decl);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This assert should be null (getDeclaration without the source for java.util). Add a new assert with getExecutableDeclaration

assertTrue(ctc.isOverriding(methods.get(0), methodsItf.get(0)));
assertTrue(ctc.isSubSignature(methods.get(0), methodsItf.get(0)));

// assertTrue(ctc.isSameSignature(methods.get(0), methodsItf.get(0)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove commented code

@pvojtechovsky
Copy link
Collaborator Author

It is mainly Simon's work. It should be merged under his name.
Thank you Simon for that effort.

@monperrus monperrus merged commit 05df3b8 into INRIA:master Jun 28, 2017
@monperrus
Copy link
Collaborator

thanks!

@pvojtechovsky pvojtechovsky deleted the issue-with-is-same-signature branch June 28, 2017 16:43
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