See references show unrelated results

[FMorschel]

In my screenshot I'll sepparate this repro into three files so the VSCode UI helps a bit to see the problem.

class A {
  void m1() {}
  void m2() {
    m1();
  }
}

class B extends A {
  @override
  void m2() {
    m1();
  }

  @override
  void m1() {
    print('B.m1');
  }
}

void f1(B m) {
  m
    ..m2()
    ..m1();
}

class C extends A {
  @override
  void m1 () {}
  @override
  void m2() {
    m1();
  }
}

Here you can see that in the above structure B and C are subclasses of A. In this example, m1 is called under A.m2, B.m2, C.m2 and under f1.

If you look for references of A.m1 you get all calls to it in the list.

Now the problem; if you ask for references for B.m1 or C.m1 you get the same unrelated results:

Here you can see that when asking for references of C.m1 I get instance calls that would never be C. Inside B and f1 in this case.

The same happens if you ask for references of B.m1, you get the call under C.m2 which could never be a B instance.

---

I'd like to ask for this to be worked on so that only B instances (or another subclass instance) would show. For C.m1 for example, I was expecting only the call inside A.m2 and C.m2. Definitelly not the call under f1 or B.m2.

[dart-github-bot]

Summary: The "See References" feature in the Dart editor incorrectly shows unrelated results when searching for references to overridden methods in subclasses. For example, searching for references to B.m1 shows calls to m1 within C.m2 and f1, even though these calls could never be to a B instance.

[scheglov]

@DanTup

[bwilkerson]

If I'm understanding the example correctly, I think it is possible. Consider

class D extends C implements B {}

An instance of D could be passed to f1 and the methods in C would be invoked.

Because of the ability to implement almost any interface, it's very difficult to prove (in general) that it's impossible to ever invoke any override of an inherited method, so we don't try.

In other words, this is working as intended, and I think we're unlikely to attempt to change it.

[FMorschel]

I see your point. But this request does make sense for final/sealed classes.

I've just tested the same code and it gives the same results. With either C or B being final there was no possible case similar to what you suggested.

I would still like for it to scan the type hierarchy to try and find a match so that case is possible to show, but my feelings are not that strong and the code where I found this behaviour could easily be changed to final classes.

Just to be clear, I would expect that result of ..m1() to show up in either B or A and there is a way to "go to super" so in C we could get there.

---

Edit

The thing that I understand of your example but I do find it weird is that if I have say:

class E {
  void m1() {}
}

You could also say it could in the future have a subclass that extends/implements A and that would also need to show the same reference. I know this makes less sense than your example but that's the whole point.

[FMorschel]

One more thing:

Your example is flawed considering that both B and C could have gotten a method with the same name and signatures that didn't match like int a({required num v}) => v.toInt(); and String a(dynamic v) => v.toString(); (here the named parameter would make them not even be able to use Never).

This would mean that (at least today) it is impossible to have a type that would match both, but the current "See references" would still show the same calls.

[bwilkerson]

... this request does make sense for final/sealed classes.

Yes, certain combinations of final and sealed make sense, and we could consider adding knowledge of those modifiers to the algorithm.

You could also say it could in the future have a subclass ...

True, but a feature like this shouldn't try to reason about possible futures, only about the code that exists today.

The point of the example is that we can't definitively know whether ..m1() might invoke the implementation in C without doing a whole-world analysis, and a whole-world analysis is known to be too expensive (and the analyzer, by design, never assumes that it has the whole world available to it).

We could always find ways to improve the algorithm. Some of them might be performant and some wouldn't be. That then raises the question of whether we'd end up in an uncanny valley where the imprecision was worse than if we don't try to be so precise.

[FMorschel]

True, but a feature like this shouldn't try to reason about possible futures, only about the code that exists today.

That's more or less what your example and current implementation do here. They assume that C and B don't have conflicting declarations and show every call everywhere.

I know the costs can be high for analysis but this can be really confusing.

I opened this issue after a talk with DanTup on the extension Discord about the following image:

I'm looking for the references for this inuteis getter. It overrides a base Dao class that every other class in the right extends. I was expecting to see this class and others below it (and uses), not its "siblings". In my case, none of these has a subclass and all of the calls for inuteis happen inside this single file so I was really thrown back by the number of resulting references I got until I understood what was happening.

[DanTup]

I hadn't considered the point Brian made above when we first discussed this (a class using implements) so I agree, I think it would be wrong to filter these out. If you can possibly construct code that would be called at that location, then not considering it a reference would look like a bug.

There have been requests to VS Code/LSP to allow these kinds of results to be grouped. I don't know if that would really help here (because I'm not sure what the groups would be), but maybe if it's ever implemented we could come up with a way to split/prioritise the results in a way that makes sense? 🤔

[FMorschel]

we could come up with a way to split/prioritise the results in a way that makes sense?

Even though I like the idea, I don't think it would make much sense. For the splitting to make any difference for this case we would need to have the full analysis anyway so until we decide that is needed/will be implemented, it won't make much sense I don't think.

I'll leave this open for the request to consider final/sealed classes on the analysis at least. Thanks for your thoughts!