review refactor: getBoundingType now returns Object by default #1672

surli · 2017-10-30T14:22:38Z

Instead of returning null, default behaviour of getBoundingType is to return Object.
This PR is an alternative to #1661

…ng type, instead of null

…instead of null

pvojtechovsky · 2017-10-30T19:58:40Z

src/main/java/spoon/support/reflect/reference/CtTypeParameterReferenceImpl.java

@@ -50,6 +50,7 @@

 	public CtTypeParameterReferenceImpl() {
 		super();
+		this.setBoundingType(getFactory().Type().objectType()); // by default set bounding type to object


we should create new CtTypeReference, because AST should not share nodes

Actually the methods already create a clone of OBJECT in TypeFactory, see:

spoon/src/main/java/spoon/reflect/factory/TypeFactory.java

Line 251 in e2e359e

return OBJECT.clone();

So I assume the nodes are not shared.

you are right. I did not knew that. So it is OK

pvojtechovsky · 2017-10-30T19:59:50Z

src/main/java/spoon/support/reflect/reference/CtTypeParameterReferenceImpl.java

+		// ugly but else make testSetterInNodes failed
+		if (superType == null) { // if null, set bounding type to object
+			superType = getFactory().Type().objectType();
+			superType.setParent(this);


use new CtTypeReference too

pvojtechovsky · 2017-10-30T20:02:35Z

This PR is compatible with algorithms, which needed fast fix #1661. This solution is on good way. I like it!

monperrus · 2017-10-30T21:01:26Z

I like it too! return null is always such a bad idea!

…turn-object

surli · 2017-10-31T09:22:14Z

Any help is welcome for that PR!

The changes are done for passing the current Spoon test suite, but I don't manage to fix #1670 or to reproduce it in a small unit test.
The bug occured when using the script https://github.com/INRIA/spoon/blob/master/doc/jenkins/build.sh on assertj-core project. I now got the following error:

Failed to execute goal fr.inria.gforge.spoon:spoon-maven-plugin:2.4:generate (default) on project assertj-core: Exception during the spoonify of the target project.: Type mismatch: cannot convert from AbstractListAssert<capture#76-of ?,List,Object,ObjectAssert> to AbstractListAssert,Object,ObjectAssert> at /Users/urli/Github/assertj-core/src/main/java/org/assertj/core/api/AbstractIterableAssert.java:491

I manage to fix the bug manually by changing

AbstractListAssert<?,List<?>,Object,ObjectAssert<Object>>

to

AbstractListAssert<?,List<? extends java.lang.Object>,Object,ObjectAssert<Object>>

manually in the code, but I don't completely understand why explicitely extending the wildcard to object in the source code fix the bug.
I think it's because this method use another method which have the following return signature:
AbstractListAssert<?,List<? extends E>,Object,ObjectAssert<E>> and JDT does not manage to make the binding properly, but I still did not manage to reproduce the bug yet.

Still, my manual bug fix seems to show that sometimes we need to explicitely output an extends Object. Then I may need to know when there was an explicit extends object in source, and when it was implicit. Any idea to fix that?

monperrus · 2017-10-31T09:30:19Z

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

@@ -618,7 +618,7 @@ public void visitCtTypeParameter(CtTypeParameter typeParameter) {
 		} else {
 			printer.writeIdentifier(ref.getSimpleName());
 		}
-		if (ref.getBoundingType() != null) {
+		if (ref.getBoundingType() != null && !ref.getBoundingType().equals(ref.getFactory().Type().OBJECT)) { // we ignore printing "extends Object"


one solution may be to simply remove this and always write "extends Object"

one solution may be to simply remove this and always write "extends Object"

Yes, in fact it's the one on WildcardReference that we should remove (https://github.com/INRIA/spoon/pull/1672/files/ed1cf1264233fa60a055032c286b682b0562362c#diff-5f33bf5d1f38230de815e4c60a67a215R1573).
Indeed it works for assertj, but it means that everytime you use Something<?> it will be written Something<? extends java.lang.Object> which is ugly.

I vote for shorter printed source code. So never print "... extends Object"

So never print "... extends Object"

And then I have a bug with assertj-core.

surli · 2017-10-31T10:18:21Z

So... in fact as here as in #1202 the bug is related to JDT own interpretation of wildcard generics in Java.
If you take this line https://github.com/joel-costigliola/assertj-core/blob/9d45e93be7f41da5c0709fc852bebd4ef79f6162/src/main/java/org/assertj/core/api/AbstractIterableAssert.java#L1435

in Intellij Idea it shows a warning on ? extends Object saying its redundant, and in Eclipse it shows an error when you remove the extends.
Then as I already said, here the only way to fix the bug is to have a way to know if this extends belongs to the original source code or not.

monperrus · 2017-10-31T10:21:47Z

OracleJDK and OpenJDK don't handle generics the same way, and this is yet another instance of the problem.

Then as I already said, here the only way to fix the bug is to have a way to know if this extends belongs to the original source code or not.

We have isImplicit that we can put in the bounding type.

…

--Martin

On 10/31/17 11:18 AM, Simon Urli wrote: So... in fact as here as in #1202 <#1202> the bug is related to JDT own interpretation of wildcard generics in Java. If you take this line https://github.com/joel-costigliola/assertj-core/blob/9d45e93be7f41da5c0709fc852bebd4ef79f6162/src/main/java/org/assertj/core/api/AbstractIterableAssert.java#L1435 in Intellij Idea it shows a warning on |? extends Object| saying its redundant, and in Eclipse it shows an error when you remove the extends. Then as I already said, here the only way to fix the bug is to have a way to know if this extends belongs to the original source code or not. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1672 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxDUsjykkmHAL-1Y-h-ieQ_gbiU9OxJks5sxvPugaJpZM4QLQr_>.

surli · 2017-10-31T10:25:23Z

We have isImplicit that we can put in the bounding type.

I'll take a look on that.

…e impliciteness to tell DJPP if it must be printed or not

surli · 2017-10-31T12:14:05Z

The last changes now fix the error in #1670 we use the isImplicit to tell DJPP if it must be output or not.

monperrus · 2017-10-31T13:49:45Z

src/main/java/spoon/reflect/reference/CtTypeParameterReference.java

+	 * Returns true if the given typeReference is the default bounding type (i.e. java.lang.Object)
+	 */
+	@DerivedProperty
+	boolean isDefaultBoundingType(CtTypeReference typeReference);


parameter needed? could be simply isDefaultBound() with no parameter?

The parameter is needed for case like https://github.com/INRIA/spoon/pull/1672/files#diff-e14ef777963d781b479e6fdd3e8d419dR79 where we want to check before setting the boundingType.

The code on the link will work if object is sent to adapt method too. Only check for null is needed.
The implementation of is default bounding type can be made static. May be we do not need such method at all.

monperrus · 2017-10-31T13:50:05Z

src/test/java/spoon/test/reference/TypeReferenceTest.java


 		reference.addBound(factory.Type().createReference(String.class));

 		assertNotNull(reference.getBoundingType());

 		reference.setBounds(new ArrayList<>());

-		assertNull(reference.getBoundingType());
+		assertEquals(factory.Type().OBJECT, reference.getBoundingType());
+		assertTrue(reference.isDefaultBoundingType(reference.getBoundingType()));
 	}


add a test corresponding to the assert4j problem?

add a test corresponding to the assert4j problem?

I cannot reproduce exactly the assert4j problem, but we already have in our test a case with an explicit "? super Object", I changed the assert accordingly: https://github.com/INRIA/spoon/pull/1672/files#diff-c20710bb8ee877d0eae2d22578a8f010R506

monperrus · 2017-10-31T13:57:05Z

I'm not convinced by the design and the encapsulation of this new method. Can the the test be rewritten?

surli · 2017-10-31T14:31:10Z

I'm not convinced by the design and the encapsulation of this new method. Can the the test be rewritten?

You're talking about CtTypeParameterReference#isDefaultBoundingType, right?
I'm completely open to redesign it, but I'm not sure to understand what you want.

monperrus · 2017-10-31T14:40:20Z

I'd propose to remove the parameter of the method. the "is" applies to "this" not to the parameter.

surli · 2017-10-31T17:40:11Z

I'd propose to remove the parameter of the method. the "is" applies to "this" not to the parameter.

I'm ok with that but I also want to have a place where the default value of boundingType is defined and that I can use without creating a new CtTypeParameterReference object. This will allow me to do some checks with a single point of definition for the default value of bounding type.

Where do you think I can create such a method?

monperrus · 2017-11-01T19:45:29Z

In a helper class with only static methods? such as JDTTreeBuilderHelper?

spoon-bot · 2017-11-06T09:46:56Z

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-20171105.234530-142

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

Detected changes: 1.

Change 1

Name	Element
Old	none
New	method boolean spoon.reflect.reference.CtTypeParameterReference::isDefaultBoundingType()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking

surli · 2017-11-06T10:43:37Z

It's ready for merging

monperrus · 2017-11-06T10:52:05Z

Thanks a lot.

surli added 4 commits October 30, 2017 13:53

Add Environment#setFactory to environment

1ee007a

Add a test

f9111fb

Now CtTypeParameterReference contains return Object as default boundi…

20c4947

…ng type, instead of null

Changes to take into account that getBoundingType will return Object …

8458d89

…instead of null

surli changed the title ~~refactor: getBoundingType now returns Object by default~~ WiP refactor: getBoundingType now returns Object by default Oct 30, 2017

Change other test condition to take into account the new contract

5913e2a

pvojtechovsky reviewed Oct 30, 2017

View reviewed changes

surli added 2 commits October 31, 2017 10:01

Merge remote-tracking branch 'inria/master' into fix-bounding-type-re…

a762d20

…turn-object

Remove changes about environment

ed1cf12

surli added the help wanted label Oct 31, 2017

monperrus reviewed Oct 31, 2017

View reviewed changes

Use implicit in default CtTypeParameterReference bounding type and us…

76cfe76

…e impliciteness to tell DJPP if it must be printed or not

surli changed the title ~~WiP refactor: getBoundingType now returns Object by default~~ review refactor: getBoundingType now returns Object by default Oct 31, 2017

monperrus reviewed Oct 31, 2017

View reviewed changes

surli and others added 2 commits November 6, 2017 10:32

Change signature of isDefaultBoundingType

c5bcee6

Update CtTypeParameterReference.java

ffd45e6

INRIA deleted a comment from spoon-bot Nov 6, 2017

monperrus merged commit dc6fbc7 into INRIA:master Nov 6, 2017

This was referenced Nov 8, 2017

StackOverflowError with Spoon 6.0.0 using the MavenLauncher #1617

Closed

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review refactor: getBoundingType now returns Object by default #1672

review refactor: getBoundingType now returns Object by default #1672

surli commented Oct 30, 2017

pvojtechovsky Oct 30, 2017

surli Oct 31, 2017 •

edited

Loading

pvojtechovsky Oct 31, 2017

pvojtechovsky Oct 30, 2017

pvojtechovsky commented Oct 30, 2017

monperrus commented Oct 30, 2017

surli commented Oct 31, 2017 •

edited

Loading

monperrus Oct 31, 2017

surli Oct 31, 2017

pvojtechovsky Oct 31, 2017

surli Oct 31, 2017

surli commented Oct 31, 2017

monperrus commented Oct 31, 2017 via email

surli commented Oct 31, 2017 •

edited

Loading

surli commented Oct 31, 2017

monperrus Oct 31, 2017

surli Oct 31, 2017

pvojtechovsky Oct 31, 2017

monperrus Oct 31, 2017

surli Oct 31, 2017

monperrus commented Oct 31, 2017 via email

surli commented Oct 31, 2017

monperrus commented Oct 31, 2017 •

edited

Loading

surli commented Oct 31, 2017

monperrus commented Nov 1, 2017 via email

spoon-bot commented Nov 6, 2017

surli commented Nov 6, 2017

monperrus commented Nov 6, 2017

review refactor: getBoundingType now returns Object by default #1672

review refactor: getBoundingType now returns Object by default #1672

Conversation

surli commented Oct 30, 2017

Choose a reason for hiding this comment

surli Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvojtechovsky commented Oct 30, 2017

monperrus commented Oct 30, 2017

surli commented Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surli commented Oct 31, 2017

monperrus commented Oct 31, 2017 via email

surli commented Oct 31, 2017 • edited Loading

surli commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monperrus commented Oct 31, 2017 via email

surli commented Oct 31, 2017

monperrus commented Oct 31, 2017 • edited Loading

surli commented Oct 31, 2017

monperrus commented Nov 1, 2017 via email

spoon-bot commented Nov 6, 2017

Revapi Analysis results

Change 1

surli commented Nov 6, 2017

monperrus commented Nov 6, 2017

surli Oct 31, 2017 •

edited

Loading

surli commented Oct 31, 2017 •

edited

Loading

surli commented Oct 31, 2017 •

edited

Loading

monperrus commented Oct 31, 2017 •

edited

Loading