Fix scope resolution of metadata on type parameters

Previously, when encountering identifiers in metadata on a class's type parameter, the analyzer would resolve them using the type parameter scope, but then fall back on using implicit `this`. The CFE would resolve them using the class body scope. In both cases, the end result was that the annotation could refer to static class members. This change brings the behavior of both the analyzer and the CFE in line with the spec, by preventing the use of implicit `this` in these annotations, and resolving them in the type parameter scope. This is not expected to break any code in practice, because annotations on type parameters are rare, as are annotations referring to static class members, and the overlap between these two should be negligibly small. Fixes dart-lang/language#1790. Bug: dart-lang/language#1790 Change-Id: Ibe5a421e04a53d29074a8b1509e1390658ed72e5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/210040 Commit-Queue: Paul Berry <paulberry@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
skia-flutter-autoroll · Aug 17, 2021 · f5a3bce · f5a3bce
1 parent c9d954e
commit f5a3bce
Show file tree

Hide file tree

Showing 17 changed files with 388 additions and 112 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,26 @@
+## 2.15.0
+
+### Language
+
+- Annotations on type parameters of classes can no longer refer to class members
+  without a prefix.  For example, this used to be permitted:
+
+  ```dart
+  class C<@Annotation(foo) T> {
+    static void foo() {}
+  }
+  ```
+
+  Now, the reference must be qualified with the class name, i.e.:
+
+  ```dart
+  class C<@Annotation(C.foo) T> {
+    static void foo() {}
+  }
+  ```
+
+  This brings the implementation behavior in line with the spec.
+
 ## 2.14.0
 
 ### Language

diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart
@@ -2007,17 +2007,6 @@ class ResolverVisitor extends ResolverBase with ErrorDetectionHelpers {
   @override
   void visitTypeName(TypeName node) {}
 
-  @override
-  void visitTypeParameter(TypeParameter node) {
-    var previousThisType = _thisType;
-    try {
-      _setupThisType();
-      super.visitTypeParameter(node);
-    } finally {
-      _thisType = previousThisType;
-    }
-  }
-
   @override
   void visitVariableDeclaration(VariableDeclaration node) {
     _variableDeclarationResolver.resolve(node as VariableDeclarationImpl);

diff --git a/pkg/analyzer/test/src/diagnostics/undefined_identifier_test.dart b/pkg/analyzer/test/src/diagnostics/undefined_identifier_test.dart
@@ -43,61 +43,6 @@ void f(int value) {
 }
 
 mixin UndefinedIdentifierTestCases on PubPackageResolutionTest {
-  test_annotation_favors_scope_resolution_over_this_resolution_class() async {
-    // If an annotation on a class type parameter cannot be resolved using the
-    // normal scope resolution mechanism, it is resolved via implicit `this`.
-    // Note: this behavior doesn't match the spec so we may change it - see
-    // https://github.com/dart-lang/language/issues/1790
-    await assertNoErrorsInCode('''
-class C<@Annotation.function(foo) @Annotation.type(B) T> {
-  static void foo() {}
-  static void B() {}
-}
-class B {}
-class Annotation {
-  const Annotation.function(void Function() f);
-  const Annotation.type(Type t);
-}
-''');
-  }
-
-  test_annotation_favors_scope_resolution_over_this_resolution_extension() async {
-    // If an annotation on an extension type parameter cannot be resolved using
-    // the normal scope resolution mechanism, it is resolved via implicit
-    // `this`.  Note: this behavior doesn't match the spec so we may change it -
-    // see https://github.com/dart-lang/language/issues/1790
-    await assertNoErrorsInCode('''
-extension E<@Annotation.function(foo) @Annotation.type(B) T> on C {}
-class C {
-  static void foo() {}
-  static void B() {}
-}
-class B {}
-class Annotation {
-  const Annotation.function(void Function() f);
-  const Annotation.type(Type t);
-}
-''');
-  }
-
-  test_annotation_favors_scope_resolution_over_this_resolution_mixin() async {
-    // If an annotation on a mixin type parameter cannot be resolved using the
-    // normal scope resolution mechanism, it is resolved via implicit `this`.
-    // Note: this behavior doesn't match the spec so we may change it - see
-    // https://github.com/dart-lang/language/issues/1790
-    await assertNoErrorsInCode('''
-mixin M<@Annotation.function(foo) @Annotation.type(B) T> {
-  static void foo() {}
-  static void B() {}
-}
-class B {}
-class Annotation {
-  const Annotation.function(void Function() f);
-  const Annotation.type(Type t);
-}
-''');
-  }
-
   test_annotation_references_static_method_in_class() async {
     await assertErrorsInCode('''
 @Annotation(foo)
@@ -114,18 +59,19 @@ class Annotation {
   }
 
   test_annotation_references_static_method_in_class_from_type_parameter() async {
-    // It is allowed for an annotation of a class type parameter to refer to
-    // a method in a class (note: this doesn't match the spec but we currently
-    // test it to make sure we match CFE behavior - see
-    // https://github.com/dart-lang/language/issues/1790)
-    await assertNoErrorsInCode('''
+    // It not is allowed for an annotation of a class type parameter to refer to
+    // a method in a class.
+    await assertErrorsInCode('''
 class C<@Annotation(foo) T> {
   static void foo() {}
 }
 class Annotation {
   const Annotation(dynamic d);
 }
-''');
+''', [
+      error(CompileTimeErrorCode.UNDEFINED_IDENTIFIER, 20, 3),
+      error(CompileTimeErrorCode.CONST_WITH_NON_CONSTANT_ARGUMENT, 20, 3),
+    ]);
   }
 
   test_annotation_references_static_method_in_extension() async {
@@ -144,18 +90,19 @@ class Annotation {
   }
 
   test_annotation_references_static_method_in_extension_from_type_parameter() async {
-    // It is allowed for an annotation of a mixin type parameter to refer to
-    // a method in a class (note: this doesn't match the spec but we currently
-    // test it to make sure we match CFE behavior - see
-    // https://github.com/dart-lang/language/issues/1790)
-    await assertNoErrorsInCode('''
+    // It is not allowed for an annotation of an extension type parameter to
+    // refer to a method in a class.
+    await assertErrorsInCode('''
 extension E<@Annotation(foo) T> on T {
   static void foo() {}
 }
 class Annotation {
   const Annotation(dynamic d);
 }
-''');
+''', [
+      error(CompileTimeErrorCode.CONST_WITH_NON_CONSTANT_ARGUMENT, 24, 3),
+      error(CompileTimeErrorCode.UNDEFINED_IDENTIFIER, 24, 3),
+    ]);
   }
 
   test_annotation_references_static_method_in_mixin() async {
@@ -174,18 +121,80 @@ class Annotation {
   }
 
   test_annotation_references_static_method_in_mixin_from_type_parameter() async {
-    // It is allowed for an annotation of a mixin type parameter to refer to
-    // a method in a class (note: this doesn't match the spec but we currently
-    // test it to make sure we match CFE behavior - see
-    // https://github.com/dart-lang/language/issues/1790)
-    await assertNoErrorsInCode('''
+    // It is not allowed for an annotation of a mixin type parameter to refer to
+    // a method in a class.
+    await assertErrorsInCode('''
 mixin M<@Annotation(foo) T> {
   static void foo() {}
 }
 class Annotation {
   const Annotation(dynamic d);
 }
-''');
+''', [
+      error(CompileTimeErrorCode.UNDEFINED_IDENTIFIER, 20, 3),
+      error(CompileTimeErrorCode.CONST_WITH_NON_CONSTANT_ARGUMENT, 20, 3),
+    ]);
+  }
+
+  test_annotation_uses_scope_resolution_class() async {
+    // If an annotation on a class type parameter cannot be resolved using the
+    // normal scope resolution mechanism, it is not resolved via implicit
+    // `this`.
+    await assertErrorsInCode('''
+class C<@Annotation.function(foo) @Annotation.type(B) T> {
+  static void foo() {}
+  static void B() {}
+}
+class B {}
+class Annotation {
+  const Annotation.function(void Function() f);
+  const Annotation.type(Type t);
+}
+''', [
+      error(CompileTimeErrorCode.UNDEFINED_IDENTIFIER, 29, 3),
+      error(CompileTimeErrorCode.CONST_WITH_NON_CONSTANT_ARGUMENT, 29, 3),
+    ]);
+  }
+
+  test_annotation_uses_scope_resolution_extension() async {
+    // If an annotation on an extension type parameter cannot be resolved using
+    // the normal scope resolution mechanism, it is not resolved via implicit
+    // `this`.
+    await assertErrorsInCode('''
+extension E<@Annotation.function(foo) @Annotation.type(B) T> on C {}
+class C {
+  static void foo() {}
+  static void B() {}
+}
+class B {}
+class Annotation {
+  const Annotation.function(void Function() f);
+  const Annotation.type(Type t);
+}
+''', [
+      error(CompileTimeErrorCode.CONST_WITH_NON_CONSTANT_ARGUMENT, 33, 3),
+      error(CompileTimeErrorCode.UNDEFINED_IDENTIFIER, 33, 3),
+    ]);
+  }
+
+  test_annotation_uses_scope_resolution_mixin() async {
+    // If an annotation on a mixin type parameter cannot be resolved using the
+    // normal scope resolution mechanism, it is not resolved via implicit
+    // `this`.
+    await assertErrorsInCode('''
+mixin M<@Annotation.function(foo) @Annotation.type(B) T> {
+  static void foo() {}
+  static void B() {}
+}
+class B {}
+class Annotation {
+  const Annotation.function(void Function() f);
+  const Annotation.type(Type t);
+}
+''', [
+      error(CompileTimeErrorCode.UNDEFINED_IDENTIFIER, 29, 3),
+      error(CompileTimeErrorCode.CONST_WITH_NON_CONSTANT_ARGUMENT, 29, 3),
+    ]);
   }
 
   @failingTest

diff --git a/pkg/front_end/lib/src/fasta/builder/class_builder.dart b/pkg/front_end/lib/src/fasta/builder/class_builder.dart
@@ -354,12 +354,12 @@ abstract class ClassBuilderImpl extends DeclarationBuilderImpl
           delayedActionPerformers, synthesizedFunctionNodes);
     }
 
-    MetadataBuilder.buildAnnotations(
-        isPatch ? origin.cls : cls, metadata, library, this, null, fileUri);
+    MetadataBuilder.buildAnnotations(isPatch ? origin.cls : cls, metadata,
+        library, this, null, fileUri, library.scope);
     if (typeVariables != null) {
       for (int i = 0; i < typeVariables!.length; i++) {
-        typeVariables![i].buildOutlineExpressions(
-            library, this, null, coreTypes, delayedActionPerformers);
+        typeVariables![i].buildOutlineExpressions(library, this, null,
+            coreTypes, delayedActionPerformers, scope.parent!);
       }
     }
 

diff --git a/pkg/front_end/lib/src/fasta/builder/field_builder.dart b/pkg/front_end/lib/src/fasta/builder/field_builder.dart
@@ -400,8 +400,8 @@ class SourceFieldBuilder extends MemberBuilderImpl implements FieldBuilder {
     _fieldEncoding.completeSignature(coreTypes);
 
     for (Annotatable annotatable in _fieldEncoding.annotatables) {
-      MetadataBuilder.buildAnnotations(
-          annotatable, metadata, library, classBuilder, this, fileUri);
+      MetadataBuilder.buildAnnotations(annotatable, metadata, library,
+          classBuilder, this, fileUri, classBuilder?.scope ?? library.scope);
     }
 
     // For modular compilation we need to include initializers of all const

diff --git a/pkg/front_end/lib/src/fasta/builder/function_builder.dart b/pkg/front_end/lib/src/fasta/builder/function_builder.dart
@@ -486,16 +486,18 @@ abstract class FunctionBuilderImpl extends MemberBuilderImpl
           isClassMember || isExtensionMember
               ? parent as DeclarationBuilder
               : null;
-      MetadataBuilder.buildAnnotations(
-          member, metadata, library, classOrExtensionBuilder, this, fileUri);
+      Scope parentScope = classOrExtensionBuilder?.scope ?? library.scope;
+      MetadataBuilder.buildAnnotations(member, metadata, library,
+          classOrExtensionBuilder, this, fileUri, parentScope);
       if (typeVariables != null) {
         for (int i = 0; i < typeVariables!.length; i++) {
           typeVariables![i].buildOutlineExpressions(
               library,
               classOrExtensionBuilder,
               this,
               coreTypes,
-              delayedActionPerformers);
+              delayedActionPerformers,
+              computeTypeParameterScope(parentScope));
         }
       }
 

diff --git a/pkg/front_end/lib/src/fasta/builder/metadata_builder.dart b/pkg/front_end/lib/src/fasta/builder/metadata_builder.dart
@@ -30,14 +30,8 @@ class MetadataBuilder {
       SourceLibraryBuilder library,
       DeclarationBuilder? classOrExtensionBuilder,
       MemberBuilder? member,
-      Uri fileUri) {
+      Uri fileUri, Scope scope) {
     if (metadata == null) return;
-    Scope scope = parent is Library ||
-            parent is Class ||
-            parent is Extension ||
-            classOrExtensionBuilder == null
-        ? library.scope
-        : classOrExtensionBuilder.scope;
     BodyBuilder bodyBuilder = library.loader
         .createBodyBuilderForOutlineExpression(library, classOrExtensionBuilder,
             member ?? classOrExtensionBuilder ?? library, scope, fileUri);

diff --git a/pkg/front_end/lib/src/fasta/builder/type_variable_builder.dart b/pkg/front_end/lib/src/fasta/builder/type_variable_builder.dart
@@ -13,6 +13,7 @@ import '../fasta_codes.dart'
         templateInternalProblemUnfinishedTypeVariable,
         templateTypeArgumentsOnTypeVariable;
 
+import '../scope.dart';
 import '../source/source_library_builder.dart';
 import '../util/helpers.dart';
 
@@ -200,9 +201,9 @@ class TypeVariableBuilder extends TypeDeclarationBuilderImpl {
       DeclarationBuilder? classOrExtensionBuilder,
       MemberBuilder? memberBuilder,
       CoreTypes coreTypes,
-      List<DelayedActionPerformer> delayedActionPerformers) {
+      List<DelayedActionPerformer> delayedActionPerformers, Scope scope) {
     MetadataBuilder.buildAnnotations(parameter, metadata, libraryBuilder,
-        classOrExtensionBuilder, memberBuilder, fileUri!);
+        classOrExtensionBuilder, memberBuilder, fileUri!, scope);
   }
 
   @override

diff --git a/pkg/front_end/lib/src/fasta/source/source_extension_builder.dart b/pkg/front_end/lib/src/fasta/source/source_extension_builder.dart
@@ -278,11 +278,11 @@ class SourceExtensionBuilder extends ExtensionBuilderImpl {
       List<DelayedActionPerformer> delayedActionPerformers,
       List<SynthesizedFunctionNode> synthesizedFunctionNodes) {
     MetadataBuilder.buildAnnotations(isPatch ? origin.extension : extension,
-        metadata, library, this, null, fileUri);
+        metadata, library, this, null, fileUri, library.scope);
     if (typeParameters != null) {
       for (int i = 0; i < typeParameters!.length; i++) {
-        typeParameters![i].buildOutlineExpressions(
-            library, this, null, coreTypes, delayedActionPerformers);
+        typeParameters![i].buildOutlineExpressions(library, this, null,
+            coreTypes, delayedActionPerformers, scope.parent!);
       }
     }
 

diff --git a/pkg/front_end/lib/src/fasta/source/source_library_builder.dart b/pkg/front_end/lib/src/fasta/source/source_library_builder.dart
@@ -2648,7 +2648,7 @@ class SourceLibraryBuilder extends LibraryBuilderImpl {
   @override
   void buildOutlineExpressions() {
     MetadataBuilder.buildAnnotations(
-        library, metadata, this, null, null, fileUri);
+        library, metadata, this, null, null, fileUri, scope);
   }
 
   /// Builds the core AST structures for [declaration] needed for the outline.