Skip to content

Commit 7685ec3

Browse files
srujzsCommit Queue
srujzs
authored and
Commit Queue
committed
[dart:js_interop] Better error reporting for invalid external types
Closes #54320 Several improvements to the error reporting: - Split errors to avoid parametrizing error strings. - Use one error per member/toJS invocation. - Highlight the invalid types in the signature. Extra tests are added to get coverage for things like operators. Change-Id: I6d8ac3cf0124730e7c2c0dab3a107da5d0263f7a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/347226 Reviewed-by: Sigmund Cherem <[email protected]> Commit-Queue: Srujan Gaddam <[email protected]>
1 parent f792cc1 commit 7685ec3

File tree

10 files changed

+443
-125
lines changed

10 files changed

+443
-125
lines changed

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

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10751,6 +10751,42 @@ const MessageCode messageJsInteropOperatorsNotSupported = const MessageCode(
1075110751
r"""Try making this class a static interop type instead.""",
1075210752
);
1075310753

10754+
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
10755+
const Template<Message Function(String string2)>
10756+
templateJsInteropStaticInteropExternalFunctionTypeViolation =
10757+
const Template<Message Function(String string2)>(
10758+
"JsInteropStaticInteropExternalFunctionTypeViolation",
10759+
problemMessageTemplate:
10760+
r"""External JS interop member contains invalid types in its function signature: '#string2'.""",
10761+
correctionMessageTemplate:
10762+
r"""Use one of these valid types instead: JS types from 'dart:js_interop', void, bool, num, double, int, String, extension types that erase to one of these types, '@staticInterop' types, 'dart:html' types when compiling to JS, or a type parameter that is a subtype of a valid non-primitive type.""",
10763+
withArguments:
10764+
_withArgumentsJsInteropStaticInteropExternalFunctionTypeViolation,
10765+
);
10766+
10767+
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
10768+
const Code<Message Function(String string2)>
10769+
codeJsInteropStaticInteropExternalFunctionTypeViolation =
10770+
const Code<Message Function(String string2)>(
10771+
"JsInteropStaticInteropExternalFunctionTypeViolation",
10772+
);
10773+
10774+
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
10775+
Message _withArgumentsJsInteropStaticInteropExternalFunctionTypeViolation(
10776+
String string2) {
10777+
if (string2.isEmpty) throw 'No string provided';
10778+
return new Message(
10779+
codeJsInteropStaticInteropExternalFunctionTypeViolation,
10780+
problemMessage:
10781+
"""External JS interop member contains invalid types in its function signature: '${string2}'.""",
10782+
correctionMessage:
10783+
"""Use one of these valid types instead: JS types from 'dart:js_interop', void, bool, num, double, int, String, extension types that erase to one of these types, '@staticInterop' types, 'dart:html' types when compiling to JS, or a type parameter that is a subtype of a valid non-primitive type.""",
10784+
arguments: {
10785+
'string2': string2,
10786+
},
10787+
);
10788+
}
10789+
1075410790
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
1075510791
const Code<Null> codeJsInteropStaticInteropGenerativeConstructor =
1075610792
messageJsInteropStaticInteropGenerativeConstructor;
@@ -10955,6 +10991,41 @@ Message _withArgumentsJsInteropStaticInteropTearOffsDisallowed(
1095510991
);
1095610992
}
1095710993

10994+
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
10995+
const Template<Message Function(String string2)>
10996+
templateJsInteropStaticInteropToJSFunctionTypeViolation =
10997+
const Template<Message Function(String string2)>(
10998+
"JsInteropStaticInteropToJSFunctionTypeViolation",
10999+
problemMessageTemplate:
11000+
r"""Function converted via 'toJS' contains invalid types in its function signature: '#string2'.""",
11001+
correctionMessageTemplate:
11002+
r"""Use one of these valid types instead: JS types from 'dart:js_interop', void, bool, num, double, int, String, extension types that erase to one of these types, '@staticInterop' types, 'dart:html' types when compiling to JS, or a type parameter that is a subtype of a valid non-primitive type.""",
11003+
withArguments: _withArgumentsJsInteropStaticInteropToJSFunctionTypeViolation,
11004+
);
11005+
11006+
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
11007+
const Code<Message Function(String string2)>
11008+
codeJsInteropStaticInteropToJSFunctionTypeViolation =
11009+
const Code<Message Function(String string2)>(
11010+
"JsInteropStaticInteropToJSFunctionTypeViolation",
11011+
);
11012+
11013+
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
11014+
Message _withArgumentsJsInteropStaticInteropToJSFunctionTypeViolation(
11015+
String string2) {
11016+
if (string2.isEmpty) throw 'No string provided';
11017+
return new Message(
11018+
codeJsInteropStaticInteropToJSFunctionTypeViolation,
11019+
problemMessage:
11020+
"""Function converted via 'toJS' contains invalid types in its function signature: '${string2}'.""",
11021+
correctionMessage:
11022+
"""Use one of these valid types instead: JS types from 'dart:js_interop', void, bool, num, double, int, String, extension types that erase to one of these types, '@staticInterop' types, 'dart:html' types when compiling to JS, or a type parameter that is a subtype of a valid non-primitive type.""",
11023+
arguments: {
11024+
'string2': string2,
11025+
},
11026+
);
11027+
}
11028+
1095811029
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
1095911030
const Template<Message Function(String name)>
1096011031
templateJsInteropStaticInteropTrustTypesUsageNotAllowed =

