Skip to content

Kernel representation of function types regresses angular code generation and analysis server quick fixes/refactorings #30961

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
stereotype441 opened this issue Oct 2, 2017 · 12 comments
Assignees
Labels
front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead.

Comments

@stereotype441
Copy link
Member

In the representation of function types used by the analyzer, it was possible to get information from a FunctionType about the typedef that created it. This is not possible with the current implementation of kernel.

We are in the process of reworking the analyzer API to be more compatible with kernel, and we've noticed two consequences of this difference:

  • It makes code generated by the Angular package less readable. Sometimes Angular needs to create a class field with the same type as a constructor argument; if this type refers to a typedef, Angular creates a field whose type also refers to the typedef. Under the current kernel design, this will need to be changed so that Angular either (a) inserts its own typedef, (b) uses the new generalized function type syntax (e.g. int Function(bool)), or (c) uses heuristics to try to locate the original typedef at code generation time.

  • It decreases the quality of code generated by the analysis server under the user's direction. For example, the following source code has a compile error due to the fact that bar is not defined. If the user asks the analysis server to fix the error by generating a bar function, it generates it with the signature void bar(F x). It behaves similarly with refactorings such as extract method and extract variable. Under the current kernel design, this will need to be changed so that it either (a) generates a more verbose function signature like void bar(int x(int value)), or (b) uses heuristics to try to locate the original typedef at code generation time.

typedef int F(int i);
void foo(F x) {
  bar(x);
}

Note that a similar situation applies to the generic parameters of typedefs, e.g. in the following code, the analysis server currently can generate a bar function with the signature void bar(F<int> x); under the current kernel design, it would have no way of preserving the type argument int, even with heuristics:

typedef void F<T>();
void foo(F<int> x) {
  bar(x);
}
@stereotype441
Copy link
Member Author

@jcollins-g is experiencing a similar issue with dartdoc.

@jcollins-g
Copy link
Contributor

For dartdoc, this change makes all typedefs using the new syntax into anonymous typedefs. This breaks linkage in a way that's similar to the consequences described for Angular code generation -- dartdoc now no longer refers to the typedef by name but expands it out into its definition wherever it is referenced.

@jcollins-g
Copy link
Contributor

Attempts to work around this for dartdoc via the analyzer are currently stymied by missing type information when reading references to typedefs. It's simply gone in FunctionTypeAliasElement.type. When I try to hack around that by using enclosingElement similarly to how I got the name, I lose the parameter value (e.g. T instead of needed bool).

@scheglov
Copy link
Contributor

scheglov commented Oct 3, 2017

Clarification, which I thought everybody knew already, but apparently not.

There is typedefReference field in FunctionType, but at the moment is is incomplete - we need to add also type arguments, update them when we substitute types in FunctionType, and serialize with FunctionType.

@sjindel-google
Copy link
Contributor

typedefReference is already serialized with FunctionType.
It's also not necessary to save the type arguments, since they can be re-derived with unifyTypes.
@stereotype441 Is there anything else you need here?

@stereotype441
Copy link
Member Author

@sjindel-google unifyTypes would handle most situations, but there is a corner case it wouldn't handle: the case where the typedef has a type parameter that is unused. Consider the example below:

typedef void F<T>();
void foo(F<int> x) {
  bar(x);
}

Under the current implementation, the type of x is stored in the kernel as () -> void, with a pointer to the declaration of F. Even with unifyTypes, there's no way to recover the fact that the type argument applied to F was int. So if the user tried to invoke an analysis server quick fix to auto-generate the signature for bar, the analysis server would have to have a fallback path to handle this situation; perhaps it would generate bar(F<dynamic> x), or perhaps it would generate bar(void Function() x).

@scheglov do you think this is ok, or do you think we need to be able to generate bar(F<int> x) in this corner case?

@scheglov
Copy link
Contributor

Ah, I didn't know about unifyTypes.
I think we can live without arguments for unused type variables.

@scheglov
Copy link
Contributor

Hm... Does unifyTypes work for function types?

This test case does not work for me.
successCase('<T>(int) => T', '(int) => double', {'T': 'double'})

image

@sjindel-google
Copy link
Contributor

It does indeed work:

import 'pkg/kernel/lib/ast.dart';
import 'pkg/kernel/lib/type_algebra.dart';
import 'pkg/kernel/lib/core_types.dart';

main() {
  var S = new TypeParameter();
  var T = new TypeParameter();
  var U = new TypeParameter();

  var F1 = new FunctionType(
      [new TypeParameterType(T)],  // params
      new TypeParameterType(U)  // return
  );

  var F2 = new FunctionType(
      [new TypeParameterType(S)],
      new TypeParameterType(U));

  var quantifiedVariables = new Set<TypeParameter>();
  quantifiedVariables.add(T);

  print(unifyTypes(F1, F2, quantifiedVariables));
}

I believe you are using it wrong. You need to remove the type parameter T from the first function's signature. Otherwise the two function types cannot be unified since they have different arity.

@sjindel-google
Copy link
Contributor

@sjindel-google unifyTypes would handle most situations, but there is a corner case it wouldn't handle: the case where the typedef has a type parameter that is unused. Consider the example below:

typedef void F<T>();
void foo(F<int> x) {
  bar(x);
}

Under the current implementation, the type of x is stored in the kernel as () -> void, with a pointer to the declaration of F. Even with unifyTypes, there's no way to recover the fact that the type argument applied to F was int. So if the user tried to invoke an analysis server quick fix to auto-generate the signature for bar, the analysis server would have to have a fallback path to handle this situation; perhaps it would generate bar(F<dynamic> x), or perhaps it would generate bar(void Function() x).

I don't know that generating bar(F<int> x) is actually the right option here. In any case, I feel that the burden of maintaining a separate type arguments list in the function type outweighs the marginal benefit of seeing bar(F<int> x) here. Also, note that not all FunctionTypes are required to have a defining typedef in Kernel proper.

@scheglov
Copy link
Contributor

scheglov commented Oct 18, 2017

@sjindel-google I don't see why I'm using it wrong.
For the following code:

typedef T F<T>(); 
F<int> f;

...the type of f is () → dart.core::int.

No type parameters.
That's the point - we need to infer the value of the type argument T of F<T> by the instantiated FunctionType.

image

@scheglov scheglov reopened this Oct 18, 2017
@scheglov
Copy link
Contributor

Thank you, you're right, F<T> has FunctionType type () → T.
And we can unify them.

@kmillikin kmillikin added legacy-area-front-end Legacy: Use area-dart-model instead. front-end-kernel and removed legacy-area-front-end Legacy: Use area-dart-model instead. labels Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead.
Projects
None yet
Development

No branches or pull requests

5 participants