Regression in argument/parameter types in 2.6.x-dev DDC

[aaronlademann-wf]

Hi there!

We have been smoke testing app builds at Workiva over the past week or so using 2.6.0-dev.7.0. We have noticed a consistent build failure stemming from the typing system.

I have created a reduced test case here: https://dartpad.dartlang.org/6c04d83abb0ce4786974f7a2bb7e23a1

As you can see in dartpad, the program compiles with no trouble using version 2.5.2.

However, if I try to compile the same program by running pub run build_runner build using the 2.6.0-dev.7.0 release, I get the following error:

Error compiling dartdevc module:package_name|web/main.ddc.js

web/main.dart:20:57: Error: The argument type 'FooComponent' can't be assigned to the parameter type 'BaseUiComponent<Null>'.
 - 'FooComponent' is from 'main.dart'.
 - 'BaseUiComponent' is from 'main.dart'.
    return customRenderer(propsInstance, stateInstance, this);
                                                        ^

[vsmenon]

This is the code:

// @JS()
class ReactElement {}

abstract class UiState {}
abstract class UiProps {}
abstract class BaseUiComponent<T extends UiProps> {}
abstract class BaseStatefulUiComponent<T extends UiProps, S extends UiState> extends BaseUiComponent<T> {}
abstract class UiComponent<T extends UiProps> extends BaseUiComponent<T> {}
abstract class UiStatefulComponent<T extends UiProps, S extends UiState> extends BaseStatefulUiComponent<T, S> {}

class FooProps extends UiProps {
  bool someField = false;
}
class FooState extends UiState {}
class FooComponent extends UiStatefulComponent<FooProps, FooState> {
  final propsInstance = FooProps();
  final stateInstance = FooState();
  
  ReactElement render(CustomRenderFunction customRenderer) {
    return customRenderer(propsInstance, stateInstance, this);
  }
}

typedef ReactElement CustomRenderFunction<T extends UiProps, S extends UiState, C extends BaseUiComponent<T>> (T props, S state, C component);

void main() {
  final componentInstance = FooComponent();
  print('is FooComponent an instance of BaseUiComponent? ${componentInstance is BaseUiComponent<FooProps>}');
  
  componentInstance.render((props, state, instance) {
    print('is FooComponent an instance of BaseUiComponent inside the callback closure? ${instance is BaseUiComponent<FooProps>}');
    print((props as FooProps).someField);
    return ReactElement();
  });
}

and this is the static error from the front end:

example.dart:19:57: Error: The argument type 'FooComponent' can't be assigned to the parameter type 'BaseUiComponent<Null>'.
 - 'FooComponent' is from 'example.dart'.
 - 'BaseUiComponent' is from 'example.dart'.
    return customRenderer(propsInstance, stateInstance, this);

I suspect this was a bug we fixed in the CFE. Note, if you explicitly provide types here:

  ReactElement render(CustomRenderFunction<FooProps, FooState, BaseUiComponent<FooProps>> customRenderer) {
    return customRenderer(propsInstance, stateInstance, this);
  }

it works.

@johnniwinther @lrhn @leafpetersen - was there an intentional change / fix here?

[vsmenon]

Marking as front-end, as all backends behave consistently.

[robbecker-wf]

I would expect this to work without it crashing as it did in prior Dart releases. Is this seen as a bug or as a side effect of an intentional change?

[chloestefantsova]

If I'm reading the Dart Language Specification correctly, reporting the error is the intended behavior in the example above. The reason is that providing missing parameters to generic typedefs is made dependent on how the type parameters of the typedef occur in the type it's describing.

Type CustomReaderFunction in the signature of render is missing type arguments, and they are provided by the algorithm described in section 14.3.2 The Instantiation to Bound Algorithm. In the notation of the algorithm, we have U_1,0 = UiProps, U_2,0 = UiState, U_3,0 = BaseUiComponent<T>, and V₀ = CustomRenderFunction<UiProps, UiState, BaseUiComponent<T>>. The condition for step 1. aren't met, so we move to step 2. where to obtain V₁ from V₀ we substitute T with Null in V₀ because it's in a contravariant position there. The position is contravariant as per section 14.1 Variance of the spec.

I landed a CL some time ago to make the CFE compliant to the spec in this regard.

[eernstg]

Agreeing entirely with @stefantsov about the explanation, I can add that the instantiation to bound algorithm is intended to produce a common supertype of all the possible types of instances of the given generic type, except that this is not possible in some cases.

So, e.g., if C is a generic class taking one type argument, the raw type C will generally desugar to a parameterized type C<T> such that every instance of C has a type which is a subtype of C<T>.

In some cases that's easy (for C<X extends num> {} we just choose num as the type argument). An example where it is not possible is typedef F<X> = X Function(X). With that declaration, F<T1> and F<T2> are just unrelated for any pair of distinct types T1 and T2, so there is no type T0 such that F<T0> is a common supertype.

The rule that cycles are eliminated in contravariant positions by Null serves to make the outermost type more general than any actual value the given type variable could have, such that we get this common supertype.

In the given example this means that it is necessary to pass the type argument as in BaseUiComponent<FooProps> in order to get a type for customRenderer which is not a common supertype. This is inconvenient because it is a breaking change and because FooProps occurs twice, but the trade-off is that instantiation to bound more consistently produces a common supertype (which makes the outcome of using a raw type more predictable).

Another matter is that it would be convenient to use a type alias to eliminate the redundancy:

// This feature is planned, but not yet implemented.
typedef BaseUiRenderFunction<TProps extends UiProps, TState extends UiState> =
    CustomRenderFunction<TProps, TState, BaseUiComponent<TProps>>;

We could then express the type as BaseUiRenderFunction<FooProps, FooState>.

[leafpetersen]

Smaller reproduction here:

class A<T extends num> {}
typedef void F<T extends num, C extends A<T>>(T p, C c);

void test(F f) {
  f(3, A<num>());
}
void main() {
}

I would expect this to work without it crashing as it did in prior Dart releases. Is this seen as a bug or as a side effect of an intentional change?

cc @eernstg

If I'm reading this correctly, I believe that this is a result of a change in the specification landed late last year. It looks like we are in an inconsistent state with the implementations though: the CFE has changed this, but the analyzer has not. In either case, we should have explicitly rolled this out as a breaking change.

We will need to either:

• roll back the CFE change re-land it after a breaking change announcement (preferably with the analyzer synced up)
• roll forward (fix the analyzer)
• roll back the specification change

@robbecker-wf how bad of a break is this? Are you seeing widespread breakage in packages/apps?

I'm guessing we haven't seen any internal breakage because of this, since we now have all compilers on the CFE internally?

[eernstg]

@leafpetersen wrote:

but the analyzer has not [implemented this yet]

It was a very long process to get this change landed, and the fact that there was essentially zero breakage internally played an important role. This was before we had a breaking change process.

I consider the more consistent behavior of the algorithm (which makes it simpler to explain what it does) a benefit, but we may of course need to go through a breaking change process if it turns out to be more breaking than expected.

[robbecker-wf]

how bad of a break is this?

It was not great, but we've verified that we can fix it. Since it seems to be only us reporting this as a problem, it was hard to get in initially and it's been in for a long time .. I would lean toward fixing it forward by having the analyzer be consistent with CFE.

Maybe there are already things happening to accomplish this, but it seems it would be very beneficial to get things in place to ensure more consistency between the various pieces. We've been heavily affected by differences between the analyzer and compilers, and between the different compilers.

[aaronlademann-wf]

@eernstg @leafpetersen @stefantsov

We have found a workaround for our libs that only requires a change at the typedef itself - not in any of the implementations that utilize the typedef. So at this point - we're ok with you guys moving forward / not reverting.

Its very encouraging to hear that you have implemented a breaking change review process / release process to prevent these types of breakages in minor releases in the future. In the past, there have been a number of them - specifically around the behavior of generic parameters - that have cost Workiva a tremendous amount of time and money to either work around, troubleshoot in an effort to get a fix, or both.

With this change in particular - I'd like to request that the analyzer behavior be updated to match what the compiler does.

In general - with any of these types of changes - and with the handful of open issues that center around the compile time behavior of DDC differing from what the analysis server does - some amount of "good faith" is lost. One of the biggest "selling points" of the Dart 2 release was the concept of a "common" front-end. The idea that - if the analyzer didn't complain, DDC wouldn't either. However, there have been numerous examples since the release of Dart 2 that call into question whether that "commonality" is something that is stable that consumers can truly rely on in their developer ecosystems. Furthermore, there continue to be major differences between the JS that is generated by the DDC versus Dart2js - which causes more pain for consumers - especially very large ones like Workiva.

[aaronlademann-wf]

how bad of a break is this?

Compared to the OOM errors that continue to crop up in tests, and now in non-test dart2js compilations - its miniscule. The memory issues are much more concerning, IMO.

[eernstg]

@robbecker-wf wrote:

We've been heavily affected by differences between the analyzer
and compilers, and between the different compilers

That is definitely a kind of problem that shouldn't exist, but we know it does come up. The 'common' in 'common front end' would be very helpful in this respect, as more and more parts of the tool chain use it. Some elements will remain incompatible even then, e.g., computations using web numbers respectively vm numbers in Dart will not behave identically.

@aaronlademann-wf wrote:

a "common" front-end .. However .. there continue to be major differences

Thanks for emphasizing this point again, it is obviously important.

The common front end is widely used in our tools, but the analyzer has its own static analysis. The analyzer needs to do much more recovery work when it performs an analysis on a program with missing parts or errors of any kind, so the transition where the analyzer switches over to use the common front end is quite complex and will not happen in the near future. With respect to the differences between DDC and dart2js I don't know the details so I can't comment on that (and similarly for the memory issues).

But with respect to the change to instantiation to bound I hear support for moving forward:

I would lean toward fixing it forward by having the analyzer be consistent with CFE

I'd like to request that the analyzer behavior be updated to match what the compiler does.

[robbecker-wf]

Given that we have a fix on our end for the compiler crash and the work going forward is on the analyzer, should this issue be closed and a new one to track the analyzer fix be made?

[eernstg]

@leafpetersen, @munificent, @lrhn, can we decide on the next step for this at the language meeting on Wednesday?

[robbecker-wf]

I'd be interested to hear back after today's meeting if this is discussed.

[eernstg]

There is general support for moving forward and implementing the change in the analyzer, but we need to scrutinize a couple of other changes that may be related.