dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.23k stars 1.58k forks source link

Members and elements #35343

Open bwilkerson opened 5 years ago

bwilkerson commented 5 years ago

The class Member is an implementation detail that clients shouldn't see. Unfortunately, it is currently a leaky abstraction, and that needs to be fixed.

IIRC, when we first implemented Member we decided that it should always be the case that a member should always compare equal to the base element of the member. I don't remember when or why (or whether) that decision was changed, but it's no longer the case. As a result, clients are resorting to (a) asking whether an element is a member and (b) getting the base element from the member.

The best solution would be for clients to never need to know about any of the implementations of Element.

The second best (that I can think of) would be to have public API for getting the "base" or "real" element from any element so that at least clients wouldn't need to reference internal classes.

bwilkerson commented 3 years ago

@scheglov Is the getter Element.declaration intended to be the support for this? If so we can close this issue.

scheglov commented 3 years ago

It seems that there are more than one grievance in the original issue comment.

Adding Element.declartion solves one of them - access of the "real" element.

The other one - equality is not addressed. It looks from reading the code that ElementImpl.== uses location, so will consider a member equal to a raw element, but not vice versa.

  solo_test_XXX() async {
    await assertNoErrorsInCode(r'''
class A<T> {
  void foo() {}
}

void f(A<int> a) {
  a.foo();
}
''');
    var rawElement = findElement.method('foo');
    var intElement = findNode.methodInvocation('a.foo').methodName.staticElement;
    print('rawElement: (${rawElement.runtimeType}) $rawElement');
    print('intElement: (${intElement.runtimeType}) $intElement');
    print('rawElement == intElement: ${rawElement == intElement}');
    print('intElement == rawElement: ${intElement == rawElement}');
    print('rawElement.hashCode: ${rawElement.hashCode}');
    print('intElement.hashCode: ${intElement.hashCode}');
  }

Output:

rawElement: (MethodElementImpl) void foo()
intElement: (MethodMember) void foo()
rawElement == intElement: true
intElement == rawElement: false
rawElement.hashCode: 253995454
intElement.hashCode: 443908654

There are probably more than one possible changes that we might do.

  1. Do nothing and decide that equality of members does not have a meaning, and only Element.declaration should be checked for equality.
  2. Still update ElementImpl.== to return false when the other object is a Member, so make it symmetrical.
  3. Define equality of Member to an Element (including other Member) as equality of declarations. I don't like this because this omits type arguments.
bwilkerson commented 3 years ago
  1. Do nothing and decide that equality of members does not have a meaning ...

I'd prefer that users didn't need to know about members, but if equality of elements has a meaning and equality of members doesn't, then clients need to know when they have a member.

So it sounds like (2) would be the better solution. It seems a little unfortunate because there's a subtlety that users need to be aware of, be we can document it in terms of the type arguments and the need to use declaration if you don't want type arguments to be taken into account in the equality test.

scheglov commented 3 years ago

https://dart-review.googlesource.com/c/sdk/+/210420