Skip to content

Conversation

@Strum355
Copy link
Contributor

Previously, if A is a subclass of/implements B and A overrides B.X, find references on A.X would not include the references for the B.X and vice versa.

Addresses https://github.com/sourcegraph/sourcegraph/issues/15699 for lsif-java.

This PR is pending validation of https://github.com/sourcegraph/sourcegraph/pull/20160 with lsif-java output specifically.

@Strum355 Strum355 requested a review from olafurpg April 19, 2021 20:36
@Strum355 Strum355 self-assigned this Apr 19, 2021
@Strum355 Strum355 changed the title emit overriden methods for method SymbolInformation emit overridden methods for method SymbolInformation Apr 19, 2021
Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks good. Can we add unit tests in TargetedSuite that validate that the overrides are defined?

It would be good to check the following cases

  • override other method that's defined in the same file
  • override other method that's defined outside the file, for example by implementing AutoCloseable
  • override interface method
  • override abstract class method
  • override method with type parameters, example
interface Haha<T> {
  void add(T elem);
}
class IntHaha implements Haha<Integer> {
  void add(Integer elem) {}
}

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test cases look great. One comment on how to make them a bit less repetitive, otherwise this is ready to merge soon!

{
case (count, symbols) =>
assertEquals(count, 1)
assertEquals(symbols.apply(0), "example/Parent#Haha#add().")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👨‍🍳 💋

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is surprisingly difficult to implement if you roll your own semantic analysis

@Strum355 Strum355 requested a review from olafurpg April 26, 2021 12:42
@Strum355 Strum355 merged commit c559b3e into main Apr 26, 2021
@Strum355 Strum355 deleted the nsc/overrides branch April 26, 2021 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement SymbolInformation.overrides

3 participants