fix: cleaning of method signature computation #1561

monperrus · 2017-09-26T19:54:28Z

We have problems regarding method signatures:

1. the current signatures of method references are completely different which is particularly confusing. One contains the declaring type which is about method links in Javadoc, not about signatures.
1. There is no clear definition of method signatures in the Java specification (while usually the return type is not considered as part of the signature, there are cases with generics where it actually matters).

This PR fixes 1) and makes a sensible choice for 2)

Fix #1483

up

monperrus · 2017-09-27T15:20:25Z

need #1562 first in order to strengthen the test case

pvojtechovsky · 2017-09-27T18:15:30Z

makes a sensible choice for 2)

how can client choose that? I see that return value is actually not printed only if it is undefined.
May be there will be some clients, who will like signatures without return type even if return type is known - it is more fitting to: "usually the return type is not considered as part of the signature"

What about new method getSignature(boolean includingReturnType)? Or an global switch or hook which let's client register own signature printer (it is probably over-engineered?)

monperrus · 2017-09-28T17:04:28Z

What about new method |getSignature(boolean includingReturnType)|?

I'm not in favor of this kind of dual-behavior method because there are hard to test, maintain and understand.

Or an global switch or hook which let's client register own signature printer (it is probably over-engineered?)

Interesting. That's an option for a future PR.

pvojtechovsky · 2017-09-28T18:45:07Z

I looked at gumtree-spoon-ast-diff project today and the labels for nodes of methods are made using method.getSignature(). This PR will may be break this algorithm, because in this case it was correct to ignore type of returned value. Will there be an easy way to generate method signature without return type, which may be different and is really not relevant in many cases?

monperrus · 2017-09-28T18:50:56Z

I looked at gumtree-spoon-ast-diff project today and the labels for nodes of methods are made using method.getSignature(). This PR will may be break this algorithm, because in this case it was correct to ignore type of returned value.

I'm not sure. It may do the opposite: make it work better :-) I've just started Travis with a signature without the return type to see what tests are broken ... or not.

monperrus · 2017-09-28T20:10:20Z

It seems to be doable to remove the return type from the signature.
Just basic assertEquals to change. Only the failure of ImportTest.testWithInnerEnumDoesNotImportStaticInnerMethods is more difficult.

monperrus · 2017-09-28T20:17:14Z

ImportTest.testWithInnerEnumDoesNotImportStaticInnerMethods is also in fact easy to fix.

So it seems we can go for a getSignature without return type.

WDYT?

pvojtechovsky · 2017-09-28T20:24:47Z

So it seems we can go for a getSignature without return type.

I can imagine that some clients will want to see return type and some wont ...
And I personally do not use getSignature - only for debugging, so I have no real use case.

Anyway I agree that this PR makes sense and Spoon should print signatures more consistent then till now. So I agree that your PR is a good way!

monperrus · 2017-09-28T21:17:31Z

when #1562 is merged, I'll improve the test, and then if everybody's OK, we'll proceed then.

monperrus · 2017-09-29T19:57:03Z

ready for review

pvojtechovsky · 2017-09-30T05:34:51Z

src/main/java/spoon/reflect/visitor/ImportScannerImpl.java

