ClassHierarchy.forEachOverridePair contains special logic for unusual
cases like this one:
class A {
void foo() {}
}
class B extends A {
void foo();
}
main() {
B b = new B();
b.foo();
}
In this case, A.foo is considered to override B.foo (contrary to the
usual situation where the derived class method overrides the
superclass method). The reasoning is that calling foo on a concrete
instance of B will cause A.foo to be executed (as illustrated in
main); therefore A.foo is callable via the interface of B.foo, thus in
a sense A.foo "overrides" B.foo.
The code contained a questionable optimization, however; it only
executed this special logic if the derived class was concrete.
Presuambly the reasoning was that if B were abstract, then a concrete
instance of B could never be created, so this situation could never
arise.
However, there is nothing to stop a concrete class from being derived
from B, e.g.:
class A {
void foo() {}
}
abstract class B extends A {
void foo();
}
class C extends B {}
main() {
B b = new C();
b.foo();
}
Now, calling foo on a concrete instance of C will cause A.foo to be
executed (as illustrated in main); therefore A.foo is callable via the
interface of B.foo, as before. So we still need to report this as an
override pair even though B is abstract.
R=ahe@google.com, scheglov@google.com
Review-Url: https://codereview.chromium.org/2998383002 .
Summary:
Previously, we use the "Vector" type in the kernel tree for the result of the
"VectorCreation" operation as as the parameter type for converted closure
functions. In the VM, we use the "Context" type instead, which the VM treats a
little differently than normal Dart-visible types, and it doesn't not handle
type checks against it correctly, breaking all closure converted code running in
checked mode.
Now, Since we are forced to use dynamic to represent the types of elements of
the context, we may as well just use dynamic for the context type itself.
Previously, we did not correct handle converted closure type checks for closures
that capture type parameters. The way we handled these type checks also had
several latent bugs that prevented type parameters being handled properly.
Now, we handle type parameters for converted closures similarly to normal
closures, and the places we treat them specially are fewer and more integrated
with the rest of the closure type checking code.
There is still a problem where assignments to captured variables are not
checked, because they are transformed to assignments into the context, whose
elements are necessarily untyped. This breaks many co19 tests, which expect type
errors on these assignments. The example below should error in checked mode, but
after closure conversion is runs with no errors.
int b;
bool c;
(() { b = c; })()
Test Plan:
- All test cases in "pkg/kernel/testcases/closures" now run in checked mode.
- Added a test "closures_types.dart" to check that captured type parameters are
handled correctly in the converted closures' signature types.
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/3007623002 .
The front end will need to use this method to iterate through the
interface of a class in order to determine when to create forwarding
stubs.
The functionality already exists; this CL merely exposes it and adds
tests.
R=scheglov@google.com
Review-Url: https://codereview.chromium.org/3003913002 .
Summary:
1. Previously, in 'BuildGraphOfConvertedClosureFunction', the VM was unable to
correctly forward parameters to converted closure functions when
they were captured in the converted function's body. This could happen when, for
example, a closure was introduced into it by async conversion.
Now, this is fixed by an approach that mirrors the technique in
'BuildGraphOfFunction'.
2. Previously, local variables declared inside loop bodies were being saved in
the loop's enclosing context, so closures within the loop would see new
values initialized to the variable in subsequent iterations.
Now, this is fixed by creating nested contexts for all loops, regardless of
whether the loop variables are captured.
3. Previously, arity checks were not being performed on converted closures, so
they could be called with too few or too many arguments. In the former case, the
missing arguments would be filled in with garbage on the stack.
Now, the assembly generation in 'CompileGraph' inserts argument count checks
for converted closures as well as regular closures.
Test Plan:
Introduced new tests in the closure conversion suite to test each bug:
1. syncstart.dart
2. loop2.dart, blocks.dart, updated for_in_closure.dart
3. arity.dart
With these changes, closure conversion passes all co19 tests in non-checked mode, except those that are not passed without it:
python tools/test.py -m release -c dartk --vm-options "--reify --reify_generic_functions" co19
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/3000333002 .
- Evaluation of PropertyGet expression by accessing a field,
execution of a getter or creating a method tearoff.
- Evaluation of PropertySet expression by setting a field or
execution of a setter.
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/2999673002 .
- Evaluation of StaticPropertyGet expression by reading a value stored in
the main environment, execution of static getter or creating a method tear off.
- Evaluation of StaticPropertySet expression by modifying the value
stored in main environment or execution of static setter.
BUG=
R=dmitryas@google.com, kmillikin@google.com
Review-Url: https://codereview.chromium.org/2997563002 .
Previously, the captured variable analysis did not provide sufficient
information for the conversion phase regarding variable uses in initalizers: in
particular, it did not differentiate the case when a variable is used in an
initializer and captured in the body vs. being captured in the body and not used
in an initializer. In addition, there were a few bugs stemming from the use
of lazy iterables and OR conjunctives with effectful operations.
Now, we separate the information about which variables are captured from flags
indicating whether variables are used in initializers. The other bugs are fixed
in obvious ways.
Finally, we reintroduce some code that ensures that redirecting factory
constructors listed in "_redirecting#" field (a hack used when writing DILL
files) remain with one-expression bodies after closure conversion.
Test Plan:
Added a test case for the initializers bug, ensured that the patched SDK builds
with closure conversion always-on.
Reviewers: dmitryas@google.com
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/2995083002 .
Summary:
Previously, there was no support for generic methods in kernel. This prevented
us from being able to pass captured type arguments to the target top-level
function in converted closures, so these type arguments were always instantiated
to 'dynamic'.
Now, we save the type arguments to the closure creation operation in the
context, and read them out and forward them appropriately in closure wrapper
function. Since fasta doesn't currently support generic methods (their type
parameters are replaced by 'dynamic'), only top-level generic functions can
surface in kernel, as they are generated by closure conversion of closures that
capture type parameters of a class.
My focus here is enabling closure conversion to work in only these cases, and as
such, the code has some temporary "hacks" in the VM that may not work for
generic member functions or generic closures when they are enabled in fasta.
Test Plan:
I ran all the tests in closures/, and those which were previously expected to
crash due to missing VM support now pass and produce correct results.
Further testing is paused until we understand why the recent commit "[kernel]
Insert kernel bodies into VM heap" has broken all these tests.
Reviewers: regis@google.com, jensj@google.com, dmitryas@google.com
BUG=
R=dmitryas@google.com, jensj@google.com
Review-Url: https://codereview.chromium.org/2998803002 .
This CL copies the kernel bodies for all functions and
fields into the VM heap. The function bodies in the VM
heap are then used when compiling the flowgraphs.
This theoretically means that the malloc'd data can be
freed and that snapshotting from kernel could possibly
work, though it hasn't been tested.
R=kmillikin@google.com
Review-Url: https://codereview.chromium.org/2972343002 .
Prior to this CL we carried around information about the containing class
and member, both of which was fetched by reading out-of-line in the binary
(i.e. while reading the current member, start reading something from the
parent member etc).
It had also required the introduction of extra fields in the kernel
binary file (dill file).
This CL cleans that up, by
a) Setting type parameters on functions as needed (in kernel_reader.cc)
b) Using the VM Class and VM Function to get the required information
(with a above the information is all available).
(in kernel_binary_flowgraph.cc.) This means that
c) We don't have to read the binary out-of-line (for TypeParameterType
to work at least), and that
d) We can remove the previously introduced extra fields from the
kernel binary file (dill file).
R=dmitryas@google.com, kmillikin@google.com
Review-Url: https://codereview.chromium.org/2973633002 .
Summary:
Previously, when a function parameter was captured both in an initializer and in
the body of a constructor, we would create two contexts: one created in a local
initializer and used by other initializers, and another in the body of the
function. This is incorrect, as it means that changes to the parameter in the
initializer's closure won't be visible in the body.
Now, to work around this problem we re-use the context created for the
initializers in the body of the constructor by moving the body into a new
constructor, and redirecting the original constructor to that one, passing the
context as an additional argument. This dance is necessary because local
initializers aren't visible in the body of a constructor.
Test Plan:
A few of the existing closure conversion tests were changed or fixed by this
revision. We also modify the 'closure_in_initializer.dart' test to hit this case
directly.
R=dmitryas@google.com
Reviewers: dmitryas@google.com
Review-Url: https://codereview.chromium.org/2991853002 .
Summary:
Previously, we filled in all occurrences of captured type variables with either
"dynamic" or their bound, if they had one.
Now, we add extra type parameters to the top-level function corresponding to the
closure, and pass in the corresponding arguments as type arguments to the
"MakeClosure" operation.
Test Plan:
Updated [type_variables.dart] and added a new test case to it.
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/2989563002 .
The closure-conversion transformation is not enabled yet. This commit
only adds the support for it to FlowGraphBuilder and
StreamingFlowGraphBuilder. More work should be done before enabling the
transformation; most mportantly, the 'platform.dill' file that is used
in the Kernel isolate and is loaded by VM for linking with executed
programs should be separated. The former should receive a file not
touched by the transformation, and the latter should receive a
transformed one.
BUG=
R=jensj@google.com, karlklose@google.com, kustermann@google.com
Review-Url: https://codereview.chromium.org/2891053003 .
It is informative in the same way as documentation.
Yesterday I tried to make Analyzer and Analysis Server stop using
FieldFormalParameterElement(s) and found one important use case requested
by the Flutter team. We need to show documentation of a field when
user requests documentation on the corresponding field formal named
parameter. This happens outside of the file that declares the parameter
and the field, so we don't have AST available. Of course we could
resolve the unit, but it would cost us something. And if we decide
that it is OK, then maybe we don't need to have documenation comments
in elements at all.
This is of course more convenience, and we could store documentation
and parameter to field mappings outside, like we store index. Just a
compromise - convenience vs. purity.
R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=
Review-Url: https://codereview.chromium.org/2983173002 .
This review scraps the (currently disabled) code for converting tearoffs in the
closure conversion pass.
The closure conversion pass can only ever do a partial job with tearoffs, due to
the possibility of an unconverted library tearing off a method from any object
it likes. Partially converting [PropertyGet]s makes the closure conversion pass
slower and introduces a new method for any field or method anywhere with a name
used in any [PropertyGet], inflating code size and potentially regressing
performance. As it provides no concrete value in return we've decided to scrap
this aspect of the transformation. Anyway, creating closures for tearoffs is
much easier for a backend than converting anonymous or nested functions, since
there is only one object ("this") captured. Thus ignoring tear-offs does not
undermine the value of the transformation.
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/2986553002 .
Paul Berry