Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

"Find uses" has false positives when a function is overridden in a class hierarchy #19697

Closed
stereotype441 opened this issue Jun 27, 2014 · 9 comments
Closed

"Find uses" has false positives when a function is overridden in a class hierarchy #19697

stereotype441 opened this issue Jun 27, 2014 · 9 comments
Labels
analyzer-server area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. closed-obsolete Closed as the reported issue is no longer relevant P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@stereotype441
Copy link
Member

Consider the following code:
  class Base {
    f() {}
    methodInBase() {
      f();
    }
  }
  class Derived1 extends Base {
    f() {}
    methodInDerived1() {
      f();
    }
  }
  class Derived2 extends Base {
    methodInDerived2() {
      f();
    }
  }
  functionTakingBase(Base b) {
    b.f();
  }
  functionTakingDerived1(Derived1 d1) {
    d1.f();
  }
  functionTakingDerived2(Derived2 d2) {
    d2.f();
  }

A search for uses of Base.f() or Derived1.f() returns all 6 invocations of f(). However some of these invocations are false positives:

  1. A search for uses of Base.f() should not return methodInDerived1(), since it is only possible to call methodInDerived1() on an object of type Derived1 (or a subtype). Therefore methodInDerived1() cannot possibly be a use of Base.f().
  2. A search for uses of Derived1.f() should not return methodInDerived2(), since it is only possible to call methodInDerived2() on an object of type Derived2 (or a subtype), and such an object can't possibly inherit Derived1's implementation of f().

By similar arguments, a search for uses of Base.f() should not return functionTakingDerived1(), and a search for uses of Derived1.f() should not return functionTakingDerived2(). These arguments are less airtight, since in unchecked mode it's possible for any type to be passed to any function. However I would argue that these unchecked uses violate programmer intent and so they're very unlikely what the user is seeking.
@bwilkerson
Copy link
Member

Added this to the 1.6 milestone.
Removed Priority-Unassigned, Area-Editor labels.
Added Priority-Medium, Area-Analyzer labels.

@kasperl
Copy link

kasperl commented Jul 10, 2014

Removed this from the 1.6 milestone.
Added Oldschool-Milestone-1.6 label.

@bwilkerson
Copy link
Member

Set owner to @scheglov.

@kasperl
Copy link

kasperl commented Aug 4, 2014

Removed Oldschool-Milestone-1.6 label.

@scheglov
Copy link
Contributor

  Paul, Brian, could you please review the following test code and see if it satisfies your expectations about search results?

  test_method_whenExtends() {
    // TODO(scheglov) implement corresponding search logic
    addTestFile('''
class A {
  mmm() {}
  use_mmm() {
    mmm(1);
  }
}
class B extends A {
  mmm() {}
  use_mmm() {
    mmm(2);
  }
}
class C extends B {
  mmm() {} // of C
  use_mmm() {
    mmm(3);
  }
}
class D extends A {
  mmm() {}
  use_mmm() {
    mmm(4);
  }
}
class E extends C {
  use_mmm() {
    mmm(5);
  }
}
class F extends C {
  mmm() {}
  use_mmm() {
    mmm(6);
  }
}
main(A a, B b, C c, D d, E e, F f) {
  a.mmm(10);
  b.mmm(20);
  c.mmm(30);
  d.mmm(40);
  e.mmm(50);
  f.mmm(60);
}
''');
    return findElementReferences('mmm() {} // of C', false).then((_) {
      expect(searchElement.kind, ElementKind.METHOD);
      // unqualified
      {
        // C extends A via B with B.mmm, so A cannot call C.mmm
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(1)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(2)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(3)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(4)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(5)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(6)');
      }
      // qualified
      {
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(10)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(20)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(30)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(40)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(50)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(60)');
      }
    });
  }

  test_method_whenImplements() {
    // TODO(scheglov) implement corresponding search logic
    addTestFile('''
class A {
  mmm() {} // of A
  use_mmm() {
    mmm(1);
  }
}
class B implements A {
  mmm() {} // of B
  use_mmm() {
    mmm(2);
  }
}
class C implements B {
  mmm() {}
  use_mmm() {
    mmm(3);
  }
}
main(A a, B b, C c) {
  a.mmm(10);
  b.mmm(20);
  c.mmm(30);
}
''');
    var futureA = findElementReferences('mmm() {} // of A', false).then(() {
      expect(searchElement.kind, ElementKind.METHOD);
      // unqualified
      {
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(1)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(2)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(3)');
      }
      // qualified
      {
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(10)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(20)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(30)');
      }
    });
    var futureB = findElementReferences('mmm() {} // of B', false).then((_) {
      expect(searchElement.kind, ElementKind.METHOD);
      // unqualified
      {
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(1)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(2)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(3)');
      }
      // qualified
      {
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(10)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(20)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(30)');
      }
    });
    return Future.wait([futureA, futureB]);
  }

