Kotlin: Change Modifiable::isPublic to not cover Kotlin internal members

[tamasvajk]

No description provided.

[atorralba]

A little surprising at first glance that the internal method has the public modifier but isPublic doesn't hold for it. But I guess it makes sense if we want isPublic to exclude Kotlin internal methods. LGTM.

[igfoo]

Hmm, hasModifier is also public, so we should probably do something like (untested)

private predicate hasModifierRefined(Modifiable e, Modifier m) {
    hasModifier(e, m)
    // Kotlin "internal" elements may also get "public" modifiers, so we want to filter those out
    and not exists(Modifier m2 | hasModifier(e, m2) and modifiers(m, "public") and modifiers(m2, "internal"))
}

instead, and use that everywhere instead of hasModifier.

It's also a bit odd that class Modifier doesn't have a predicate to get the name of the modifier. That would probably allow this to be tidied up a bit, and remove some calls to dbscheme relations.

[tamasvajk]

I wanted to introduce as little change as possible. hasModifier has a comment on top of it:

  /**
   * Holds if this element has modifier `m`.
   *
   * For most purposes, the more specialized predicates `isAbstract`, `isPublic`, etc.
   * should be used.
   *
   * Both this method and those specialized predicates take implicit modifiers into account.
   * For instance, non-default instance methods in interfaces are implicitly
   * abstract, so `isAbstract()` will hold for them even if `hasModifier("abstract")`
   * does not.
   */
  predicate hasModifier(string m) { modifiers(this.getAModifier(), m) }

So in my view it's okay to return the public modifier even if isPublic doesn't hold. That feels similar to the abstract case.

I think the best option would be making hasModifier private, but it's used heavily for concatenating all modifiers. (For all other purposes we anyways should go with the specialized predicates, which in some case are overridden (for example Method::isPublic).)

What do you think?

[igfoo]

My proposal is that codeql treats internal predicates as simply internal. The only reason that we sometimes get a public modifier in the database is that the Java extractor isn't able to tell that they are internal, and it isn't easy to patch that up before the database is created, and so I suggest that we patch it at the lowest level of QL, i.e. wrapping the relevant table and only using the wrapper elsewhere.

As far as queries are concerned, there should therefore be no public modifier, whether you ask isPublic or hasModifier. This isn't a case of it being implicit, it simply shouldn't exist.

An alternative approach would be to always extract internal entities as also being public in the Kotlin extractor, so entities would always have both modifiers consistently. This would presumably be consistent with Java's world view, but I think that it would be more confusing on balance.

[tamasvajk]

I see what you mean. How do you like e66d2dd?

[igfoo]

I don't think that we need to change this. this.getAModifier() calls Modifier.getElement() which has already done the filtering in your change above.

[tamasvajk]

Thanks for spotting this. I removed it.

[tamasvajk]

Thanks for spotting this. I removed it.