Fixes#39074
DDC emits Dart code that can usually be called with the same semantics
as JS there is no guarantee that a function passed to JS and then
invoked successfully was wrapped with `allowInterop`. The wrapping is
always required in Dart2JS. To make DDC more strict, add interceptors
that check for the usage of `allowInterop`.
Whenever a JS interop function or setter is passed an argument which is
statically typed as a Function, but not wrapped with `allowInterop` at
the call site, wrap it with `assertInterop` which will check the
argument at call time and fail with a clear error if it was not wrapped.
Whenever a JS interop function is torn off, either at the top level or
from an instance, wrap it with a function that will also inject these
checks at runtime.
There are still holes where we can't catch the mistake:
- An argument which is statically dynamic and a Function at runtime
won't be caught.
- A Function which is stored in a collection won't be caught.
- A JS interop definition where a getter returns a Function which takes
a Function as an argument is not checked.
- A dynamic call through to javascript is not checked.
Changes:
- Refactor `_isJsLibrary` and add `isJsMember`, and `isAllowInterop`
utilities to determine what needs wrapping.
- Update `assertInterop` to give a more clear error when it fails, and
to ignore non function arguments.
- Add `tearoffInterop` to wrap a function an ensure that any function
typed arguments are wrapped.
- Inject `assertInterop` around Function arguments passed to JS methods.
- Inject `assertInterop` around Function arguments passed to static or
instance JS setters.
- Inject a runtime wrapper around static or instance Function tearoffs.
- Add a test covering all flavors of checks that are supported.
- Change the interop expando to an `Expando<dynamic>` in the NNBD SDK to work
around a stricter type check. https://github.com/dart-lang/sdk/issues/39971
Potential improvements:
If the `tearoffInterop` turns out to be too heavy, we could loosen it so
that we only wrap methods if any of their argument types are statically
declared to be a Function.
Change-Id: Ibc92df5b54e1a041b4102a07b8398b774b6bd1d2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/128462
Commit-Queue: Nate Bosch <nbosch@google.com>
Reviewed-by: Vijay Menon <vsm@google.com>
Reviewed-by: Nicholas Shahan <nshahan@google.com>
This change hides InstanceCallInstr inside PolymorphicInstanceCallInstr
and makes sure that PolymorphicInstanceCallInstr owns its arguments
(which would become critical once call arguments become instruction
inputs).
Also, PolymorphicInstanceCallInstr now extends TemplateDartCall which
makes it possible to avoid duplication of methods related to all calls.
Issue: https://github.com/dart-lang/sdk/issues/39788
Change-Id: Ie3d4ff46a8e99a0988ba88a88ca9a60be8503ce0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129307
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Also makes option handling more agnostic to default
and negatable values, and disallows trying to use
the preview server outside of upgrade.
Change-Id: Icff10bd016d822edc9c37c0069ebc0c851f78a7e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/128920
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Auto-Submit: Janice Collins <jcollins@google.com>
Commit-Queue: Janice Collins <jcollins@google.com>
The check of not removing 64-bit->32-bit truncating conversions was in the wrong place.
Fixes: https://github.com/dart-lang/sdk/issues/39885
Fixes: https://github.com/flutter/flutter/issues/47454
Change-Id: Ic32a13cbf7ec622692cdca86b3237dae1c0f1cef
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,dart-sdk-linux-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,front-end-linux-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129284
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Also, 'void' is a reserved word, and the grammar does not allow
type arguments after 'void':
<typeNotFunction> ::= void | <typeNotVoidNotFunction>
<typeNotVoidNotFunction> ::= <typeName> <typeArguments>? ... etc
R=brianwilkerson@google.com
Change-Id: Ia54916a8d30ac507bac2133dc3ffabd2c654b09b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129984
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Turns out we aren't testing to ensure that the LSP generator has been
run as part of our test suite. Ideally we'd do so and we'd update the
generator to produce code that doesn't violate lints, but that's more
work than I can do today. Hence this stop-gap CL.
Change-Id: Ia9e5a35817fc4d08e2a4b87b9c2ef64725c822b7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/130004
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
All changes (other than analysis_options.yaml and the change to the generator) made via dartfix.
Change-Id: I7b186371e22048ceddf6fd4407a66aba8b96480c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/130003
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
The extracted code mostly chooses which ErrorCode to report, and
does not do resolution logic itself. So, I think it will make the
resolver itself cleaner to keep it separately.
R=brianwilkerson@google.com
Change-Id: Idc6b652028ddb11913d5cd5c8273299ed94e7079
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129987
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Other than analysis_server/test/analysis/notification_overrides_test.dart,
which had a naming conflict, all of the changes were made using dartfix.
Change-Id: I911dd75dcdee00420caa48724125e86d47c8857d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/130002
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
The only use is assertNoTestErrors(), it cannot be replaced with not
ExpectedError(s) at all, we have 209 tests failing with UNUSED_LOCAL_VARIABLE
R=brianwilkerson@google.com
Change-Id: I19b6e0785e7553c00b130f827b4bb35b0742507f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129831
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
With this approach, UnnecessaryThis is a bit more complicated than
the one outlined in the previous message:
@override
void visitThisExpression(ThisExpression node) {
final parent = node.parent;
Element element;
if (parent is PropertyAccess) {
element = parent.propertyName.staticElement;
} else if (parent is MethodInvocation) {
element = parent.methodName.staticElement;
} else {
return;
}
if (_canReferenceElementWithoutThisPrefix(element, node)) {
rule.reportLint(parent);
}
}
bool _canReferenceElementWithoutThisPrefix(Element element, AstNode node) {
if (element == null) {
return false;
}
var id = element.displayName;
var isSetter = element is PropertyAccessorElement && element.isSetter;
var result = context.resolveNameInScope(id, isSetter, node);
// No result, definitely no shadowing.
// The requested element is inherited, or from an extension.
if (result.isNone) {
return true;
}
// The result has the matching name, might be shadowing.
// Check that the element is the same.
if (result.isRequestedName) {
return result.element == element;
}
// The result has the same basename, but not the same name.
// Must be an instance member, so that:
// - not shadowed by a local declaration;
// - prevents us from going up to the library scope;
// - the requested element must be inherited, or from an extension.
if (result.isDifferentName) {
var enclosing = result.element.enclosingElement;
return enclosing is ClassElement;
}
// Should not happen.
return false;
}
R=brianwilkerson@google.com, pquitslund@google.com
Change-Id: I83e2a3a67f3db43caeb8157116e823268c9bfec7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129819
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
