Fix: Correctly resolve in scope variables of instanceof patterns #6645

[Luro02]

I have not looked at the JLS, so I might have missed something.

The code in the SiblingsFunction introduced in #6444 has been removed, because I think it does not belong there? Seems wrong to return the instanceof variables there.

[MartinWitt]

@Luro02 mind using our assertions and testing utils? Take a look at https://github.com/INRIA/spoon/blob/master/CONTRIBUTING.md or ask the next mistral instance.

[Luro02]

@Luro02 mind using our assertions and testing utils? Take a look at https://github.com/INRIA/spoon/blob/master/CONTRIBUTING.md or ask the next mistral instance.

Are you referring to that assertNotNull(decl); or do you have a problem with the assertThat? I am fine with changing the assertThat to a assertSame to conform with the rest of the test class.

[SirYwell]

This looks somewhat incomplete, especially with more complex if conditions. For example:

• if (!(!(a instanceof String s))) { ... }
• if (!(!(!(a instanceof String s)))) { ... }
• if (a instanceof String s || b) { ... }
• if (a instanceof String s && b) { ... }
• if (!(a instanceof String s1) || !(b instanceof String s2)) { ... }

Could you expand the tests to cover these scenarios?

It feels like we need full DA/DU analysis here to properly support that feature...

[Luro02]

This is likely going to be more involved than I expected, so I am converting this to a draft for now.

[Luro02]

I went through the JLS https://docs.oracle.com/javase/specs/jls/se25/html/jls-6.html#jls-6.3.1 and implemented the pattern variable resolution accordingly.

The pushed implementation seems to be mostly working, except for switches, which I have not implemented yet. There are a lot of tests missing, many things are not covered, but I am unsure how these should look like.

The Contributing Guidelines suggest using assertThat, which is kind of useless here, but they should work for assertThat(decl).isSame() and isNull.

The main problem would be how to get the declarations and associated references from a piece of code. I am tempted to simply fetch a list of all declarations, and all references, then hardcode which indices of the references should resolve to which declaration, but maybe there is a cleaner way?

[Luro02]

Here is the current state of the PR:

The original code starts at the given element, checks if the desired variable is in scope, and if not, moves up to the parent of the element. Same repeats until it reaches the package declaration or runs out of parents.

For instanceof patterns, the scope changes through the parents, e.g. a pattern that was in scope will not be in scope anymore when negated (it would then match on false). The final scope of the pattern is defined by the statement containing the expression with the instanceof pattern. To figure out which instanceof pattern is in scope, one has to start at the pattern definition, then walk up the element tree.

My implementation does the following:

The loop that moves up the tree keeps track of the current scopes. When it moves up to the parent, it updates the scopes with new variables that have become available (like one declared in a sibling) or removes ones that no longer apply. The code can then look at the scopes to find any variable that is in scope.

When moving to the parent, it only knows the variables from the child, but there might be siblings that introduce variables too. The code currently hardcodes which siblings are explored, but I think always applying the siblings function might be more sensible here.

For siblings it will try to find one variable declaration (a leaf in the tree), then move up from there. Given that it uses the same code that is used for the child, it will automatically explore the other branches.

---

The code has become rather complicated, so the next steps would be to simplify where applicable. I found a few bugs in the old code as well, e.g. it used filterChildren to get the variables of a case, but then a variable declared in the block case ..: { /* here */ } would be discovered in the following blocks, which is wrong.

[Luro02]

It has been a while since I last worked on the PR, because I was busy.

I think the implementation is far enough that it is ready for an initial review.
All tests are passing locally (let's see what the CI thinks), and most of the concerning pieces of code have been resolved/cleaned up.

It looks like I am not supposed to add custom test assertions? Will have to investigate where I should add them.

What remains to be done?

• Go through the JLS and check that the code matches it, I think it does, but maybe I missed something
• Add support for patterns in a ? b : c
• Add support for switch expressions and their patterns, I skipped these JLS sections, but maybe I already covered them with the current code
• Add more tests to ensure the code always behaves correctly (not sure how much time I will spend on this, maybe an AI agent can find some edge cases I might have forgotten).

What I need feedback on:

• There are two FIXMEs, one of them is just the definition of "cannot complete normally" while the other requires some form of control flow analysis. The latter I would rather not implement, sounds like a ton of work.
• I reused the old code that used the SiblingsFunction to resolve the variable declarations for things like executables, for loops, and so on. Here I would like to know whether I should keep it as-is or apply the mentioned fix (see TODO in the code)

[I-Al-Istannen]

Thank you very much! I added some nitpicks and some thoughts, but I don't think I have completely grasped it.

I think I found one omission (if without a then but a non-terminating else) and some other small comments.

[I-Al-Istannen]

We should under-approximate this, if the JLS implementation requires too much. I asked some AI and it spit out 60 lines of fun code, so I guess technically it is doable, but if you have a save under-approximation that's fine by me. Do keep a FIXME there then, though :)

[monperrus]

we're almost there to merge, @I-Al-Istannen WDYT? Thanks!

[I-Al-Istannen]

Might be nice if @SirYwell would take another cursory glance, but I think it looks fine otherwise

[SirYwell]

I wasn't able to take a closer look, but I noticed a few things.

[SirYwell]

That sounds rather odd, why do we need a comparator? The identity hash code also doesn't have many guarantees, so depending on what it is used for, the results might be wrong (and the comment definitely is).

[Luro02]

Do you have a suggestion on how to improve this? Like I said, assertj wants a comparator to change the equality behaviour, maybe there is a function I have missed?

[SirYwell]

I didn't find anything straightforward, but zipSatisfy combined with an assertThat(a).isSameAs(b) might work? From my understanding this would require an additional assertion that the sizes match.

[Luro02]

I don't see how this would be better than my current implementation which does exactly what is desired.

[SirYwell]

Comparing the identity hash code has different semantics than comparing by ==, that is System.identityHashCode(a) == System.identityHashCode(b) does not imply a == b. Using the identity hash code is not reliable.