Skip to content

Implement recursive top-level inference #46579

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
scheglov opened this issue Jul 7, 2021 · 8 comments
Closed

Implement recursive top-level inference #46579

scheglov opened this issue Jul 7, 2021 · 8 comments
Assignees
Labels
dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on

Comments

@scheglov
Copy link
Contributor

scheglov commented Jul 7, 2021

Currently in the analyzer we do inference in two phases - first we resolve the initializer presuming all not yet inferred types of fields are dynamic. Then we get dependencies as elements referenced by the initializer, and infer in the topological order.

Type inference in the following code in the analyzer is done incorrectly.

class A {
  static final a = A.b.c;
  static final b = A();
  final c = a;
}

When we resolve the initializer of a, we see that it references b, but its type is presumed to be dynamic, so we don't know that it also references c. So, we miss the cycle a -> c -> a.

We should do something like CFE does, and perform recursive inference.

@scheglov scheglov added legacy-area-analyzer Use area-devexp instead. dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec labels Jul 7, 2021
@scheglov scheglov changed the title Top-level type inference misses dependencies Implement recursive top-level inference Jul 7, 2021
@stereotype441
Copy link
Member

Note: I've just made an update to dart-lang/language#1650 describing in more detail how the analyzer and CFE currently handle top level inference, and discussing options. We may want to wait until a decision has been made on that issue before proceeding with this fix.

@scheglov
Copy link
Contributor Author

scheglov commented Jul 9, 2021

https://dart-review.googlesource.com/c/sdk/+/206421 passes shared tests and smoke tests in google3.
I have not tested it yet on the whole google3 code base.

@scheglov
Copy link
Contributor Author

@scheglov scheglov self-assigned this Jul 10, 2021
@srawlins srawlins added the P2 A bug or feature request we're likely to work on label Jul 21, 2021
@srawlins
Copy link
Member

srawlins commented Nov 1, 2021

Is this still in the works, @scheglov ?

@scheglov
Copy link
Contributor Author

scheglov commented Nov 1, 2021

It is up to the language team to decide here, whether they want to go with this change to inference.
This is tracked in dart-lang/language#1650.

We probably should keep this open.

@scheglov
Copy link
Contributor Author

scheglov commented May 6, 2022

@leafpetersen This is the issue about type inference that I mentioned as related to macro type inference.

@eernstg
Copy link
Member

eernstg commented May 10, 2022

@scheglov wrote:

It is up to the language team to decide here, whether they want to go with this change to inference

I just added a comment to dart-lang/language#1650, noting that there might not be a need to change anything in the spec.

copybara-service bot pushed a commit that referenced this issue Nov 7, 2022
Bug: #46579
Change-Id: Ib2498e17bfb14bed2beadcf2491c98bf18e9d09a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206421
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
@scheglov
Copy link
Contributor Author

The CL landed, so I guess this can be marked fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

4 participants