cc @bwilkerson.
Added NeedsInfo label.

@stereotype441
Copy link
Member Author

I'm troubled by the "whenExtends" test code because not only do all the called methods have the same name (mmm), but all the calling methods have the same name as well (use_mmm). This means that in principle, once could make even tighter inferences about what's callable than what I've requested in this bug report (for example, even B.use_mmm can't call C.use_mmm, because the only way B.use_mmm could be called on an object derived from C is via a super call, and there are no super calls in this code).

I'd feel much more comfortable if we gave all the use_mmm calls different names (as I did in the bug report). However even with that change I believe some of the test code is incorrect. See my note (marked "(*)") below:

  test_method_whenExtends() {
    // TODO(scheglov) implement corresponding search logic
    addTestFile('''
class A {
  mmm() {}
  use_mmm_a() {
    mmm(1);
  }
}
class B extends A {
  mmm() {}
  use_mmm_b() {
    mmm(2);
  }
}
class C extends B {
  mmm() {} // of C
  use_mmm_c() {
    mmm(3);
  }
}
class D extends A {
  mmm() {}
  use_mmm_d() {
    mmm(4);
  }
}
class E extends C {
  use_mmm_e() {
    mmm(5);
  }
}
class F extends C {
  mmm() {}
  use_mmm_f() {
    mmm(6);
  }
}
main(A a, B b, C c, D d, E e, F f) {
  a.mmm(10);
  b.mmm(20);
  c.mmm(30);
  d.mmm(40);
  e.mmm(50);
  f.mmm(60);
}
''');
    return findElementReferences('mmm() {} // of C', false).then((_) {
      expect(searchElement.kind, ElementKind.METHOD);
      // unqualified
      {
        // C extends A via B with B.mmm, so A cannot call C.mmm
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(1)');
        // (*) I don't follow this logic. Code in some other file could do:
        // new C().use_mmm_a(), in which case the call mmm(1) would go to C.mmm.

        assertHasResult(SearchResultKind.INVOCATION, 'mmm(2)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(3)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(4)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(5)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(6)');
      }
      // qualified
      {
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(10)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(20)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(30)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(40)');
        assertHasResult(SearchResultKind.INVOCATION, 'mmm(50)');
        assertNoResult(SearchResultKind.INVOCATION, 'mmm(60)');
      }
    });
  }

test_method_whenImplements() seems correct to me.

@scheglov
Copy link
Contributor

Partial (very partial) fix.
https://codereview.chromium.org/575613002

Added Started label.

@scheglov
Copy link
Contributor

Rollback of the previous CL
https://codereview.chromium.org/560133003

@scheglov
Copy link
Contributor

OK, there are (probably) use case for this, but I'm not going to land it now.
But here is the patch that implements what this issue requests (land the rolledback CL first).

Attachment:
20140915-filter-using-target-class.diff (32.83 KB)

Removed the owner.
Removed Type-Defect label.
Added Type-Enhancement, Analyzer-Server, Triaged labels.

@stereotype441 stereotype441 added Type-Enhancement area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-server labels Sep 16, 2014
@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug and removed triaged labels Mar 1, 2016
@lrhn lrhn added the closed-obsolete Closed as the reported issue is no longer relevant label May 9, 2023
@lrhn lrhn closed this as completed May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
analyzer-server area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. closed-obsolete Closed as the reported issue is no longer relevant P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@kevmoo @stereotype441 @scheglov @kasperl @lrhn @bwilkerson