bug: ParentNotInitializedException in AbstractTypingContext#adaptType()

[sibwaf]

This line throws an exception when the result is a CtTypeReference without a parent, but with type parameters. And "standard" types from a TypeFactory don't have parents.

It's also kinda connected to #3733:

1. Type reference for java.lang.Object gets corrupted with type parameters
2. getBound() returns the corrupted type reference for java.lang.Object here
3. The corrupted type reference destroys adaptType() because it doesn't have a parent as a "standard" type in a TypeFactory

I haven't managed to make a minimal reproducible sample and can't share the main code, sorry.

[sibwaf]

Actually, looking through the CtElementImpl code I've found a bunch of problems.

The first one, which is related to the issue, is that if an element has parent == null then getParent throws an exception because the element's parent wasn't initialized. This seems alright for the most part, but doesn't make sense for elements which do not have a parent - like the "static" type references in the TypeFactory. Those should either be assigned a parent or override the method so it doesn't throw an exception.

Then things get more interesting about the two getParent overloads.

Those overloads throw an exception if current element's parent is null (to match the main no-parameter overload; exception is thrown by the main overload), or return null when no parents match the predicate. But the thing is that one of the overloads doesn't behave like this:

spoon/src/main/java/spoon/support/reflect/declaration/CtElementImpl.java

Lines 384 to 392 in 3435fac

	public <P extends CtElement> P getParent(Class<P> parentType) throws ParentNotInitializedException {
	if (parent == null) {
	return null;
	}
	if (parentType.isAssignableFrom(getParent().getClass())) {
	return (P) getParent();
	}
	return getParent().getParent(parentType);
	}

// yikes
CtTypeReference<?> object = factory.Type().OBJECT;
object.getParent(CtClass.java); // null
object.getParent(new TypeFilter<>(CtClass.java)); // exception

The solution: fix the "Class" overload so both overloads handle null-parent the same way as the main method.

The javadoc states that the "Filter" overload can return the same object (which doesn't make sense for me and probably for a lot of other people, it's a getParent method), but it doesn't state the same for the "Class" overload. That's just confusing:

spoon/src/main/java/spoon/reflect/declaration/CtElement.java

Lines 250 to 259 in 3435fac

	/**
	* Gets the first parent that matches the given type.
	*/
	<P extends CtElement> P getParent(Class<P> parentType) throws ParentNotInitializedException;
	/**
	* Gets the first parent that matches the filter.
	* If the receiver (this) matches the filter, it is also returned
	*/
	<E extends CtElement> E getParent(Filter<E> filter) throws ParentNotInitializedException;

The good news: neither implementation does this! (and it should be that way)

spoon/src/main/java/spoon/support/reflect/declaration/CtElementImpl.java

Lines 396 to 414 in 3435fac

	public <E extends CtElement> E getParent(Filter<E> filter) throws ParentNotInitializedException {
	E current = (E) getParent();
	while (true) {
	try {
	while (current != null && !filter.matches(current)) {
	current = (E) current.getParent();
	}
	break;
	} catch (ClassCastException e) {
	// expected, some elements are not of type
	current = (E) current.getParent();
	}
	}
	if (current != null && filter.matches(current)) {
	return current;
	}
	return null;
	}

The solution: fix the javadoc.

And that's not all. I just... what? We failed casting current to E, so let's cast current to E?

spoon/src/main/java/spoon/support/reflect/declaration/CtElementImpl.java

Lines 399 to 407 in 3435fac

	try {
	while (current != null && !filter.matches(current)) {
	current = (E) current.getParent();
	}
	break;
	} catch (ClassCastException e) {
	// expected, some elements are not of type
	current = (E) current.getParent();
	}

The solution: remove this nonsense.

I can make a pull request for the small stuff (the overload madness) if my solutions seem plausible. But "the main issue" needs more discussion. Maybe "ParentNotInitializedException" shouldn't even be used at all.

[slarse]

Nice finds on those issues. Here are my two cents.

The solution: fix the "Class" overload so both overloads handle null-parent the same way as the main method.

