dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.24k stars 1.57k forks source link

Order of argument type checks is unclear. #34075

Open sjindel-google opened 6 years ago

sjindel-google commented 6 years ago

When argument types are dynamically checked for strong mode, is the order of the checks defined? Or can they be checked in any order?

For example, consider this code:

abstract class C<T> {
  void foo(Object x, T y);
}

class D<S, T> extends C<T> {
  void foo(covariant S x, T y) {
    // ...
  }
}

main() {
  C<Object> c = new D<int, int>();
  c.foo("error", "error");
}

Is is a valid behavior for this program to throw an exception for the incorrect type of "y" (having been checked before "x")?

leafpetersen commented 6 years ago

cc @lrhn @eernstg I see no reason to tie this down in the spec. I would prefer us to explicitly allow any order of checks so long as no code is observed to have run in between the checks.

sjindel-google commented 6 years ago

For context, this could facilitate an optimization we're currently evaluating in the VM wherein the code D.foo above would have two entry-points, one which would skip the check against y. In this scenario, would like to order the code for checking y before checking x so that the entry-point which skips y can just jump to the check for x directly.

lrhn commented 6 years ago

I agree that there is no advantage, and some disadvantage, in specifying which error is thrown first. It is an error during the parameter binding of the invocation if all arguments are not assignable to the corresponding formal (or if there are arguments with no corresponding formal, in the case of dynamic invocations).

rakudrama commented 6 years ago

I think the order should be specified.

For dart2js we will likely want to keep the order consistent so that errors in production are repeatable as the program evolves. It would be a disadvantage to have the 'same' error clustered into multiple groups because each push of the program used a different order and reported a different TypeError. Developers need to be able to tell that the most recent push fixed the problem, not just caused it to express differently.

Samir, if I understand, you say that an unconstrained order permits you to reorder checks to enable better tail-merging of the check sequence. Can you quantify the benefit?

leafpetersen commented 6 years ago

For dart2js we will likely want to keep the order consistent so that errors in production are repeatable

This is consistent with the order being unspecified.

sjindel-google commented 6 years ago

@rakudrama: I can't quantify the benefit yet, since we're still working on the optimization. However, I understand that we're releasing Dart 2 tomorrow, and making it unspecified after it is already specified would be a breaking change.

eernstg commented 6 years ago

We have previously preferred specifying the execution order for many parts of the language (typically: following the textual order), based on the argument that this is good for correctness and predictability. I believe that we have also explicitly noted that this may prevent certain optimizations (exactly like the one which is mentioned here).

Following that rationale I'd tend to prefer a specified and deterministic ordering, based on the textual order of the declarations.

However, I do acknowledge that this is a corner case and it's unlikely to cause many bugs, so I won't push for it if there's a strong push for leaving it unspecified.

mraleph commented 6 years ago

We are already performing optimizations in the AOT where we split function prologue into two parts complementing each other: one of the parts contains just covariant checks, while the other becomes a separate forwarding stub and contains all of the checks and is used when arriving from contexts that we consider dynamic.

Requiring the specific order of checks would invalidate this optimization in the current form: we would need the forwarder to contain all the checks meaning an potential code size increase.

In general I would favor leaving this unspecified because it simplifies implementations and gives them a bit of flexibility.

The behavioral difference can only be observed on incorrect programs

eernstg commented 6 years ago

OK, we may then want to make it explicit in the specification that this particular kind of ordering is unspecified. Are we only talking about the ordering among dynamic type checks on parameters of a function invocation, or does it involve the ordering of dynamic type checks relative to other actions as well?

mraleph commented 6 years ago

Only between type checks themselves. We can still specify that they happen before any other code in the body is executed.

rakudrama commented 6 years ago

It is easy to underestimate the challenges of debugging deployed programs. "The behavioral difference can only be observed on incorrect programs" - incorrect programs cause a disproportionate part of the engineering cost of deployed programs.

Does 'unspecified' include 'nondeterministic', i.e. a different order on a repeated call with the same arguments?

eernstg commented 6 years ago

Can we put an upper bound on the non-determinism of an invocation of a function where more than one actual argument violates the actual value of the type annotation of the corresponding formal parameter?

Maybe: Such an invocation will incur a dynamic error which may be based on any one among the actual arguments violating the type annotation.

I wouldn't be surprised if it were possible to create a typing situation where the exact same objects may be passed, but the choice of culprit will be different because the associated static typing situation is different.

On the other hand, we may be able to promise that there is no non-determinism in the side effects associated with the evaluation of the actual argument expressions (because they are all evaluated before these dynamic type checks take place).

Or what do you think we can promise?

Oh, and @mraleph said:

... [the actual argument type checks must] happen before any other code in the body is executed.

which I think we must definitely enforce in any case.

lrhn commented 6 years ago

There is no breaking change in making this decision. The specification is clear-ish (still written in Dart 1 terms) - it is a run-time error if you cannot bind any of the arguments to its corresponding formal because of non-assignability. The specification does not say which error, nor does it say which order to check.

This only applies to dynamic invocations and covariant arguments (declared or from generics), since all other parameter passing cases are guaranteed to be successful by the static type system.

So, the only difference this might make is to cause a different of two or more simultaneous errors to be reported. All the errors must be fixed anyway, and fixing one will expose the other.

Over-specifying error conditions is a great way to introduce unnecessary overhead. I'd prefer not to do that.