Method signature and method documentation can be out of sync in IntelliSense

[tausbn]

Describe the bug
This was discussed on Slack where I suggested this could be handled via a compiler warning. Thinking about it some more, however, I think it could equally be handled through a small change in the IDE.

(I wasn't sure whether to classify this as a bug, but it certainly is suprising and potentially confusing behaviour, so I figured "bug" was the most suitable label.)

To Reproduce
Create a query file with the following contents

class SmallNat extends int {
    SmallNat() { this in [0 .. 10] }
}
class Super extends int {
    Super() { this in [1, 2] }
    /** Gets the result of subtracting `bar` from `foo`. */
    int subtract(SmallNat foo, SmallNat bar) { result = foo - bar }
}
class Sub extends Super {
    Sub() { this = 1 }
    override int subtract(SmallNat bar, SmallNat foo) { result = bar - foo }
}
from Sub s, int val
where val = s.subtract(7, 5)
select val

If you hover over the call to s.subtract, the IntelliSense displayed will have the QLDoc associated with Super::subtract, but the parameter list associated with Sub::subtract (and thus incorrectly specify the behaviour of Sub::subtract).

Expected behavior
The parameter list should agree with the QLDoc presented. The most convenient way of achieving this would be to simply reuse the parameter list from Super::subtract when Sub::subtract doesn't supply any QLDoc of its own. In other words, making sure that the parameter list displayed comes from the same method definition as the QLDoc displayed.

[aeisenberg]

Thanks for raising the issue. And under your suggestion, if someone deleted the line Gets the result of subtracting bar from foo, then the hover parameter support will revert back to those for Sub?

[tausbn]

I think either way would make sense (the alternative being still using the Super parameter list). The only place where this would be somewhat confusing would be where the IntelliSense comes from the definition of the parameter (i.e. hovering over the definition of Sub::subtract, it might be confusing to see the parameter list as SmallNat foo, SmallNat bar, as this is not consistent with the method itself). So this would argue in favour of what you propose.

At an actual call, I don't think it matters much, as presumably the parameter lists are each consistent enough to make sense in isolation (but for consistency it's probably best to use the one for the "closest" matching method).

[aeisenberg]

Looks like Code QL follows the behaviour for Typescript in VS Code. In the following snippet, hover over the call to subtract and you will see the docs for Super and the parameter list from Sub. Since all of this information is negotiated from the language server protocol, I'm not sure how much control we have over this.

class Super {
  /**
   * some docs
   */
  subtract(x: number, y: number) {
    return x - y;
  }
}

class Sub extends Super {
  subtract(y: number, x: number) {
    return x - y;
  }
}

new Sub().subtract(1, 2);

[alexet]

We do have full control over this as we are the ones who add the signature to the top of the docs.

Actually implementing this looks non-trivial due to out current internal apis. Currently we have a single method across all ql entities for getting their documentation which wouldn't give the required information in this case where we also need to return the predicate where we got the doc from.