Skip to content
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

Analyzer: Issues with inherited parameter types that include generic function types #31804

Open
MichaelRFairhurst opened this issue Jan 9, 2018 · 2 comments
Labels
analyzer-api Issues that impact the public API of the analyzer package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@MichaelRFairhurst
Copy link
Contributor

MichaelRFairhurst commented Jan 9, 2018

When inheriting parameter types ie:

class Base {
  void f(int x);
}

class Sub extends Base {
  f(x); // type of `x` inherited from `Base.f`
}

The inherited type may be a generic function. This leads to confusion in the element model because the enclosing type parameter contexts are assumed to be a discoverable via enclosingElement.

This means, given

class Base {
  void f(void Function<T>(T) x);
}

class Sub extends Base {
  f(x); // the type of `x` inherited from `Base.f`
}

the FunctionType from x must get a new synthetic Element so that its parameter T can be enclosed within the Sub class.

This is currently working in the above example, but not in more complex examples such as:

  void f(List<void Function<T>(T)> x);

in which case the FunctionTypeElement is not copied into a synthetic when f is overriden without a parameter type annotation, leading to a crash at deserialization time from summaries.

This may also affect other cases:

typedef F = Function<T>();
...
  void f(List<F> fs);
// or
  void f(Function(Function<T>()) x);

etc.

@MichaelRFairhurst MichaelRFairhurst added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on bug labels Jan 9, 2018
@MichaelRFairhurst MichaelRFairhurst self-assigned this Jan 9, 2018
@eernstg
Copy link
Member

eernstg commented Jan 9, 2018

Maybe there are a couple of typos?: class Sub extends/implements Base.. and void f(List<void Function<T>(T)> x).

@MichaelRFairhurst
Copy link
Contributor Author

Yep thanks for the corrections Erik!

whesse pushed a commit that referenced this issue Jan 10, 2018
This is based on https://dart-review.googlesource.com/c/sdk/+/20481/10,
which required a rollback due to numerous secondary revealed issues.

Rather than deserializing function types with the proper scope, and
breaking inherited function type params, ie, `m(x)` where `x`
implicitly gets the type of `x` from `super.m(... x)`, deserialize
generic types incorrectly so that `const x = <Function<T>(T)>[]` fails
but `m(x)` works.

I have work towards fixing this issue, but it is incomplete (#31804),
and had to be rolled back and the fix is only getting more tangled:
https://dart-review.googlesource.com/c/sdk/+/33582.

For now, this will allow people to use Function types *without* type
parameters with no issues, and introduce no secondary issues, and
hopefully be a more straightforward change in and of itself.

-- From Rolled Back CL --

Remove field formal parameter test from other branch


Test genericFunctionType summarization deeper: params, return.

Didn't see a standard way of testing 'void' in the test so far, so I
followed some other tests and used isNotNull, which could be improved.

Fix type arguments vs parameters.

Fix #30858, generic function types not serializable when const.

Previously declaring for instance const lists (or even final lists) of
generic function types ie `final x = <void Function()>[]` would throw
errors during summarization.

The type parameters don't seem correctly stored yet, but that is an
edge case. Left a TODO, for now this should go in to prevent the
analyzer crash.

One thing I changed as well is that `serializeType` assumed it was
getting a type name, and `serializeTypeName` accepted types but threw
when it didn't get a type name. I flipped the names so that
`serializeTypeName` accepts a type name and `serializeType` accepts a
type in general and decides whether its a type name or a generic
function type to proceed with specialization.

Bug:
Change-Id: I98f5a362c9b2e8b3a7e87532ff4645b5963277ab
Reviewed-on: https://dart-review.googlesource.com/33660
Commit-Queue: Mike Fairhurst <mfairhurst@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
whesse pushed a commit that referenced this issue Jan 11, 2018
solves the generic function type scoping problem (fixes the test).

Includes further failures documented here:
#31804 with @failingTests.

These do seem to fail today as is, so it should be safe to land.

Bug:
Change-Id: Ice384b6fee35f1b1c4235bb0e4de7a90e2379937
Reviewed-on: https://dart-review.googlesource.com/33582
Commit-Queue: Mike Fairhurst <mfairhurst@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
@lrhn lrhn added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed bug labels Jan 17, 2018
@bwilkerson bwilkerson added this to the Dart2.1 milestone Sep 2, 2018
@bwilkerson bwilkerson modified the milestones: Dart2.1, PostDart2.1 Sep 4, 2018
@aadilmaan aadilmaan modified the milestones: Future, D25 Release Jun 4, 2019
@srawlins srawlins added the analyzer-api Issues that impact the public API of the analyzer package label Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-api Issues that impact the public API of the analyzer package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

7 participants