@@ -448,7 +448,9 @@ protected boolean addMethodImport(CtExecutableReference ref) {
 	protected boolean isImportedInMethodImports(CtExecutableReference<?> ref) {
 		if (!(ref.isImplicit()) && methodImports.containsKey(ref.getSimpleName())) {
 			CtExecutableReference<?> exist = methodImports.get(ref.getSimpleName());
-			if (exist.getSignature().equals(ref.getSignature())) {
+			if ((exist.getDeclaringType() != null ? exist.getDeclaringType().getQualifiedName() : "" + "." + exist.getSignature()).equals(


could you add brackets around (x?y:z), so the precedence of operators is clear even for me ;-)
Or better to create private (or public?) method `getQualifiedSignature()'

pvojtechovsky · 2017-09-30T05:38:12Z

src/main/java/spoon/support/template/SubstitutionVisitor.java

+	private static String getShortSignatureForJavadoc(CtExecutableReference<?> ref) {
+		SignaturePrinter sp = new SignaturePrinter();
+		sp.writeNameAndParameters(ref);
+		return ref.getDeclaringType().getSimpleName() + CtExecutable.EXECUTABLE_SEPARATOR + sp.getSignature();


What about new public class MethodSignatureHelper, which would implement

getSignature(...)

getJavaDocSignature(...)

getQualifiedSignature(...)

.,. ?

pvojtechovsky · 2017-09-30T05:40:45Z

src/main/java/spoon/support/visitor/SignaturePrinter.java

@@ -38,6 +36,8 @@

 	private final StringBuffer signature = new StringBuffer();


what about faster StringBuilder ?

pvojtechovsky · 2017-09-30T05:43:19Z

src/test/java/spoon/test/comment/CommentTest.java

@@ -686,7 +686,7 @@ public void testSnippedWithComments(){
 		assertTrue(clazz1==builder.getSnippetCompilationUnit().getDeclaredTypes().get(0));

 		CtMethod<?> methodString = (CtMethod<?>) clazz1.getMethods().toArray()[0];
-		assertEquals("java.io.File foo(java.lang.String)", methodString.getSignature());
+		assertEquals("foo", methodString.getSimpleName());


Why this change? methodString.getSignature() fails here?

pvojtechovsky · 2017-09-30T06:06:36Z

This implementation is OK for me. The comments above are optional.

monperrus · 2017-10-02T19:41:26Z

That could be interesting, yes.

monperrus · 2017-10-02T19:44:03Z

thanks, changed.

pvojtechovsky · 2017-10-02T19:57:55Z

Thank You Martin for this fix!

up

628ed5f

up

monperrus changed the title ~~fix: cleaning of method signature computation~~ WIP: fix: cleaning of method signature computation Sep 27, 2017

INRIA deleted a comment from spoon-bot Sep 27, 2017

trying without the return type

ac737f5

monperrus force-pushed the unify-signature branch from 43d7cef to ac737f5 Compare September 28, 2017 19:45

up

2fb1dd3

INRIA deleted a comment from spoon-bot Sep 29, 2017

monperrus added 3 commits September 29, 2017 21:26

up

e59598a

up

e28d553

up

44708e6

INRIA deleted a comment from spoon-bot Sep 29, 2017

monperrus added 3 commits September 29, 2017 21:39

up

581c6a8

up

1fba870

up

c17e787

INRIA deleted a comment from spoon-bot Sep 29, 2017

up

201101c

monperrus changed the title ~~WIP: fix: cleaning of method signature computation~~ fix: cleaning of method signature computation Sep 29, 2017

pvojtechovsky reviewed Sep 30, 2017

View reviewed changes

Commit by Martin Monperrus on 02 October 2017

e130fe3

pvojtechovsky merged commit a4e9c9c into INRIA:master Oct 2, 2017

surli mentioned this pull request Nov 14, 2017

[RELEASE] Major Spoon Release 6.0.0: done. #1699

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cleaning of method signature computation #1561

fix: cleaning of method signature computation #1561

monperrus commented Sep 26, 2017 •

edited

Loading

monperrus commented Sep 27, 2017

pvojtechovsky commented Sep 27, 2017

monperrus commented Sep 28, 2017 via email •

edited

Loading

pvojtechovsky commented Sep 28, 2017

monperrus commented Sep 28, 2017 via email

monperrus commented Sep 28, 2017

monperrus commented Sep 28, 2017

pvojtechovsky commented Sep 28, 2017

monperrus commented Sep 28, 2017

monperrus commented Sep 29, 2017

pvojtechovsky Sep 30, 2017

pvojtechovsky Sep 30, 2017

pvojtechovsky Sep 30, 2017

pvojtechovsky Sep 30, 2017

pvojtechovsky commented Sep 30, 2017

monperrus commented Oct 2, 2017 via email

monperrus commented Oct 2, 2017

pvojtechovsky commented Oct 2, 2017

		@@ -38,6 +36,8 @@

		private final StringBuffer signature = new StringBuffer();

fix: cleaning of method signature computation #1561

fix: cleaning of method signature computation #1561

Conversation

monperrus commented Sep 26, 2017 • edited Loading

monperrus commented Sep 27, 2017

pvojtechovsky commented Sep 27, 2017

monperrus commented Sep 28, 2017 via email • edited Loading

pvojtechovsky commented Sep 28, 2017

monperrus commented Sep 28, 2017 via email

monperrus commented Sep 28, 2017

monperrus commented Sep 28, 2017

pvojtechovsky commented Sep 28, 2017

monperrus commented Sep 28, 2017

monperrus commented Sep 29, 2017

pvojtechovsky Sep 30, 2017

Choose a reason for hiding this comment

pvojtechovsky Sep 30, 2017

Choose a reason for hiding this comment

pvojtechovsky Sep 30, 2017

Choose a reason for hiding this comment

pvojtechovsky Sep 30, 2017

Choose a reason for hiding this comment

pvojtechovsky commented Sep 30, 2017

monperrus commented Oct 2, 2017 via email

monperrus commented Oct 2, 2017

pvojtechovsky commented Oct 2, 2017

monperrus commented Sep 26, 2017 •

edited

Loading

monperrus commented Sep 28, 2017 via email •

edited

Loading