‎pkg/_js_interop_checks/lib/js_interop_checks.dart

Lines changed: 94 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,13 @@ import 'package:front_end/src/fasta/codes/fasta_codes.dart'
4747
show
4848
templateJsInteropExtensionTypeNotInterop,
4949
templateJsInteropFunctionToJSRequiresStaticType,
50-
templateJsInteropStaticInteropExternalTypeViolation;
50+
templateJsInteropStaticInteropExternalAccessorTypeViolation,
51+
templateJsInteropStaticInteropExternalFunctionTypeViolation,
52+
templateJsInteropStaticInteropToJSFunctionTypeViolation;
5153
import 'package:kernel/class_hierarchy.dart';
5254
import 'package:kernel/core_types.dart';
5355
import 'package:kernel/kernel.dart' hide Pattern;
56+
import 'package:kernel/src/printer.dart';
5457
import 'package:kernel/target/targets.dart';
5558
import 'package:kernel/type_environment.dart';
5659

@@ -67,6 +70,9 @@ class JsInteropChecks extends RecursiveVisitor {
6770
final Map<String, Class> _nativeClasses;
6871
final JsInteropDiagnosticReporter _reporter;
6972
final StatefulStaticTypeContext _staticTypeContext;
73+
final AstTextStrategy _textStrategy = const AstTextStrategy(
74+
showNullableOnly: true, useQualifiedTypeParameterNames: false);
75+
7076
bool _classHasJSAnnotation = false;
7177
bool _classHasAnonymousAnnotation = false;
7278
bool _classHasStaticInteropAnnotation = false;
@@ -398,14 +404,7 @@ class JsInteropChecks extends RecursiveVisitor {
398404
((hasDartJSInteropAnnotation(annotatable) ||
399405
annotatable is ExtensionTypeDeclaration))) {
400406
// Checks for dart:js_interop APIs only.
401-
final function = node.function;
402-
_reportProcedureIfNotAllowedType(function.returnType, node);
403-
for (final parameter in function.positionalParameters) {
404-
_reportProcedureIfNotAllowedType(parameter.type, node);
405-
}
406-
for (final parameter in function.namedParameters) {
407-
_reportProcedureIfNotAllowedType(parameter.type, node);
408-
}
407+
_reportExternalProcedureIfNotAllowedFunctionType(node);
409408
}
410409
}
411410
}
@@ -749,10 +748,7 @@ class JsInteropChecks extends RecursiveVisitor {
749748
if (functionType.typeParameters.isNotEmpty) {
750749
report(messageJsInteropFunctionToJSTypeParameters);
751750
}
752-
_reportStaticInvocationIfNotAllowedType(functionType.returnType, node);
753-
for (final parameter in functionType.positionalParameters) {
754-
_reportStaticInvocationIfNotAllowedType(parameter, node);
755-
}
751+
_reportFunctionToJSInvocationIfNotAllowedFunctionType(functionType, node);
756752
}
757753
}
758754

@@ -950,25 +946,95 @@ class JsInteropChecks extends RecursiveVisitor {
950946
return false;
951947
}
952948