Since we have inconsistent behavior here, this might be the time to introduce a "null object" for parent (in the same spirit as #3705. To me, one of the largest usability issues of Spoon is the proliferation of methods that sometimes return an object and sometimes null. Calling code "done right" becomes cluttered with null checks.

The javadoc states that the "Filter" overload can return the same object (which doesn't make sense for me and probably for a lot of other people, it's a getParent method), but it doesn't state the same for the "Class" overload. [...] The solution: fix the javadoc.

I agree, the documented (but not actual) behavior is very counter-intuitive.

And that's not all. I just... what? We failed casting current to E, so let's cast current to E? [...] The solution: remove this nonsense.

This is not what it looks like. When casting to a type parameter (which you really shouldn't do), you actually in practice cast to the erasure of that type parameter. In the case of E, the erasure is CtElement, as that is its upper bound. So, (E) current will always succeed.

The ClassCastException is in fact raised by filter.matches(E). The JVM will perform a dynamic invocation in which it type checks the argument to the called method's formal parameters, and here this can fail as there is no guarantee that CtElement.getParent() returns an element of type E. The whole method is something of a hack, so removing the seemingly nonsensical catch block actually breaks the entire method.

[sibwaf]

this might be the time to introduce a "null object" for parent

I don't think that that's a good idea. I dislike null-objects in general because in most cases those prevent "the scary NPE" but hide invalid logic which in my opinion is a lot worse. Imagine a "null" bank account - it allows you to send money into nothing instead of just failing a transaction if there is no "invalid destination" check. These objects, of course, have their own niche, but I don't think that's the right situation to use them. Here's why:

1. Someone forgot to add a parent to an element, and all elements have a "null-parent" by default. That's an error when an object is supposed to have a parent - and it's hidden by the "null" one.
2. Totally unrelated objects can suddenly become related. Imagine two CtClass objects from different packages having the same parent because we forgot to assign different "null-packages" for them (I haven't actually checked if CtClass can have a null-parent now, that's just a theoretical situation). Ugh. So null-objects should be introduced very carefully case-by-case and not for everything at the same time (or even not for everything in general).

As for an alternative solution - we can add a flag isParentInitialized which is assigned in the setParent method. This changes the getParent contract, but allows parentless elements.
Having actual nulls as parents also kinda allows elements to appear as siblings with insufficient checks (e1.getParent() == e2.getParent()), but there's nothing we can do about this. The e.getParent() == null and e.getParent() == NO_PARENT checks are interchangeable, but an actual null at least doesn't allow most interactions with the non-existing parent.

methods that sometimes return an object and sometimes null

Maybe it's time to introduce JetBrains' NonNull/Nullable annotations (or some alternatives I'm not aware of)? Those help a lot with eliminating redundant checks and adding necessary ones. That's a lot of work across the library, but gradually adding those here and there should be fine and will prevent more and more problems with time while improving end user experience (as the users should get these annotations as well because they have "class" retention).

The whole method is something of a hack, so removing the seemingly nonsensical catch block actually breaks the entire method.

Ugh, what a mess generics are. Sometimes I wish I wasn't a Java developer.
The code still can use a bit of refactoring / clarification with comments IMO.

[slarse]

this might be the time to introduce a "null object" for parent

I don't think that that's a good idea [...]

Just based on a hunch, I don't think you'd often need to check if it's NO_PARENT. It also wouldn't need to be singleton: there could be a null/missing parent for each individual node. Returning null is just as much hiding a problem as returning a null object, but with the latter it is possible to carefully design how it interacts with other elements. For example, one could have the null object raise an exception for every single method invocation, but in that exception insert enough information about where the object came from that it's easy to debug the failure. To me, null shows a lack of design. Getting an NPE tells you almost nothing of the nature of the failure and may also occur long after null was returned from somewhere, making debugging it annoying at best.

But I don't disagree that it could cause other problems. For example, it only shifts the problem of the return value of getParent() by one step. Another option would be to raise an exception (fail-fast), or return Optional such that you're forced to check. The former requires one to check isParentInitialized() first, but at least it shows intent. The latter would unfortunately break the API immensely, and so probably isn't an option.

As for an alternative solution - we can add a flag isParentInitialized which is assigned in the setParent method. This changes the getParent contract, but allows parentless elements.

Do you mean that setParent(null) would mean that an element has a null parent, but still returns true for isParentInitialized()? I wouldn't expect that behavior as a user, might be confusing.

methods that sometimes return an object and sometimes null

Maybe it's time to introduce JetBrains' NonNull/Nullable annotations (or some alternatives I'm not aware of)? Those help a lot with eliminating redundant checks and adding necessary ones. That's a lot of work across the library, but gradually adding those here and there should be fine and will prevent more and more problems with time while improving end user experience (as the users should get these annotations as well because they have "class" retention).

This is a non-breaking improvement so I see no reason why not to do this. There are many variations of those annotations, and we should then pick the annotations that are most universally supported (I have no idea of which ones are).

The whole method is something of a hack, so removing the seemingly nonsensical catch block actually breaks the entire method.

Ugh, what a mess generics are. Sometimes I wish I wasn't a Java developer.
The code still can use a bit of refactoring / clarification with comments IMO.

Indeed, the code is very hard to understand and should be rewritten.

[slarse]

Actually, thinking a bit more about it, using annotations to denote which methods may or may not return null is probably the only feasible route for the general problem of null returns in Spoon. It's non-breaking, and also the only alternative that doesn't impose performance overhead. Unlike using Optional, it allows CtElement to say that certain methods are nullable, while specific implementations can override that and say that they always return something. I think that'd be a pretty good solution.

[sibwaf]

return Optional such that you're forced to check. ... The latter would unfortunately break the API immensely, and so probably isn't an option.

Yeah, breaking the API isn't good. And optionals are usually a hassle to use anyway.

I'm also warming up a bit to the idea of using null-objects here. Perhaps they could be good enough if combined with the separate "isParentInitialized" flag (or just treating parent == null as not initialized as it is now) - so if someone forgot to initialize the parent via setParent (and possibly "setNoParent()"?) an exception is thrown, and if there is no parent then the null-object is returned. Though I still think that an actual null would be better - there's no parent and you shouldn't interact with it.

using annotations to denote which methods may or may not return null is probably the only feasible route for the general problem of null returns in Spoon

There's always a chance to migrate to Kotlin. The problem with annotations is that we need to choose the exact library for this. The JetBrains ones are obviously supported in IntelliJ, but I have no clue about any static analysis tools or any other IDEs. Migrating later if something happens could become a very "fun" problem. It seems there was a JSR-305 about this, but it's kinda dead now.

I'll try fixing the getParents overloads in the meantime.