Add the first Kernel check for invalid JS interop

Add a visitor that checks for invalid JS interop usage and reports
diagnostics. Wire the visitor up to the `DevCompilerTarget` and `Dart2jsTarget`.

- Add a message without an analyzer code for this error. In the long term we may
  want to also add it to analyzer.
- Add a new package `_js_interop_checks` to share the kernel visitor between
  dart2js and ddc. Some of the code is copied from ddc, and in the long term we
  can centralize more of the detection of JS interop annotations to this
  package.
- Implement the first check to detect definitions of `operator []` or
  `operator []=` which are not allowed in JS interop classes.

Change-Id: I095a4b7f4732796dbc3cae55b32d5fc9bcdbd798
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/130733
Commit-Queue: Nate Bosch <nbosch@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>

.packages

@@ -7,6 +7,7 @@
 # Please update this file if you add a package to DEPS or /pkg
 #
 _fe_analyzer_shared:pkg/_fe_analyzer_shared/lib
+_js_interop_checks:pkg/_js_interop_checks/lib
 analysis_server:pkg/analysis_server/lib
 analysis_server_client:pkg/analysis_server_client/lib
 analysis_tool:pkg/analysis_tool/lib

CHANGELOG.md

@@ -36,6 +36,12 @@ additional details see the [announcement].
   `package:js` interop specification must now be wrapped with a call to
   `allowInterop`. This behavior was always enforced by `dart2js`, but was not
   enforced consistently in `ddc`. It will now be enforced in both.
+* JS interop classes with an index operator are now static errors.
+
+#### Dart2JS
+
+* JS interop classes with an index operator are now static errors instead of
+  causing invalid code in dart2js.
 
  [announcement]: https://github.com/dart-lang/sdk/issues/38994
 

pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart

@@ -5229,6 +5229,17 @@ Message _withArgumentsInvokeNonFunction(String name) {
       arguments: {'name': name});
 }
 
+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+const Code<Null> codeJsInteropIndexNotSupported =
+    messageJsInteropIndexNotSupported;
+
+// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
+const MessageCode messageJsInteropIndexNotSupported = const MessageCode(
+    "JsInteropIndexNotSupported",
+    message:
+        r"""JS interop classes do not support [] and []= operator methods.""",
+    tip: r"""Try replacing with a normal method.""");
+
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const Template<
     Message Function(String name)> templateLabelNotFound = const Template<

pkg/_js_interop_checks/README.md

@@ -0,0 +1 @@
+Shared kernel visitors checking for incorrect usage of `@JS()` style interop.

pkg/_js_interop_checks/lib/js_interop_checks.dart

@@ -0,0 +1,34 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:kernel/kernel.dart';
+import 'package:kernel/target/targets.dart';
+import 'package:_fe_analyzer_shared/src/messages/codes.dart'
+    show Message, LocatedMessage, messageJsInteropIndexNotSupported;
+
+import 'src/js_interop.dart';
+
+class JsInteropChecks extends RecursiveVisitor<void> {
+  final DiagnosticReporter<Message, LocatedMessage> _diagnosticsReporter;
+
+  JsInteropChecks(this._diagnosticsReporter);
+
+  @override
+  void visitClass(Class c) {
+    if (!hasJSInteropAnnotation(c)) return;
+    super.visitClass(c);
+  }
+
+  @override
+  void visitProcedure(Procedure procedure) {
+    if (procedure.isStatic) return;
+    if (procedure.name.name == '[]=' || procedure.name.name == '[]') {
+      _diagnosticsReporter.report(
+          messageJsInteropIndexNotSupported,
+          procedure.fileOffset,
+          procedure.name.name.length,
+          procedure.location.file);
+    }
+  }
+}

pkg/_js_interop_checks/lib/src/js_interop.dart

@@ -0,0 +1,41 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:kernel/kernel.dart';
+
+/// Returns true iff the class has an `@JS(...)` annotation from `package:js`.
+bool hasJSInteropAnnotation(Class c) =>
+    c.annotations.any(_isPublicJSAnnotation);
+
+final _packageJs = Uri.parse('package:js/js.dart');
+
+/// Returns [true] if [e] is the `JS` annotation from `package:js`.
+bool _isPublicJSAnnotation(Expression value) {
+  var c = _annotationClass(value);
+  return c != null &&
+      c.name == 'JS' &&
+      c.enclosingLibrary.importUri == _packageJs;
+}
+
+/// Returns the class of the instance referred to by metadata annotation [node].
+///
+/// For example:
+///
+/// - `@JS()` would return the "JS" class in "package:js".
+/// - `@anonymous` would return the "_Anonymous" class in "package:js".
+///
+/// This function works regardless of whether the CFE is evaluating constants,
+/// or whether the constant is a field reference (such as "anonymous" above).
+Class _annotationClass(Expression node) {
+  if (node is ConstantExpression) {
+    var constant = node.constant;
+    if (constant is InstanceConstant) return constant.classNode;
+  } else if (node is ConstructorInvocation) {
+    return node.target.enclosingClass;
+  } else if (node is StaticGet) {
+    var type = node.target.getterType;
+    if (type is InterfaceType) return type.classNode;
+  }
+  return null;
+}

