Validation of `@protected` method invocations.

Validation of `@protected` method invocations.

* Defines `isProtected` accessors.
* Adds a simple mechanism to our test `AnalysisContextFactory` allowing us to contribute package resolution to test cases.
* Adds `meta` awareness to `ResolverTest`.

NB: this *only* adds method invocation support.  I'll generalize to all members in a follow-up.  There's enough here though that I wanted to get some early feedback.

BUG=
R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/sdk/commit/84f32fe5a653824b127ceed729d5831191ff06a6

[pquitslund, 2016-02-23 17:17:37]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com

[Brian Wilkerson, 2016-02-23 17:45:56]

LGTM

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/lib/dart/eleme...
File pkg/analyzer/lib/dart/element/element.dart (right):

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/lib/dart/eleme...
pkg/analyzer/lib/dart/element/element.dart:818: * Return `true` if this
annotation marks the associated method as being
"method" --> "member"

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:616: Element element =
node.methodName.staticElement;
Given that this is a hint, I suspect that we want to use "bestElement" rather
than "staticElement".

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:635:
!invokingClass.type.isSubtypeOf(definingClass.type)) {
The comment for the annotation says "extend or mix in", but the use of
"isSubtypeOf" will also allow classes that implement the interface of the
defining class, such as:

class Foo {
  @protected
  void m1() {}
}

class Bar implements Foo {
  void m2() {
    m1(); // Expect a hint here.
  }
}

I think we need to explicitly walk the superclass chain (looking at mixins) in
order to get the right behavior.

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/resolver_test.dart:3618: class B extends A {
We should test "with" and "implements" relationships.

[pquitslund, 2016-02-23 22:59:07]

Comments addressed.

Maybe take another look at resolver to vet my superclass handling?

Thanks!

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/lib/dart/eleme...
File pkg/analyzer/lib/dart/element/element.dart (right):

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/lib/dart/eleme...
pkg/analyzer/lib/dart/element/element.dart:818: * Return `true` if this
annotation marks the associated method as being
On 2016/02/23 17:45:56, Brian Wilkerson wrote:
> "method" --> "member"

Done.

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:616: Element element =
node.methodName.staticElement;
On 2016/02/23 17:45:56, Brian Wilkerson wrote:
> Given that this is a hint, I suspect that we want to use "bestElement" rather
> than "staticElement".

Done.

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:635:
!invokingClass.type.isSubtypeOf(definingClass.type)) {
On 2016/02/23 17:45:56, Brian Wilkerson wrote:
> The comment for the annotation says "extend or mix in", but the use of
> "isSubtypeOf" will also allow classes that implement the interface of the
> defining class, such as:
> 
> class Foo {
>   @protected
>   void m1() {}
> }
> 
> class Bar implements Foo {
>   void m2() {
>     m1(); // Expect a hint here.
>   }
> }
> 
> I think we need to explicitly walk the superclass chain (looking at mixins) in
> order to get the right behavior.

Fixed.

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1723243002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/resolver_test.dart:3618: class B extends A {
On 2016/02/23 17:45:56, Brian Wilkerson wrote:
> We should test "with" and "implements" relationships.

Done.

[Brian Wilkerson, 2016-02-23 23:42:58]

LGTM

https://codereview.chromium.org/1723243002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1723243002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:625: if (decl == null) {
Are we handling static methods as a call site?

class A {
  @protected m1() {}
}
class B extends A {
  static m2(A a) => a.m1();
}

https://codereview.chromium.org/1723243002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1723243002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/resolver_test.dart:3581: void
test_protected_method_call_1() {
nit: there's a naming convention here of test methods starting with "test_"
followed by the name of the hint code (converted to camel-case), followed a
description of the case being tested. Perhaps rename to something like

    test_invalidUseOfProtectedMember_method_topLevelFunction

the next method might be something like

    test_invalidUseOfProtectedMember_method_instanceMethod_unrelatedClass

(Maybe it's too much, but the naming scheme helps me think of cases that should
be tested. Ymmv.)

https://codereview.chromium.org/1723243002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/resolver_test.dart:3626: void
test_protected_method_call_OK_1() {
nit: tests to ensure that a hint is not generated have generally gone in
NonHintCodeTest.

[pquitslund, 2016-02-24 17:12:17]

Thanks!

I'll follow-up with support for fields, getters/setters, etc.

https://codereview.chromium.org/1723243002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1723243002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:625: if (decl == null) {
Yep.  Test added!

https://codereview.chromium.org/1723243002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1723243002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/resolver_test.dart:3581: void
test_protected_method_call_1() {
On 2016/02/23 23:42:58, Brian Wilkerson wrote:
> nit: there's a naming convention here of test methods starting with "test_"
> followed by the name of the hint code (converted to camel-case), followed a
> description of the case being tested. Perhaps rename to something like
> 
>     test_invalidUseOfProtectedMember_method_topLevelFunction
> 
> the next method might be something like
> 
>     test_invalidUseOfProtectedMember_method_instanceMethod_unrelatedClass
> 
> (Maybe it's too much, but the naming scheme helps me think of cases that
should
> be tested. Ymmv.)

Done.

[pquitslund, 2016-02-24 17:25:47]

Description was changed from

==========
Validation of `@protected` method invocations.

* Defines `isProtected` accessors.
* Adds a simple mechanism to our test `AnalysisContextFactory` allowing us to
contribute package resolution to test cases.
* Adds `meta` awareness to `ResolverTest`.

NB: this *only* adds method invocation support.  I'll generalize to all members
in a follow-up.  There's enough here though that I wanted to get some early
feedback.

BUG=
==========

to

==========
Validation of `@protected` method invocations.

* Defines `isProtected` accessors.
* Adds a simple mechanism to our test `AnalysisContextFactory` allowing us to
contribute package resolution to test cases.
* Adds `meta` awareness to `ResolverTest`.

NB: this *only* adds method invocation support.  I'll generalize to all members
in a follow-up.  There's enough here though that I wanted to get some early
feedback.

BUG=
R=brianwilkerson@google.com

Committed:
https://github.com/dart-lang/sdk/commit/84f32fe5a653824b127ceed729d5831191ff06a6
==========

[pquitslund, 2016-02-24 17:25:49]

Committed patchset #2 (id:20001) manually as
84f32fe5a653824b127ceed729d5831191ff06a6 (presubmit successful).