953-
void _reportIfNotAllowedExternalType(
954-
DartType type, TreeNode node, Name name, Uri? fileUri) {
955-
if (!_isAllowedExternalType(type)) {
956-
_reporter.report(
957-
templateJsInteropStaticInteropExternalTypeViolation.withArguments(
958-
type, true),
959-
node.fileOffset,
960-
name.text.length,
961-
fileUri);
949+
bool _isAllowedExternalFunctionType(FunctionType type) =>
950+
_isAllowedExternalType(type.returnType) &&
951+
type.namedParameters.every((p) => _isAllowedExternalType(p.type)) &&
952+
type.positionalParameters.every((p) => _isAllowedExternalType(p));
953+
954+
String _disallowedExternalFunctionTypeString(FunctionType functionType) {
955+
String typeStringToErrorTypeString(String type) => '*$type*';
956+
String typeToString(DartType type) {
957+
final string = type.toText(_textStrategy);
958+
return _isAllowedExternalType(type)
959+
? string
960+
: typeStringToErrorTypeString(string);
962961
}
962+
963+
String namedTypeToString(NamedType type) {
964+
final string = type.toText(_textStrategy);
965+
return _isAllowedExternalType(type.type)
966+
? string
967+
: typeStringToErrorTypeString(string);
968+
}
969+
970+
final positionalParameterTypeString =
971+
functionType.positionalParameters.map(typeToString).join(', ');
972+
final namedParameterTypeString =
973+
functionType.namedParameters.map(namedTypeToString).join(', ');
974+
String parameterTypeString;
975+
if (positionalParameterTypeString.isNotEmpty &&
976+
namedParameterTypeString.isNotEmpty) {
977+
parameterTypeString =
978+
'$positionalParameterTypeString, {$namedParameterTypeString}';
979+
} else {
980+
parameterTypeString = namedParameterTypeString.isNotEmpty
981+
? '{$namedParameterTypeString}'
982+
: positionalParameterTypeString;
983+
}
984+
return '${typeToString(functionType.returnType)} '
985+
'Function($parameterTypeString)';
963986
}
964987

965-
void _reportProcedureIfNotAllowedType(DartType type, Procedure node) =>
966-
_reportIfNotAllowedExternalType(type, node, node.name, node.fileUri);
988+
void _reportExternalProcedureIfNotAllowedFunctionType(Procedure node) {
989+
FunctionType functionType;
990+
if (node.isExtensionMember || node.isExtensionTypeMember) {
991+
functionType = extensionIndex.getFunctionType(node)!;
992+
} else {
993+
functionType = node.signatureType ??
994+
node.function.computeFunctionType(Nullability.nonNullable);
995+
}
996+
final isGetter = extensionIndex.isGetter(node);
997+
final isSetter = extensionIndex.isSetter(node);
998+
if (isGetter || isSetter) {
999+
// There's only one type, so only report that one type instead of a
1000+
// function type. This also avoids duplication in reporting external
1001+
// fields, which are just a getter and a setter.
1002+
final accessorType = isGetter
1003+
? functionType.returnType
1004+
: functionType.positionalParameters[0];
1005+
if (!_isAllowedExternalType(accessorType)) {
1006+
_reporter.report(
1007+
templateJsInteropStaticInteropExternalAccessorTypeViolation
1008+
.withArguments(accessorType, true),
1009+
node.fileOffset,
1010+
node.name.text.length,
1011+
node.location?.file);
1012+
}
1013+
} else {
1014+
// Methods, operators, constructors, factories.
1015+
if (!_isAllowedExternalFunctionType(functionType)) {
1016+
_reporter.report(
1017+
templateJsInteropStaticInteropExternalFunctionTypeViolation
1018+
.withArguments(
1019+
_disallowedExternalFunctionTypeString(functionType)),
1020+
node.fileOffset,
1021+
node.name.text.length,
1022+
node.location?.file);
1023+
}
1024+
}
1025+
}
9671026

968-
void _reportStaticInvocationIfNotAllowedType(
969-
DartType type, StaticInvocation node) =>
970-
_reportIfNotAllowedExternalType(
971-
type, node, node.name, node.location?.file);
1027+
void _reportFunctionToJSInvocationIfNotAllowedFunctionType(
1028+
FunctionType functionType, StaticInvocation invocation) {
1029+
if (!_isAllowedExternalFunctionType(functionType)) {
1030+
_reporter.report(
1031+
templateJsInteropStaticInteropToJSFunctionTypeViolation.withArguments(
1032+
_disallowedExternalFunctionTypeString(functionType)),
1033+
invocation.fileOffset,
1034+
invocation.name.text.length,
1035+
invocation.location?.file);
1036+
}
1037+
}
9721038
}
9731039

9741040
class JsInteropDiagnosticReporter {

‎pkg/_js_interop_checks/lib/src/transformations/js_util_optimizer.dart

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -948,6 +948,32 @@ class ExtensionIndex {
948948
ExtensionMemberKind.Operator,
949949
ProcedureKind.Operator);
950950

951+
/// Given an interop extension type or extension member [node], gets the
952+
/// function type as written.
953+
///
954+
/// Extension type and extension instance members include the instance as the
955+
/// first positional parameter of the generated function. Since this was never
956+
/// written by the user, it is excluded in the resulting function type.
957+
///
958+
/// If not an interop extension type or extension member, returns null.
959+
FunctionType? getFunctionType(Procedure node) {
960+
if (getExtensionDescriptor(node) == null &&
961+
getExtensionTypeDescriptor(node) == null) return null;
962+
963+
final functionType = node.signatureType ??
964+
node.function.computeFunctionType(Nullability.nonNullable);
965+
var positionalParameters = functionType.positionalParameters;
966+
if (isInstanceInteropMember(node)) {
967+
// Ignore the instance parameter.
968+
positionalParameters = positionalParameters.skip(1).toList();
969+
}
970+
return FunctionType(positionalParameters, functionType.returnType,
971+
functionType.declaredNullability,
972+
namedParameters: functionType.namedParameters,
973+
typeParameters: functionType.typeParameters,
974+
requiredParameterCount: functionType.requiredParameterCount);
975+
}
976+
951977
/// Return whether [node] is an external static interop constructor/factory.
952978
///
953979
/// If [literal] is true, we check if [node] is an object literal constructor,

‎pkg/front_end/lib/src/fasta/codes/fasta_codes_cfe_generated.dart

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4716,36 +4716,38 @@ Message _withArgumentsJsInteropIsAPrimitiveExtensionType(
47164716

47174717
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
47184718
const Template<Message Function(DartType _type, bool isNonNullableByDefault)>
4719-
templateJsInteropStaticInteropExternalTypeViolation = const Template<
4719+
templateJsInteropStaticInteropExternalAccessorTypeViolation =
4720+
const Template<
47204721
Message Function(DartType _type, bool isNonNullableByDefault)>(
4721-
"JsInteropStaticInteropExternalTypeViolation",
4722+
"JsInteropStaticInteropExternalAccessorTypeViolation",
47224723
problemMessageTemplate:
4723-
r"""Type '#type' is not a valid type in the signature of 'dart:js_interop' external APIs or APIs converted via 'toJS'.""",
4724+
r"""External JS interop member contains an invalid type: '#type'.""",
47244725
correctionMessageTemplate:
4725-
r"""Use one of these valid types instead: JS types from 'dart:js_interop', '@staticInterop' types, 'dart:html' types when compiling to JS, void, bool, num, double, int, String, extension types that erases to one of these types, or a type parameter that is bound to a static interop or 'dart:html' type.""",
4726-
withArguments: _withArgumentsJsInteropStaticInteropExternalTypeViolation,
4726+
r"""Use one of these valid types instead: JS types from 'dart:js_interop', void, bool, num, double, int, String, extension types that erase to one of these types, '@staticInterop' types, 'dart:html' types when compiling to JS, or a type parameter that is a subtype of a valid non-primitive type.""",
4727+
withArguments:
4728+
_withArgumentsJsInteropStaticInteropExternalAccessorTypeViolation,
47274729
);
47284730

47294731
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
47304732
const Code<Message Function(DartType _type, bool isNonNullableByDefault)>
4731-
codeJsInteropStaticInteropExternalTypeViolation =
4733+
codeJsInteropStaticInteropExternalAccessorTypeViolation =
47324734
const Code<Message Function(DartType _type, bool isNonNullableByDefault)>(
4733-
"JsInteropStaticInteropExternalTypeViolation",
4735+
"JsInteropStaticInteropExternalAccessorTypeViolation",
47344736
);
47354737

47364738
// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
4737-
Message _withArgumentsJsInteropStaticInteropExternalTypeViolation(
4739+
Message _withArgumentsJsInteropStaticInteropExternalAccessorTypeViolation(
47384740
DartType _type, bool isNonNullableByDefault) {
47394741
TypeLabeler labeler = new TypeLabeler(isNonNullableByDefault);
47404742
List<Object> typeParts = labeler.labelType(_type);
47414743
String type = typeParts.join();
47424744
return new Message(
4743-
codeJsInteropStaticInteropExternalTypeViolation,
4745+
codeJsInteropStaticInteropExternalAccessorTypeViolation,
47444746
problemMessage:
4745-
"""Type '${type}' is not a valid type in the signature of 'dart:js_interop' external APIs or APIs converted via 'toJS'.""" +
4747+
"""External JS interop member contains an invalid type: '${type}'.""" +
47464748
labeler.originMessages,
47474749
correctionMessage:
4748-
"""Use one of these valid types instead: JS types from 'dart:js_interop', '@staticInterop' types, 'dart:html' types when compiling to JS, void, bool, num, double, int, String, extension types that erases to one of these types, or a type parameter that is bound to a static interop or 'dart:html' type.""",
4750+
"""Use one of these valid types instead: JS types from 'dart:js_interop', void, bool, num, double, int, String, extension types that erase to one of these types, '@staticInterop' types, 'dart:html' types when compiling to JS, or a type parameter that is a subtype of a valid non-primitive type.""",
47494751
arguments: {
47504752
'type': _type,
47514753
},

‎pkg/front_end/messages.status

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -670,8 +670,12 @@ JsInteropOperatorCannotBeRenamed/analyzerCode: Fail # Web compiler specific
670670
JsInteropOperatorCannotBeRenamed/example: Fail # Web compiler specific
671671
JsInteropOperatorsNotSupported/analyzerCode: Fail # Web compiler specific
672672
JsInteropOperatorsNotSupported/example: Fail # Web compiler specific
673-
JsInteropStaticInteropExternalTypeViolation/analyzerCode: Fail # Web compiler specific
674-
JsInteropStaticInteropExternalTypeViolation/example: Fail # Web compiler specific
673+
JsInteropStaticInteropExternalAccessorTypeViolation/analyzerCode: Fail # Web compiler specific
674+
JsInteropStaticInteropExternalAccessorTypeViolation/example: Fail # Web compiler specific
675+
JsInteropStaticInteropExternalFunctionTypeViolation/analyzerCode: Fail # Web compiler specific
676+
JsInteropStaticInteropExternalFunctionTypeViolation/example: Fail # Web compiler specific
677+
JsInteropStaticInteropToJSFunctionTypeViolation/analyzerCode: Fail # Web compiler specific
678+
JsInteropStaticInteropToJSFunctionTypeViolation/example: Fail # Web compiler specific
675679
JsInteropStaticInteropGenerativeConstructor/analyzerCode: Fail # Web compiler specific
676680
JsInteropStaticInteropGenerativeConstructor/example: Fail # Web compiler specific
677681
JsInteropStaticInteropMockMissingGetterOrSetter/analyzerCode: Fail # Web compiler specific

‎pkg/front_end/messages.yaml

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5936,9 +5936,19 @@ JsInteropNonStaticWithStaticInteropSupertype:
59365936
problemMessage: "Class '#name' does not have an `@staticInterop` annotation, but has supertype '#name2', which does."
59375937
correctionMessage: "Try marking '#name' as a `@staticInterop` class, or don't inherit '#name2'."
59385938

5939-
JsInteropStaticInteropExternalTypeViolation:
5940-
problemMessage: "Type '#type' is not a valid type in the signature of 'dart:js_interop' external APIs or APIs converted via 'toJS'."
5941-
correctionMessage: "Use one of these valid types instead: JS types from 'dart:js_interop', '@staticInterop' types, 'dart:html' types when compiling to JS, void, bool, num, double, int, String, extension types that erases to one of these types, or a type parameter that is bound to a static interop or 'dart:html' type."
5939+
# TODO(srujzs): Is there any way to save the correction message into a variable
5940+
# to avoid duplication?
5941+
JsInteropStaticInteropExternalAccessorTypeViolation:
5942+
problemMessage: "External JS interop member contains an invalid type: '#type'."
5943+
correctionMessage: "Use one of these valid types instead: JS types from 'dart:js_interop', void, bool, num, double, int, String, extension types that erase to one of these types, '@staticInterop' types, 'dart:html' types when compiling to JS, or a type parameter that is a subtype of a valid non-primitive type."
5944+
5945+
JsInteropStaticInteropExternalFunctionTypeViolation:
5946+
problemMessage: "External JS interop member contains invalid types in its function signature: '#string2'."
5947+
correctionMessage: "Use one of these valid types instead: JS types from 'dart:js_interop', void, bool, num, double, int, String, extension types that erase to one of these types, '@staticInterop' types, 'dart:html' types when compiling to JS, or a type parameter that is a subtype of a valid non-primitive type."
5948+
5949+
JsInteropStaticInteropToJSFunctionTypeViolation:
5950+
problemMessage: "Function converted via 'toJS' contains invalid types in its function signature: '#string2'."
5951+
correctionMessage: "Use one of these valid types instead: JS types from 'dart:js_interop', void, bool, num, double, int, String, extension types that erase to one of these types, '@staticInterop' types, 'dart:html' types when compiling to JS, or a type parameter that is a subtype of a valid non-primitive type."
59425952

59435953
JsInteropStaticInteropGenerativeConstructor:
59445954
problemMessage: "`@staticInterop` classes should not contain any generative constructors."

0 commit comments

Comments
 (0)