From 16ac19d097260d0152f7e5d5d659e55b30c74cf7 Mon Sep 17 00:00:00 2001 From: Riley Porter Date: Wed, 28 Jul 2021 00:41:43 +0000 Subject: [PATCH] [package:js] Add checks for external extension members. Reports an error if there are external members in an extension, where the extension on type is not a JS interop class. A follow up change will allow extensions with external members on some Native classes, like dart:html classes. Change-Id: I01c645aa46e46071aae5f380c5e36329f61d3e18 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/207722 Commit-Queue: Riley Porter Reviewed-by: Sigmund Cherem Reviewed-by: Srujan Gaddam --- .../lib/src/messages/codes_generated.dart | 12 ++ .../lib/js_interop_checks.dart | 128 +++++++++++----- pkg/front_end/messages.status | 2 + pkg/front_end/messages.yaml | 4 + tests/lib/js/external_nonjs_static_test.dart | 57 +++++++ tests/lib/js/external_static_test.dart | 143 ++++++++++++++++++ tests/lib/js/parameters_static_test.dart | 9 ++ .../lib_2/js/external_nonjs_static_test.dart | 41 +++++ tests/lib_2/js/external_static_test.dart | 110 ++++++++++++++ 9 files changed, 465 insertions(+), 41 deletions(-) diff --git a/pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart b/pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart index 69bbfeb894f..5be60d47997 100644 --- a/pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart +++ b/pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart @@ -6217,6 +6217,18 @@ const MessageCode messageJsInteropEnclosingClassJSAnnotationContext = severity: Severity.context, message: r"""This is the enclosing class."""); +// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE. +const Code codeJsInteropExternalExtensionMemberNotOnJSClass = + messageJsInteropExternalExtensionMemberNotOnJSClass; + +// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE. +const MessageCode messageJsInteropExternalExtensionMemberNotOnJSClass = + const MessageCode("JsInteropExternalExtensionMemberNotOnJSClass", + message: + r"""JS interop class required for 'external' extension members.""", + tip: + r"""Try adding a JS interop annotation to the on type class of the extension."""); + // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE. const Code codeJsInteropExternalMemberNotJSAnnotated = messageJsInteropExternalMemberNotJSAnnotated; diff --git a/pkg/_js_interop_checks/lib/js_interop_checks.dart b/pkg/_js_interop_checks/lib/js_interop_checks.dart index e4d46916f3e..756793097bc 100644 --- a/pkg/_js_interop_checks/lib/js_interop_checks.dart +++ b/pkg/_js_interop_checks/lib/js_interop_checks.dart @@ -12,6 +12,7 @@ import 'package:_fe_analyzer_shared/src/messages/codes.dart' messageJsInteropAnonymousFactoryPositionalParameters, messageJsInteropEnclosingClassJSAnnotation, messageJsInteropEnclosingClassJSAnnotationContext, + messageJsInteropExternalExtensionMemberNotOnJSClass, messageJsInteropExternalMemberNotJSAnnotated, messageJsInteropIndexNotSupported, messageJsInteropNamedParameters, @@ -30,6 +31,7 @@ class JsInteropChecks extends RecursiveVisitor { bool _classHasJSAnnotation = false; bool _classHasAnonymousAnnotation = false; bool _libraryHasJSAnnotation = false; + Map? _libraryExtensionsIndex; /// Libraries that use `external` to exclude from checks on external. static final Iterable _pathsWithAllowedDartExternalUsage = [ @@ -86,7 +88,8 @@ class JsInteropChecks extends RecursiveVisitor { @override void defaultMember(Member member) { - _checkJSInteropAnnotation(member); + _checkInstanceMemberJSAnnotation(member); + if (!_isJSInteropMember(member)) _checkDisallowedExternal(member); // TODO(43530): Disallow having JS interop annotations on non-external // members (class members or otherwise). Currently, they're being ignored. super.defaultMember(member); @@ -165,11 +168,12 @@ class JsInteropChecks extends RecursiveVisitor { super.visitLibrary(lib); _libraryIsGlobalNamespace = false; _libraryHasJSAnnotation = false; + _libraryExtensionsIndex = null; } @override void visitProcedure(Procedure procedure) { - _checkJSInteropAnnotation(procedure); + _checkInstanceMemberJSAnnotation(procedure); if (_classHasJSAnnotation && !procedure.isExternal) { // If not one of few exceptions, member is not allowed to exclude // `external` inside of a JS interop class. @@ -183,38 +187,45 @@ class JsInteropChecks extends RecursiveVisitor { procedure.fileUri); } } - if (!_isJSInteropMember(procedure)) return; - if (!procedure.isStatic && - (procedure.name.text == '[]=' || procedure.name.text == '[]')) { - _diagnosticsReporter.report(messageJsInteropIndexNotSupported, - procedure.fileOffset, procedure.name.text.length, procedure.fileUri); - } - - var isAnonymousFactory = - _classHasAnonymousAnnotation && procedure.isFactory; - - if (isAnonymousFactory) { - // ignore: unnecessary_null_comparison - if (procedure.function != null && - !procedure.function.positionalParameters.isEmpty) { - var firstPositionalParam = procedure.function.positionalParameters[0]; - _diagnosticsReporter.report( - messageJsInteropAnonymousFactoryPositionalParameters, - firstPositionalParam.fileOffset, - firstPositionalParam.name!.length, - firstPositionalParam.location!.file); - } + if (!_isJSInteropMember(procedure)) { + _checkDisallowedExternal(procedure); } else { - // Only factory constructors for anonymous classes are allowed to have - // named parameters. - _checkNoNamedParameters(procedure.function); + // Check JS interop indexing. + if (!procedure.isStatic && + (procedure.name.text == '[]=' || procedure.name.text == '[]')) { + _diagnosticsReporter.report( + messageJsInteropIndexNotSupported, + procedure.fileOffset, + procedure.name.text.length, + procedure.fileUri); + } + + // Check JS Interop positional and named parameters. + var isAnonymousFactory = + _classHasAnonymousAnnotation && procedure.isFactory; + if (isAnonymousFactory) { + // ignore: unnecessary_null_comparison + if (procedure.function != null && + !procedure.function.positionalParameters.isEmpty) { + var firstPositionalParam = procedure.function.positionalParameters[0]; + _diagnosticsReporter.report( + messageJsInteropAnonymousFactoryPositionalParameters, + firstPositionalParam.fileOffset, + firstPositionalParam.name!.length, + firstPositionalParam.location!.file); + } + } else { + // Only factory constructors for anonymous classes are allowed to have + // named parameters. + _checkNoNamedParameters(procedure.function); + } } } @override void visitConstructor(Constructor constructor) { - _checkJSInteropAnnotation(constructor); + _checkInstanceMemberJSAnnotation(constructor); if (_classHasJSAnnotation && !constructor.isExternal && !constructor.isSynthetic) { @@ -225,9 +236,12 @@ class JsInteropChecks extends RecursiveVisitor { constructor.name.text.length, constructor.fileUri); } - if (!_isJSInteropMember(constructor)) return; - _checkNoNamedParameters(constructor.function); + if (!_isJSInteropMember(constructor)) { + _checkDisallowedExternal(constructor); + } else { + _checkNoNamedParameters(constructor.function); + } } /// Reports an error if [functionNode] has named parameters. @@ -243,9 +257,9 @@ class JsInteropChecks extends RecursiveVisitor { } } - /// Reports an error if [member] does not correctly use the JS interop - /// annotation or the keyword `external`. - void _checkJSInteropAnnotation(Member member) { + /// Reports an error if given instance [member] is JS interop, but inside a + /// non JS interop class. + void _checkInstanceMemberJSAnnotation(Member member) { var enclosingClass = member.enclosingClass; if (!_classHasJSAnnotation && @@ -262,13 +276,24 @@ class JsInteropChecks extends RecursiveVisitor { enclosingClass.name.length) ]); } + } - // Check for correct `external` usage. - if (member.isExternal && - !_isAllowedExternalUsage(member) && - !hasJSInteropAnnotation(member)) { - if (member.enclosingClass != null && !_classHasJSAnnotation || - member.enclosingClass == null && !_libraryHasJSAnnotation) { + /// Assumes given [member] is not JS interop, and reports an error if + /// [member] is `external` and not an allowed `external` usage. + void _checkDisallowedExternal(Member member) { + if (member.isExternal) { + // TODO(rileyporter): Allow extension members on some Native classes. + if (member.isExtensionMember) { + _diagnosticsReporter.report( + messageJsInteropExternalExtensionMemberNotOnJSClass, + member.fileOffset, + member.name.text.length, + member.fileUri); + } else if (!hasJSInteropAnnotation(member) && + !_isAllowedExternalUsage(member)) { + // Member could be JS annotated and not considered a JS interop member + // if inside a non-JS interop class. Should not report an error in this + // case, since a different error will already be produced. _diagnosticsReporter.report( messageJsInteropExternalMemberNotJSAnnotated, member.fileOffset, @@ -289,13 +314,19 @@ class JsInteropChecks extends RecursiveVisitor { } /// Returns whether [member] is considered to be a JS interop member. + /// + /// A JS interop member is `external`, and is in a valid JS interop context, + /// which can be: + /// - inside a JS interop class + /// - inside an extension on a JS interop class + /// - a top level member that is JS interop annotated or in a JS interop + /// library + /// If a member belongs to a class, the class must be JS interop annotated. bool _isJSInteropMember(Member member) { if (member.isExternal) { if (_classHasJSAnnotation) return true; + if (member.isExtensionMember) return _isJSExtensionMember(member); if (member.enclosingClass == null) { - // In the case where the member does not belong to any class, a JS - // annotation is not needed on the library to be considered JS interop - // as long as the member has an annotation. return hasJSInteropAnnotation(member) || _libraryHasJSAnnotation; } } @@ -303,4 +334,19 @@ class JsInteropChecks extends RecursiveVisitor { // Otherwise, not JS interop. return false; } + + /// Returns whether given extension [member] is in an extension that is on a + /// JS interop class. + bool _isJSExtensionMember(Member member) { + assert(member.isExtensionMember); + if (_libraryExtensionsIndex == null) { + _libraryExtensionsIndex = {}; + member.enclosingLibrary.extensions.forEach((extension) => + extension.members.forEach((memberDescriptor) => + _libraryExtensionsIndex![memberDescriptor.member] = extension)); + } + + var onType = _libraryExtensionsIndex![member.reference]!.onType; + return onType is InterfaceType && hasJSInteropAnnotation(onType.classNode); + } } diff --git a/pkg/front_end/messages.status b/pkg/front_end/messages.status index 76faff7051a..b201a4cd2c9 100644 --- a/pkg/front_end/messages.status +++ b/pkg/front_end/messages.status @@ -510,6 +510,8 @@ JsInteropDartClassExtendsJSClass/analyzerCode: Fail # Web compiler specific JsInteropDartClassExtendsJSClass/example: Fail # Web compiler specific JsInteropEnclosingClassJSAnnotation/analyzerCode: Fail # Web compiler specific JsInteropEnclosingClassJSAnnotation/example: Fail # Web compiler specific +JsInteropExternalExtensionMemberNotOnJSClass/analyzerCode: Fail # Web compiler specific +JsInteropExternalExtensionMemberNotOnJSClass/example: Fail # Web compiler specific JsInteropExternalMemberNotJSAnnotated/analyzerCode: Fail # Web compiler specific JsInteropExternalMemberNotJSAnnotated/example: Fail # Web compiler specific JsInteropIndexNotSupported/analyzerCode: Fail # Web compiler specific diff --git a/pkg/front_end/messages.yaml b/pkg/front_end/messages.yaml index 58334a0d5a2..f0cb7198e22 100644 --- a/pkg/front_end/messages.yaml +++ b/pkg/front_end/messages.yaml @@ -4908,6 +4908,10 @@ JsInteropEnclosingClassJSAnnotationContext: template: "This is the enclosing class." severity: CONTEXT +JsInteropExternalExtensionMemberNotOnJSClass: + template: "JS interop class required for 'external' extension members." + tip: "Try adding a JS interop annotation to the on type class of the extension." + JsInteropExternalMemberNotJSAnnotated: template: "Only JS interop members may be 'external'." tip: "Try removing the 'external' keyword or adding a JS interop annotation." diff --git a/tests/lib/js/external_nonjs_static_test.dart b/tests/lib/js/external_nonjs_static_test.dart index 47b62524e68..913d20278f0 100644 --- a/tests/lib/js/external_nonjs_static_test.dart +++ b/tests/lib/js/external_nonjs_static_test.dart @@ -98,4 +98,61 @@ class AnonymousClass { // [web] Only JS interop members may be 'external'. } +extension ExtensionNonJS on NonJSClass { + external var field; + // ^ + // [web] JS interop class required for 'external' extension members. + external final finalField; + // ^ + // [web] JS interop class required for 'external' extension members. + external static var staticField; + // ^ + // [web] JS interop class required for 'external' extension members. + external static final staticFinalField; + // ^ + // [web] JS interop class required for 'external' extension members. + + external get getter; + // ^ + // [web] JS interop class required for 'external' extension members. + external set setter(_); + // ^ + // [web] JS interop class required for 'external' extension members. + + external static get staticGetter; + // ^ + // [web] JS interop class required for 'external' extension members. + external static set staticSetter(_); + // ^ + // [web] JS interop class required for 'external' extension members. + + external method(); + // ^ + // [web] JS interop class required for 'external' extension members. + external static staticMethod(); + // ^ + // [web] JS interop class required for 'external' extension members. + external optionalParameterMethod([int? a, int b = 0]); + // ^ + // [web] JS interop class required for 'external' extension members. + external overridenMethod(); + // ^ + // [web] JS interop class required for 'external' extension members. + + nonExternalMethod() => 1; + static nonExternalStaticMethod() => 2; +} + +class NonJSClass { + void overridenMethod() => 5; +} + +extension ExtensionGenericNonJS on GenericNonJSClass { + external T method(); + // ^ + // [web] JS interop class required for 'external' extension members. +} + +class GenericNonJSClass {} + main() {} diff --git a/tests/lib/js/external_static_test.dart b/tests/lib/js/external_static_test.dart index 8fac26dba92..0a1c8dbfc07 100644 --- a/tests/lib/js/external_static_test.dart +++ b/tests/lib/js/external_static_test.dart @@ -86,4 +86,147 @@ class AnonymousClass { // [web] Only JS interop members may be 'external'. } +extension ExtensionNonJS on NonJSClass { + external var field; + // ^ + // [web] JS interop class required for 'external' extension members. + external final finalField; + // ^ + // [web] JS interop class required for 'external' extension members. + external static var staticField; + // ^ + // [web] JS interop class required for 'external' extension members. + external static final staticFinalField; + // ^ + // [web] JS interop class required for 'external' extension members. + + external get getter; + // ^ + // [web] JS interop class required for 'external' extension members. + external set setter(_); + // ^ + // [web] JS interop class required for 'external' extension members. + + external static get staticGetter; + // ^ + // [web] JS interop class required for 'external' extension members. + external static set staticSetter(_); + // ^ + // [web] JS interop class required for 'external' extension members. + + external method(); + // ^ + // [web] JS interop class required for 'external' extension members. + external static staticMethod(); + // ^ + // [web] JS interop class required for 'external' extension members. + external optionalParameterMethod([int? a, int b = 0]); + // ^ + // [web] JS interop class required for 'external' extension members. + external overridenMethod(); + // ^ + // [web] JS interop class required for 'external' extension members. + + @JS('fieldAnnotation') + external var annotatedField; + // ^ + // [web] JS interop class required for 'external' extension members. + @JS('memberAnnotation') + external annotatedMethod(); + // ^ + // [web] JS interop class required for 'external' extension members. + + nonExternalMethod() => 1; + static nonExternalStaticMethod() => 2; +} + +class NonJSClass { + void overridenMethod() => 5; +} + +extension ExtensionGenericNonJS on GenericNonJSClass { + external T method(); + // ^ + // [web] JS interop class required for 'external' extension members. +} + +class GenericNonJSClass {} + +extension ExtensionJS on JSClass { + external var field; + external final finalField; + external static var staticField; + external static final staticFinalField; + + external get getter; + external set setter(_); + + external static get staticGetter; + external static set staticSetter(_); + + external method(); + external static staticMethod(); + external optionalParameterMethod([int? a, int b = 0]); + + @JS('fieldAnnotation') + external var annotatedField; + + @JS('memberAnnotation') + external annotatedMethod(); + + nonExternalMethod() => 1; + static nonExternalStaticMethod() => 2; +} + +@JS() +class JSClass {} + +extension ExtensionGenericJS on GenericJSClass { + external T method(); +} + +@JS() +class GenericJSClass {} + +extension ExtensionAnonymousJS on AnonymousJSClass { + external var field; + external get getter; + external set setter(_); + external method(); +} + +@JS() +@anonymous +class AnonymousJSClass {} + +extension ExtensionAbstractJS on AbstractJSClass { + external var field; + external get getter; + external set setter(_); + external method(); +} + +@JS() +abstract class AbstractJSClass {} + +extension ExtensionAnnotatedJS on AnnotatedJSClass { + external var field; + external get getter; + external set setter(_); + external method(); +} + +@JS('Annotation') +class AnnotatedJSClass {} + +extension ExtensionPrivateJS on _privateJSClass { + external var field; + external get getter; + external set setter(_); + external method(); +} + +@JS() +class _privateJSClass {} + main() {} diff --git a/tests/lib/js/parameters_static_test.dart b/tests/lib/js/parameters_static_test.dart index 7fa7ddca0e9..b6c68fe72ce 100644 --- a/tests/lib/js/parameters_static_test.dart +++ b/tests/lib/js/parameters_static_test.dart @@ -58,4 +58,13 @@ abstract class Qux { // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. } +extension ExtensionFoo on Foo { + external int singleNamedArg({int? a}); + // ^ + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. + external int mixedNamedArgs(int a, {int? b}); + // ^ + // [web] Named parameters for JS interop functions are only allowed in a factory constructor of an @anonymous JS class. +} + main() {} diff --git a/tests/lib_2/js/external_nonjs_static_test.dart b/tests/lib_2/js/external_nonjs_static_test.dart index 6635d27fe10..1de7e61e2ba 100644 --- a/tests/lib_2/js/external_nonjs_static_test.dart +++ b/tests/lib_2/js/external_nonjs_static_test.dart @@ -72,4 +72,45 @@ class AnonymousClass { // [web] Only JS interop members may be 'external'. } +extension ExtensionNonJS on NonJSClass { + external get getter; + // ^ + // [web] JS interop class required for 'external' extension members. + external set setter(_); + // ^ + // [web] JS interop class required for 'external' extension members. + + external static get staticGetter; + // ^ + // [web] JS interop class required for 'external' extension members. + external static set staticSetter(_); + // ^ + // [web] JS interop class required for 'external' extension members. + + external method(); + // ^ + // [web] JS interop class required for 'external' extension members. + external static staticMethod(); + // ^ + // [web] JS interop class required for 'external' extension members. + external overridenMethod(); + // ^ + // [web] JS interop class required for 'external' extension members. + + nonExternalMethod() => 1; + static nonExternalStaticMethod() => 2; +} + +class NonJSClass { + void overridenMethod() => 5; +} + +extension ExtensionGenericNonJS on GenericNonJSClass { + external T method(); + // ^ + // [web] JS interop class required for 'external' extension members. +} + +class GenericNonJSClass {} + main() {} diff --git a/tests/lib_2/js/external_static_test.dart b/tests/lib_2/js/external_static_test.dart index 8ac09be2881..3f4c60b73e8 100644 --- a/tests/lib_2/js/external_static_test.dart +++ b/tests/lib_2/js/external_static_test.dart @@ -66,4 +66,114 @@ class AnonymousClass { // [web] Only JS interop members may be 'external'. } +extension ExtensionNonJS on NonJSClass { + external get getter; + // ^ + // [web] JS interop class required for 'external' extension members. + external set setter(_); + // ^ + // [web] JS interop class required for 'external' extension members. + + external static get staticGetter; + // ^ + // [web] JS interop class required for 'external' extension members. + external static set staticSetter(_); + // ^ + // [web] JS interop class required for 'external' extension members. + + external method(); + // ^ + // [web] JS interop class required for 'external' extension members. + external static staticMethod(); + // ^ + // [web] JS interop class required for 'external' extension members. + external overridenMethod(); + // ^ + // [web] JS interop class required for 'external' extension members. + + @JS('memberAnnotation') + external annotatedMethod(); + // ^ + // [web] JS interop class required for 'external' extension members. + + nonExternalMethod() => 1; + static nonExternalStaticMethod() => 2; +} + +class NonJSClass { + void overridenMethod() => 5; +} + +extension ExtensionGenericNonJS on GenericNonJSClass { + external T method(); + // ^ + // [web] JS interop class required for 'external' extension members. +} + +class GenericNonJSClass {} + +extension ExtensionJS on JSClass { + external get getter; + external set setter(_); + + external static get staticGetter; + external static set staticSetter(_); + + external method(); + external static staticMethod(); + + @JS('memberAnnotation') + external annotatedMethod(); + + nonExternalMethod() => 1; + static nonExternalStaticMethod() => 2; +} + +@JS() +class JSClass {} + +extension ExtensionGenericJS on GenericJSClass { + external T method(); +} + +@JS() +class GenericJSClass {} + +extension ExtensionAnonymousJS on AnonymousJSClass { + external get getter; + external set setter(_); + external method(); +} + +@JS() +@anonymous +class AnonymousJSClass {} + +extension ExtensionAbstractJS on AbstractJSClass { + external get getter; + external set setter(_); + external method(); +} + +@JS() +abstract class AbstractJSClass {} + +extension ExtensionAnnotatedJS on AnnotatedJSClass { + external get getter; + external set setter(_); + external method(); +} + +@JS('Annotation') +class AnnotatedJSClass {} + +extension ExtensionPrivateJS on _privateJSClass { + external get getter; + external set setter(_); + external method(); +} + +@JS() +class _privateJSClass {} + main() {}