codeql icon indicating copy to clipboard operation
codeql copied to clipboard

Shadowing happens when overriding method

Open jesuistombe opened this issue 1 year ago • 3 comments

Consider the following example:

MyCallable.qll
import java

class CallableByErasure extends Callable {
    override predicate hasQualifiedName(string package, string type, string name) {
      this.isDeclaredIn(package, type) and this.hasName(name)
    }
  
    predicate isDeclaredIn(string package, string type) {
      this.getDeclaringType().getErasure().(RefType).hasQualifiedName(package, type)
    }
}

Query.ql
import java
// import MyCallable

from Call c
where c.getCallee().hasQualifiedName("java.util", "Set<String>", "iterator")
select c

The query correctly finds one instance (see test case below). If I uncomment import MyCallable however, then the query does not return anything. Is this expected behaviour? How do I prevent this from happening?

This is the test case I used:

import java.util.HashSet;
import java.util.Set;

class GenericsClass {
    public void foo() { 
        Set<String> set = new HashSet<String>();
        set.iterator();
    }
}

jesuistombe avatar Feb 06 '24 10:02 jesuistombe

When creating a subclass and overriding a method in it, any value that is a member of the subtype will use the most specific member predicate that matches the signature.

So, in the case above, every Call value is also a CallableByErasure value (since there is no characteristic predicate to narrow the type). This means that every call to hasQualifiedName will use the version defined in CallableByErasure, rather than in the super type. See the language reference for more specifics.

Furthermore, your isDeclaredIn method does not handle generics. If you were to remove <String> from Set<String>, then the result would behave as you expect (even though it is still using the sub-class's predicate).

aeisenberg avatar Feb 06 '24 22:02 aeisenberg

Thanks, that makes sense! Btw, I designed the isDeclaredIn on purpose to ignore generics - my idea was to use this class, whenever I don't care about generics and the usual Callable, whenever I do care about generics.

So what would be the codeql way of doing this? Renaming hasQualifiedName so that it doesn't override or use something like an algebraic datatype to go around this?

jesuistombe avatar Feb 07 '24 08:02 jesuistombe

Hmmm...this is turning into a stylistic and personal preferences question. I'd personally rename hasQualifiedName to something like hasNonGenericQualifiedName or hasErasureQualifiedName. Since the way you are implementing changes the meaning of what a qualified name is. But, there are other ways to achieve the same goal.

aeisenberg avatar Feb 07 '24 22:02 aeisenberg