fix: consolidate a consistent behavior for CtElement#getParent

[sibwaf]

The three existing overloads are not consistent in their behavior. There is also an issue with javadoc for getParent(Filter) - it states that the method can return the receiving element which would be counter-intuitive if it was actually true (it's not). Some discussion can be seen in #3734.

When an element doesn't have a parent:

1. getParent() throws ParentNotInitializedException
2. getParent(Class) returns null
3. getParent(Filter) throws ParentNotInitializedException

When there are no matching parents:

1. getParent() - not applicable, any parent matches
2. getParent(Class) returns null
3. getParent(Filter) throws ParentNotInitializedException (and, maybe, returns null in some cases)

This PR defines the following behavior:

1. If the current element doesn't have a parent, ParentNotInitializedException is thrown
2. Else, if no matching parents exist, null is returned

This PR includes a behavior consistency test, matching behavior changes, a javadoc fix and an easier-to-read getParent(Filter) implementation.

[slarse]

@dya-tel You're failing some style checks, could you run mvn checkstyle:check locally and fix any violations of the checkstyle rules?

[slarse]

Aaand, now you ran into flaky CI. I'm pretty sure the test failure now is caused by Maven connection pooling issues with Azure (see #3745). You can do an empty commit (git commit --allow-empty) and push again to re-trigger CI, and it should work.

Sorry about that, this has happened often enough now that we probably need to implement a workaround.

[sibwaf]

Haha, I don't mind. Seems like it has finally worked.

[slarse]

I think this PR looks good overall. I'd definitely prefer standard while loops over do-while loops, as the latter are much less commonly used in Java (and here, the same thing can be achieved with standard while loops without jumping through hoops).

I have one concern, and that's the usability aspect of the getParent() methods. For the getParent() (no-argument) method, you are guaranteed that e.getParentInitialized() == true <==> e.getParent() != null, making the usage very neat. The other overloads have no such guarantee, and you need to double up on checks (both check that the parent is initialized, and that the return value is not null). There is also a fact that getParent(Class) and getParent(Filter) search up the tree, should they throw ParentNotInitialized if a transitive parent is not initialized? They don't currently, but my first thought when reading the PR was that that was the intended behavior.

I think it makes sense for getParent(Class) and getParent(Filter) to return null instead of throwing an exception. In my mind, they both search through the collection of all parents of the current element and try to find a match, and return null if no match is found. If the current element has no parent, then that collection is empty (but not non-existent), leading to there being no match, and thus we return null.

@dya-tel Thoughts? One way or another this PR should be merged (in my opinion), as getParent(Class) and getParent(Filter) should at the very least maintain the same behavior. Due to the major revision coming up (see #2869), this is the time to perform a breaking change like this!

Ps. We'll bring in an integrator for the final verdict on this once we've discussed it (I'm not an integrator, was just asked to review this).

[slarse]

The same behavior can be achieved with a standard while loop. You can also get rid of the "unchecked" warning by casting with the parent type instead.

Suggested change

	CtElement current = this;
	do {
	current = current.getParent();
	if (parentType.isAssignableFrom(current.getClass())) {
	return (P) current;
	}
	} while (current.isParentInitialized());
	CtElement current = getParent();
	while (current.isParentInitialized()) {
	if (parentType.isAssignableFrom(current.getClass())) {
	return parentType.cast(current);
	}
	current = current.getParent();
	}

[slarse]

My suggestion here is incorrect:

The 'do-while' is actually required here. With your 'while' variants if the first parent matches but doesn't have a parent itself, it won't be returned.

[slarse]

A standard while loop can do the same thing (couldn't do a suggestion here due to the fragmented diff):

		CtElement current = getParent();
		while (current.isParentInitialized()) {
			try {
				if (filter.matches((E) current)) {
					return (E) current;
				}
			} catch (ClassCastException ignored) {
				// expected, some elements are not of type
			}
			current = current.getParent();
		};

[slarse]

My suggestion here is incorrect:

The 'do-while' is actually required here. With your 'while' variants if the first parent matches but doesn't have a parent itself, it won't be returned.

[slarse]

Why not use the ternary operator here like you've done in the other places?

[sibwaf]

The 'do-while' is actually required here. With your 'while' variants if the first parent matches but doesn't have a parent itself, it won't be returned.

The 'null/exception' decision is a tough one indeed. In the current situation I prefer the 'null' way too, but looking forward the 'exception' way should be better. As to why - well, first of all, because overloads would match the 'main' one's behavior. I also treat the ParentNotInitializedException just the way it's named - someone forgot to initialize the parent of an element. Thus the model is invalid and we should halt and catch fire. The problem is that currently this exception can also mean that there is no parent for an element (when there shouldn't be one) and this creates a lot of confusion and design problems IMO (like the need of multiple checks everywhere, unexpected exceptions when cloning elements and problems with detecting incorrectly built models).

About the transitive parents. The hierarchy must end somewhere, so there's always a transitive parent which has an uninitialized one (in the current meaning). And this will cause an exception if a match is not found.

With everything said, I can remake this PR the 'null' way and cleanup the redundant checks for now. And someday when the ParentNotInitializedException actually makes sense, someone could just rework it all and bring the exceptions back without the need for any checks.

[slarse]

The 'do-while' is actually required here. With your 'while' variants if the first parent matches but doesn't have a parent itself, it won't be returned.

Indeed you are right. Do-while it is.

As to why - well, first of all, because overloads would match the 'main' one's behavior.

By the very nature of what the overloads do, this is already not the case. One gets the immediate parent, the other search for ancestors that match a type or lambda. They are fundamentally different things.

I also treat the ParentNotInitializedException just the way it's named - someone forgot to initialize the parent of an element. Thus the model is invalid and we should halt and catch fire. [...] About the transitive parents. The hierarchy must end somewhere, so there's always a transitive parent which has an uninitialized one (in the current meaning). And this will cause an exception if a match is not found.

Well, that's not really how it works even in this PR. Imagine that I programmatically create a class and add a method. Then the method has the class as a parent, but the class has no parent. If I search for parents with method.getParent(CtPackage.class), then there's no exception. This is what I'm getting at: it doesn't throw on a broken/partial model, it throws if the element you call getParent on doesn't have a parent.

The truly consistent solution would be to throw if no match is found, instead of returning null. But, as stated, that's not how this PR works. It would force an EAFP-style of usage, and I think we can both agree that's not the way to go. It's also sometimes desirable to work with partial models, and so we shouldn't make that harder than it needs to be.

With everything said, I can remake this PR the 'null' way and cleanup the redundant checks for now. And someday when the ParentNotInitializedException actually makes sense, someone could just rework it all and bring the exceptions back without the need for any checks.

I'm not convinced of either approach. They both have problems. IMO the best solution would be to rename the overloads to searchAncestor or something like that, but that's probably a bit too breaking for the tastes of Spoon integrators who value backwards compatibility very highly.

[slarse]

@dya-tel Do you agree that these are the options?

1. Calling any e.getParent() method throws an exception if e does not have a parent, but returns null for the Class and Filter overloads if no match is found in the transitive search.
   • Drawback: Requires both checking e.isParentInitialized() and e.getParent() != null for the overloads.
2. Calling e.getParent() throws an exception if e does not have a parent, while the Class and Filter overloads only return null (they never throw).
   • Drawback: Might be confusing that the overloads don't follow the same exception-throwing pattern as the no-argument version.
3. Rename the overloads to e.g. searchAncestor and define their behavior separately.
   • Drawback: Super breaking.

I think only 1. and 2. are realistic, but I threw 3. in there because why not.

[sibwaf]

By the very nature of what the overloads do, this is already not the case. One gets the immediate parent, the other search for ancestors that match a type or lambda. They are fundamentally different things.

Well, in my eyes the overloads do the same thing - search for a parent. The parameter-less one just matches any parent, something along the lines of e.getParent() == e.getParent(it -> true). Kinda like collection.any() is "give me any element" and collection.any(it -> filter(it)) is "give me any matching element". But this is just how I perceive things. Maybe the naming throws me off the intended meaning, though I don't see any contradictions in my logic.

Well, that's not really how it works even in this PR. Imagine that I programmatically create a class and add a method. Then the method has the class as a parent, but the class has no parent. If I search for parents with method.getParent(CtPackage.class), then there's no exception. This is what I'm getting at: it doesn't throw on a broken/partial model, it throws if the element you call getParent on doesn't have a parent.

Yep, it indeed doesn't work the way I described - it was a "what if" scenario on how it could be made "the good way in my opinion". Let the elements have null parents, but check if those were directly assigned. So the getParent() could return null if the model is OK or throw an exception in a broken model so we could detect unintentionally dangling elements. Or, maybe, just get rid of the exception completely.

Calling any e.getParent() method throws an exception if e does not have a parent, but returns null for the Class and Filter overloads if no match is found in the transitive search.
Drawback: Requires both checking e.isParentInitialized() and e.getParent() != null for the overloads.

It's a pretty bad solution for now from the usability view, but "the only right way" in my "what if scenario".

Calling e.getParent() throws an exception if e does not have a parent, while the Class and Filter overloads only return null (they never throw).
Drawback: Might be confusing that the overloads don't follow the same exception-throwing pattern as the no-argument version.

A good enough solution for now - helps to cleanup Spoon's code and is easy to use in client code. In the "what if scenario" doesn't work at all.

Rename the overloads to e.g. searchAncestor and define their behavior separately.
Drawback: Super breaking.

Let's just not.

---

All in all, I'd prefer the second option for now.

[slarse]

e.getParent() == e.getParent(it -> true)

I agree, this is a valid way to view it. Well argued, I now understand your mental model.

My mental model is more like e.getParent() is like LinkedList.getFirst(), which throws if the list is empty, and e.getParent(Filter) is something akin' LinkedList.contains(Object), which never throws. One directly gets an element (which may not be there; hence exception), the other searches the collection, which may be empty but always exists, and so a membership check never needs to throw (this logic of course hinges on "no match" => null, which I'm not a fan of but that's the case in large parts of Spoon).

It's a pretty bad solution for now from the usability view, but "the only right way" in my "what if scenario".

Given your above definition, I also agree that it would be a theoretically pure solution.

Or, maybe, just get rid of the exception completely.

That's also probably too breaking, but interesting. The semantics of the exception are unclear, as you've clearly demonstrated.

A good enough solution for now - helps to cleanup Spoon's code and is easy to use in client code. [...] All in all, I'd prefer the second option for now.

I agree. It's the pragmatic solution, although I can see now why it's not as intellectually satisfying as your initial approach.

[sibwaf]

Well, this was "fun". Here's the null variant.

[slarse]

Well, this was "fun". Here's the null variant.

I'm sorry if you found it frustrating, but as this is a change to the behavior of publicly accessible functionality, we need to carefully consider the alternatives. Thank you for taking the time to do so.

I think the code now looks excellent, I just feel that it would be prudent to clearly document the return values of the getParent() overloads. After that, I think it's all good.

[slarse]

Could you document this with an @return, in particular that it returns null if there's no match?

[slarse]

This is based on the discussion we had previously about the poor documentation of null returns in Spoon :)

[slarse]

Same as above.

[sibwaf]

I'm sorry if you found it frustrating

No-no-no, please pay no mind. It's totally fine, I'm just grunting.

I think the code now looks excellent

Thanks! Though, I didn't do much.

[slarse]

@dya-tel

Thanks! Though, I didn't do much.

I think in discussing the different options as carefully as we did, you've made a significant contribution only with that. You've also helped me with practicing code review, which is something I really do need practice with. And the code is good, and the docs much more clear now. So, again, thank you for taking the time :)

@monperrus This is ready for your consideration. It's a breaking change. Here's the tl;dr of it all.

See the opening post for a description of how the getParent() methods worked before this PR, and the initial intention of this PR which was to make the behavior of calling e.getParent entirely consistent across all overloads: throw ParentNotInitializedError if e does not have a parent, or return null if either of the "search" overloads don't find a match.

After a lot of discussion, @dya-tel and I came to the conclusion that it is more usable if e.getParent(Class) and e.getParent(Filter) do not throw ParentNotInitializedError, but instead return null both for when no match is found, and for when no parent even exists. This let's you use them with only one check: is the return value null.

The breaking behavioral change is therefore that e.getParent(Filter) now returns null instead of throwing when the immediate parent of e is not initialized. This was already the behavior of e.getParent(Class). It's not as theoretically pure as @dya-tel's initial PR, but it leads to cleaner use of the methods.

I recommend a merge, it's a good contribution, and we've weighed the pros and cons of the different options carefully.

[monperrus]

We have a major release in front of us, so this is timely.

Although we very much value backward-compatibility, the fact that very few tests break is a good indicator that this change is relatively safe.

So LGTM, will merge.

@dya-tel , thanks a lot for your contribution, @slarse thanks for the shepherding.

[monperrus]

Thanks @dya-tel your contribution is much appreciated!