languages/dart-sdk
languages/dart-sdk
1
0
Fork 0
mirror of https://github.com/dart-lang/sdk synced 2024-09-16 03:36:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

[package:js] Add checks for external extension members.

Browse source 
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 <rileyporter@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Srujan Gaddam <srujzs@google.com>
This commit is contained in:
Riley Porter 2021-07-28 00:41:43 +00:00 committed by commit-bot@chromium.org
parent 746b8f1f5c
commit 16ac19d097
9 changed files with 465 additions and 41 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart
View file

@ -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<Null> 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<Null> codeJsInteropExternalMemberNotJSAnnotated =
messageJsInteropExternalMemberNotJSAnnotated;

128
pkg/_js_interop_checks/lib/js_interop_checks.dart
View file

@ -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<Reference, Extension>? _libraryExtensionsIndex;
/// Libraries that use `external` to exclude from checks on external.
static final Iterable<String> _pathsWithAllowedDartExternalUsage = <String>[
@ -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);
}
}

2
pkg/front_end/messages.status
View file

@ -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

4
pkg/front_end/messages.yaml
View file

@ -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."

57
tests/lib/js/external_nonjs_static_test.dart
View file

@ -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<T> on GenericNonJSClass<T> {
external T method();
// ^
// [web] JS interop class required for 'external' extension members.
}
class GenericNonJSClass<T> {}
main() {}

143
tests/lib/js/external_static_test.dart
View file

@ -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<T> on GenericNonJSClass<T> {
external T method();
// ^
// [web] JS interop class required for 'external' extension members.
}
class GenericNonJSClass<T> {}
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<T> on GenericJSClass<T> {
external T method();
}
@JS()
class GenericJSClass<T> {}
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() {}

9
tests/lib/js/parameters_static_test.dart
View file

@ -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() {}

41
tests/lib_2/js/external_nonjs_static_test.dart
View file

@ -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<T> on GenericNonJSClass<T> {
external T method();
// ^
// [web] JS interop class required for 'external' extension members.
}
class GenericNonJSClass<T> {}
main() {}

110
tests/lib_2/js/external_static_test.dart
View file

@ -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<T> on GenericNonJSClass<T> {
external T method();
// ^
// [web] JS interop class required for 'external' extension members.
}
class GenericNonJSClass<T> {}
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<T> on GenericJSClass<T> {
external T method();
}
@JS()
class GenericJSClass<T> {}
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() {}