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

feature: CtLambda#getMethod() and fix of #1159 #1164

Merged
merged 4 commits into from
Feb 7, 2017

Conversation

pvojtechovsky
Copy link
Collaborator

@pvojtechovsky pvojtechovsky commented Feb 2, 2017

This PR fixes issue #1159. The issue #1159 is caused by wrong return type of CtExecutableReference created for lambda. Therefore this code failed too:

assertTrue(lambdaRef.getDeclaringExecutable().getType()
     .equals(paramRef.getDeclaringExecutable().getType()));

Now there is a question whether solution of this PR is correct.

In current spoon model the interface CtLambda represents both:

  • CtExpression, which returns an instance, which implements a type defined by result of CtLambda.getType()
  • CtExecutable, which represents an executable element. It means it has parameters, body, thrownTypes ... and return type

The problem was that return type of executable element was not accessible. Therefore during creation of CtExecutableReference of lambda expression, there was used wrong return type of that executable (see change in ExecutableFactory).

Now the issue #1159 is fixed - the parameter references are equal.
... but I have found another issue #1163, which causes that CtLambdaImpl#getExecutableType actually does not work correctly.

@monperrus
Copy link
Collaborator

I'd say there is a solution without introducing a new method.

What about setting the type to the correct value when it's available or simply overriding getType in CtLambdaImpl.

@pvojtechovsky
Copy link
Collaborator Author

CtLambdaImpl#getType is already used for different purpose and makes sense too. It cannot be overriden.

What about setting the type to the correct value when it's available

I do not understand. Do you mean to have not derived property CtLambdaImpl#executableType, which is set during compilation of model?

@pvojtechovsky pvojtechovsky force-pushed the LambdaParameterReferenceNotEqual branch from e62b3a1 to 25e855c Compare February 2, 2017 22:21
@pvojtechovsky
Copy link
Collaborator Author

an idea: to replace CtLambda#getExecutableType(), by
A) ctLambda.getExecutable().getType()
B) ctLambda.getExecutableMethod().getType()
C) ctLambda.getMethod().getType()
Note: all these methods are derived From the result of ctLambda.getType().map(new SAMMethodFilter()).list() ...

I suggest that, because the new method getExecutableType is too special. More generic method which returns CtMethod, which is implemented by result of lambda expression, is better.

@pvojtechovsky pvojtechovsky force-pushed the LambdaParameterReferenceNotEqual branch from 25e855c to 11ca484 Compare February 3, 2017 18:06
@pvojtechovsky pvojtechovsky changed the title feature: CtLambda#getExecutableType() and fix of #1159 feature: CtLambda#getMethod() and fix of #1159 Feb 3, 2017
@pvojtechovsky
Copy link
Collaborator Author

I replaced CtLambda#getExecutableType() by CtLambda#getMethod(), which is more useful.

If somebody needs the getExecutableType, then s/he can call ctLambda.getMethod().getType(). It is the same.

I think this code makes sense. The client needs to know, which method is called by lambda expression.

What about the method name CtLambda#getMethod(). Is it good name, or you have better one?

Note: the tests fails because fix of #1163 is not part of this PR (to keep PR simple). After #1165 is merged this PR will pass tests too.

@pvojtechovsky pvojtechovsky force-pushed the LambdaParameterReferenceNotEqual branch from 98df1be to 2e410bb Compare February 5, 2017 13:05
@spoon-bot
Copy link
Collaborator

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170204.234700-25

New API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-SNAPSHOT

Detected changes: 2.

Change 1

Name Element
Old method void spoon.support.visitor.java.reflect.RtMethod::(java.lang.Class, java.lang.String, java.lang.Class, java.lang.reflect.TypeVariable<java.lang.reflect.Method>[], java.lang.Class[], java.lang.Class[], int, java.lang.annotation.Annotation[], java.lang.annotation.Annotation[][], boolean)
New method void spoon.support.visitor.java.reflect.RtMethod::(java.lang.Class, java.lang.String, java.lang.Class, java.lang.reflect.TypeVariable<java.lang.reflect.Method>[], java.lang.Class[], java.lang.Class[], int, java.lang.annotation.Annotation[], java.lang.annotation.Annotation[][], boolean, boolean)
Code java.method.numberOfParametersChanged
Description The number of parameters of the method have changed.
Breaking binary: breaking, source: breaking

Change 2

Name Element
Old none
New method spoon.reflect.declaration.CtMethod spoon.reflect.code.CtLambda::getMethod()
Code java.method.addedToInterface
Description Method was added to an interface.
Breaking semantic: potentially_breaking, binary: non_breaking, source: breaking

@@ -60,6 +62,12 @@
CtExpression<T> getExpression();

/**
* @return a method which is implemented by this lambda expression
Copy link
Collaborator

Choose a reason for hiding this comment

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

@return the method that this lambda expression implements. Must be defined as a non-default method in an interface, eg Consumer.accept().

@monperrus
Copy link
Collaborator

One question: Is lambda.getMethod() equivalent to lambda.getRefarence().getOverridingExecutable().getDeclaration()?

@pvojtechovsky
Copy link
Collaborator Author

Is lambda.getMethod() equivalent to lambda.getReference().getOverridingExecutable().getDeclaration()?

I made a test and lambda.getReference().getOverridingExecutable() is actually null.

I do not know whether it should be equivalent or not. Probably be yes.

I just know that the expression lambda.getReference().getOverridingExecutable().getDeclaration() cannot be used instead of lambda.getMethod(), because lambda.getMethod() is used internally in lambda.getReference(), so it would end with stack overflow.

lambda.getReference().getOverridingExecutable().getDeclaration()
@spoon-bot
Copy link
Collaborator

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170205.234548-26

New API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-SNAPSHOT

Detected changes: 1.

Change 1

Name Element
Old none
New method spoon.reflect.declaration.CtMethod spoon.reflect.code.CtLambda::getMethod()
Code java.method.addedToInterface
Description Method was added to an interface.
Breaking source: breaking, semantic: potentially_breaking, binary: non_breaking

@monperrus
Copy link
Collaborator

OK. This really makes sense, and I'm fine with adding this method to lambda, but with a more specific name.

Do you prefer getOverridenMethodor getImplementedMethod or getImplementedInterfaceMethodor something similar?

@pvojtechovsky
Copy link
Collaborator Author

I like both getOverridenMethod or getImplementedMethod.
getOverridenMethod is probably better, because it's meaning is same like CtExecutable#getOverridenMethod. I will rename it

@monperrus
Copy link
Collaborator

OK, good.

I did a spelling mistake: getOverriddenMethod with double r and double d

@spoon-bot
Copy link
Collaborator

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170206.234614-27

New API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-SNAPSHOT

Detected changes: 1.

Change 1

Name Element
Old none
New method spoon.reflect.declaration.CtMethod spoon.reflect.code.CtLambda::getOverriddenMethod()
Code java.method.addedToInterface
Description Method was added to an interface.
Breaking semantic: potentially_breaking, source: breaking, binary: non_breaking

@surli surli merged commit 825335e into INRIA:master Feb 7, 2017
@pvojtechovsky pvojtechovsky deleted the LambdaParameterReferenceNotEqual branch February 7, 2017 13:52
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.

4 participants