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

Members and elements #35343

Open
bwilkerson opened this issue Dec 6, 2018 · 4 comments
Open

Members and elements #35343

bwilkerson opened this issue Dec 6, 2018 · 4 comments
Labels
analyzer-api Issues that impact the public API of the analyzer package analyzer-technical-debt area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@bwilkerson
Copy link
Member

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 bwilkerson added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) analyzer-api Issues that impact the public API of the analyzer package labels Dec 6, 2018
@stereotype441 stereotype441 added the P2 A bug or feature request we're likely to work on label Dec 7, 2018
@bwilkerson
Copy link
Member Author

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

@scheglov
Copy link
Contributor

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
Copy link
Member Author

  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
Copy link
Contributor

copybara-service bot pushed a commit that referenced this issue Jul 19, 2022
…ember anymore.

It was already the case that `Member != ElementImpl`, but now we also
making that `ElementImpl != Member`.

Google3 looks green.

Bug: #35343
Change-Id: I883c96e364fc0452eddc966a2d95312a29dc55f6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/210420
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-api Issues that impact the public API of the analyzer package analyzer-technical-debt area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants