Dart2JS and DDC differ in reified typedef signature

[matanlurey]

This is affecting prod customers for AngularDart internally, as the program works as intended during local development and in tests (DDC), and fails mysteriously in production (Dart2JS). Here is a minimal example showing this difference in behavior:

void main() {
  print(identical(FunctionThatReturnsString, FunctionThatReturnsString));     // true
  print(identical(FunctionThatReturnsString, FunctionThatAlsoReturnsString)); // false
}

typedef FunctionThatReturnsString = String Function();
typedef FunctionThatAlsoReturnsString = String Function();

This seems to work-as-expected for DDC, but not Dart2JS.

Is this intended? If so, we'll need to ban typedef as being used as a dependency injection token-type in AngularDart, as it is dangerous to use. If this is fixable, then lets fix it! Let me know (this is P1 internally).

/cc @rakudrama @vsmenon @jmesserly

[jmesserly]

This is a bug in DDC: #32785 ... they should be equal, as they are equivalent function types. Let me dig up my old branch with the fix and see what the status is of that branch.

[jmesserly]

merging into the other bug.

[jmesserly]

I was confused, the other bug is about == not identical.

I'll check the spec on identical, but it may be up to the implementation. DDC's behavior for identical is a little more intuitive to me (and necessary, if we want to provide good error messages). I suspect combining the two types may be a result of dart2js's minification.

In general, I would not recommend relying on identical for types; it may expose implementation details of the compiler/runtime's canonicalization scheme, and it doesn't have any meaning semantically.

For example once #32785 is fixed, I'd like to change DDC so it doesn't canonicalize all (non-typedef) function types. So () -> int will no longer be identical between two modules, although it will be equal. If that makes sense.

[matanlurey]

@jmesserly:

In general, I would not recommend relying on identical for types; it may expose implementation details of the compiler/runtime's canonicalization scheme, and it doesn't have any meaning semantically.

Thanks! I wish we could 😢 Unfortunately we use identical to check tokens for dependency injection, mostly because this is critical-path code (used 10000s of times) and we guarantee, statically, that we only rely on identity-based equality (not any overloaded == operators).

That is, we want Dart2JS to always emit: if (a !== b), not: if (interceptor.$equals(a, b)). We could try and special case Type, i.e. if we can tell the token is a Type, have it flow through == not identical, but I'd prefer not to do that (it will be confusing if/when it doesn't work as intended).

For example once #32785 is fixed, I'd like to change DDC so it doesn't canonicalize all (non-typedef) function types. So () -> int will no longer be identical between two modules, although it will be equal. If that makes sense.

I think at minimum that will make DDC match the Dart2JS experience, which is important for us. From our perspective, we are evaluating either (a) banning using typedef as a token or (b) special-casing Type to flow the == not ===.

[jmesserly]

We could try and special case Type, i.e. if we can tell the token is a Type, have it flow through == not identical, but I'd prefer not to do that (it will be confusing if/when it doesn't work as intended).

You may have to special case Type, and not just for typedefs/function types. Generic classes do not need to be identical either. In other words C<int> and C<int>. Currently DDC canonicalizes all generic types at runtime, but that's both a startup/performance cost and a memory leak (we can't collect types). In dart2js, equal Types are not necessarily identical. It may happen to work in some cases, but it's not a good idea to rely on that IMO.

[jmesserly]

Just had another idea that might help you. If y'all need canonicalization, you could implement that on top of Type.==:

final _injectionTypes = HashSet<Type>();
Type canonicalizeType(Type type) =>
    _injectionTypes.add(type) ? type : _injectionTypes.lookup(type);

The benefit of that is you have control over which Types to canonicalize, and can decide how to manage the GC issues. That way the canonicalization cost isn't imposed on all Types, only on those that really need it (e.g. for injection). It's a neat trick for speeding up == on immutable objects, at the cost of some initial overhead, since it's safe to use identical after the canonicalization step.

[matanlurey]

One extra confusing point for Dart2JS. The following triggers an error in analyzer:

void main() {
  // ERROR: Two keys in a map literal can't be equal.
  print(const {Fn1: 'Fn1', Fn2: 'Fn2'});
}

typedef Fn1 = String Function();
typedef Fn2 = String Function();

... but Dart2JS executes this program fine, printing {() => String: Fn2}.

/cc @rakudrama @jmesserly - is this correct?

[eernstg]

@jmesserly wrote:

I'll check the spec on identical, but it may be up to the implementation.

Yes, but the answer is actually not entirely obvious.

There is one exception: 'A simple or qualified identifier denoting a class or type alias that is not qualified by a deferred prefix' is a constant expression and must be canonicalized. So int == int and identical(int, int).

But no such promises are made for other terms denoting types, e.g., List<int> or void Function(int). The latter are not expressions so we cannot evaluate them to a Type directly, but we can obtain the value of a type variable:

Type typeOf<X>() => X;

main() {
  print(typeOf<List<int>>() == typeOf<List<int>>()); // (1).
  print(identical(typeOf<List<int>>(), typeOf<List<int>>())); // (2).
}

However, I believe that the specification is silent on the expectations about equality (where I'd like to require cases like (1) to yield 'true') and identical (where I'd like to let implementations decide in cases like (2)).

We have language team support for #32782 'Type values obtained from a typedef should represent the underlying type, not the typedef', which means that we have precedence for requiring equality for function types according to their structure, whereas identical is implementation defined.

Similarly, generalized-void.md specifies that void and Object must behave similarly at run time:

In case of a dynamic error, implementations are encouraged to emit an error message that includes information about such parts of types being `void` rather than `Object`. Developers will then see types which are similar to the source code declarations. This may be achieved using distinct `Type` objects to represent types such as `F<void, void>` and `F<Object, void>`, comparing equal using `==` but not `identical`.

So it turns out that we haven't actually specified it completely yet, but the only approach that matches the cases where we get close is that we'd require equality based on the structure of types (so Types for List<int> and List<int> must be equal, even if they were obtained on separate occasions), but it is up to each implementation whether they are also identical.

I've noted that we will probably need to clarify the spec on this topic.

[jmesserly]

regarding:

One extra confusing point for Dart2JS. The following triggers an error in analyzer:
... but Dart2JS executes this program fine, printing {() => String: Fn2}.

That sounds like a bug, probably in the CFE. They haven't implemented all of the static checking for constants yet.

[eernstg]

@rakudrama, I suppose you changed this to 'area-language' because there is a need to clarify the rules? I created issue #34447 in order to get that done. So we may keep this as 'area-language' until #34447 has been resolved (with a language team decision), or we could mark this one as blocked on #34447, depending on how you'd usually handle such situations.

[eernstg]

@rakudrama, cf. my question in #34429 (comment), I'm removing 'area-language' from this issue. Please re-adjust if you prefer to handle it differently. Note that #34447 may be closed when this CL is landed.

[eernstg]

#34447 is now closed.

[jmesserly]

Update: now that DDC behavior matches dart2js (identical for both cases) & Erik added the spec, can we close this?

[eernstg]

Closing, I believe there are no more tasks here: The spec part was handled in #34447 and the DDC part in #32785.