Need to expand AstCheckerTest to support more patterns for delegation/surcharging

[slarse]

As evidenced by #4038, the AstCheckerTest is too conservative. I'll use #4038 as grounds for the following patterns we probably want to add support for.

First of all, the PushStackInIntercessionChecker needs a refactoring, it's pretty hard to understand what's expected e.g. for a method to count as delegated.

CtInterfaceImpl.removeNestedType should be detected as a delegated method, but it isn't as the method invocation isn't in any location expected by PushStackInIntercessionChecker.isDelegate, so that should be expanded. I'm thinking we might want to cover this pattern:

// store return value of delegated-to method in a variable
@Override
public SomeType method() {
    SomeType val = super.method();
    [...]
    if (something) {
        // all returns must use val
        return val;
    }

    return val;
}

Now, the pattern used in this PR is more like this:

// use delegated method in early return if condition
@Override
public boolean method() {
    if (!super.method()) {
        return false;
    }
    [...]

    return true;
}

But supporting that pattern is a little bit more difficult as it's harder to verify that the return value of the method is actually used. It is used implicitly here, and it's possible to verify, it's just some amount of work to do so, whereas verifying that all return values reference a particular local variable is trivial.

CtTypeImpl.removeNestedType has essentially the same problem, but for the surcharge instead of the delegate check. I think supporting the pattern I suggested above should cover this as well.

Obviously the pattern doesn't make for ideal code, but it's easy to verify and applicable to all kinds of return values.

[slarse]

@I-Al-Istannen WDYT?

[I-Al-Istannen]

First of all, the PushStackInIntercessionChecker needs a refactoring, it's pretty hard to understand what's expected e.g. for a method to count as delegated.

For sure, I spend too long trying to debug what the test was trying to do, why it was failing, counting how many statements appeared in my method bodies, ... The AST checks have a lot of magic and many have multiple different exclusion lists and sometimes also hard coded exceptions in helper methods. They are another area that could use some documentation but you probably don't really touch them too often anyways - especially if you are just using Spoon.

@I-Al-Istannen WDYT?

The restructured method looks fine, that's probably a good compromise. If you decide on that approach I'll adjust my PR.

I could also try making a PR for this issue, I am just a bit weary that I might overlook some invariants or assumptions in the current tests when rewriting them.

[slarse]

I could also try making a PR for this issue, I am just a bit weary that I might overlook some invariants or assumptions in the current tests when rewriting them.

I'll have a go at it, I'll get back to you :)