pkg/_js_interop_checks/pubspec.yaml

@@ -0,0 +1,5 @@
+name: _js_interop_checks
+publish_to: none
+
+environment:
+  sdk: '>=2.7.0 <3.0.0'

pkg/compiler/lib/src/kernel/dart2js_target.dart

@@ -6,11 +6,15 @@
 // on the dart2js internals.
 library compiler.src.kernel.dart2js_target;
 
+import 'package:_fe_analyzer_shared/src/messages/codes.dart'
+    show Message, LocatedMessage;
+import 'package:_js_interop_checks/js_interop_checks.dart';
 import 'package:kernel/ast.dart' as ir;
-import 'package:kernel/core_types.dart';
 import 'package:kernel/class_hierarchy.dart';
+import 'package:kernel/core_types.dart';
 import 'package:kernel/reference_from_index.dart';
 import 'package:kernel/target/targets.dart';
+
 import 'invocation_mirror_constants.dart';
 
 const Iterable<String> _allowedDartSchemePaths = const <String>[
@@ -88,7 +92,13 @@ class Dart2jsTarget extends Target {
       Map<String, String> environmentDefines,
       DiagnosticReporter diagnosticReporter,
       ReferenceFromIndex referenceFromIndex,
-      {void logger(String msg)}) {}
+      {void logger(String msg)}) {
+    for (var library in libraries) {
+      JsInteropChecks(
+              diagnosticReporter as DiagnosticReporter<Message, LocatedMessage>)
+          .visitLibrary(library);
+    }
+  }
 
   @override
   ir.Expression instantiateInvocation(

pkg/dev_compiler/lib/src/kernel/target.dart

@@ -5,12 +5,15 @@
 import 'dart:collection';
 import 'dart:core' hide MapEntry;
 
+import 'package:_fe_analyzer_shared/src/messages/codes.dart'
+    show Message, LocatedMessage;
 import 'package:kernel/class_hierarchy.dart';
 import 'package:kernel/core_types.dart';
 import 'package:kernel/kernel.dart';
 import 'package:kernel/reference_from_index.dart';
 import 'package:kernel/target/targets.dart';
 import 'package:kernel/transformations/track_widget_constructor_locations.dart';
+import 'package:_js_interop_checks/js_interop_checks.dart';
 
 import 'constants.dart' show DevCompilerConstantsBackend;
 import 'kernel_helpers.dart';
@@ -144,6 +147,9 @@ class DevCompilerTarget extends Target {
       {void logger(String msg)}) {
     for (var library in libraries) {
       _CovarianceTransformer(library).transform();
+      JsInteropChecks(
+              diagnosticReporter as DiagnosticReporter<Message, LocatedMessage>)
+          .visitLibrary(library);
     }
   }
 

pkg/front_end/messages.status

@@ -387,6 +387,8 @@ InvalidVoid/part_wrapped_script1: Fail
 InvalidVoid/part_wrapped_script2: Fail
 InvalidVoid/script1: Fail
 InvalidVoid/script2: Fail
+JsInteropIndexNotSupported/analyzerCode: Fail # Web compiler specific
+JsInteropIndexNotSupported/example: Fail # Web compiler specific
 LanguageVersionInvalidInDotPackages/analyzerCode: Fail
 LanguageVersionInvalidInDotPackages/part_wrapped_script: Fail # Importing file in the (now) part.
 LanguageVersionMismatchInPart/analyzerCode: Fail

pkg/front_end/messages.yaml

@@ -3850,3 +3850,7 @@ NullableMixinError:
 NullableMixinWarning:
   template: "Mixing in '#name' marked with '?'."
   severity: WARNING
+
+JsInteropIndexNotSupported:
+  template: "JS interop classes do not support [] and []= operator methods."
+  tip: "Try replacing with a normal method."

pkg/front_end/test/spell_checking_list_messages.txt

@@ -30,8 +30,10 @@ field's
 flutter_runner
 futureor
 h
+interop
 libraries.json
 loadlibrary
+JS
 name.#name
 name.stack
 native('native