Skip to content

Silent inconsistency in top level inference #32394

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
2 of 4 tasks
leafpetersen opened this issue Mar 2, 2018 · 11 comments
Closed
2 of 4 tasks

Silent inconsistency in top level inference #32394

leafpetersen opened this issue Mar 2, 2018 · 11 comments
Assignees
Labels
legacy-area-analyzer Use area-devexp instead. P1 A high priority bug; for example, a single project is unusable or has many test failures
Milestone

Comments

@leafpetersen
Copy link
Member

The code below should issue an error on the assignment to p, or should issue a warning that top level inference can not be performed. Currently it seems to work correctly on task model and CFE but not driver.

Passing on:

  • dartanalyzer --strong
  • Intellij (strong)
  • ddc
  • ddc -k
var x = y.map((a) => a.toString());
var y = [3];
var z = x.toList();

void main() {
  String p = z;
}
@leafpetersen leafpetersen added the legacy-area-analyzer Use area-devexp instead. label Mar 2, 2018
@leafpetersen
Copy link
Member Author

@scheglov
Copy link
Contributor

scheglov commented Mar 9, 2018

We disabled these errors in https://codereview.chromium.org/2983293002
Should we restore them?

@leafpetersen
Copy link
Member Author

I don't think so, but I don't really know that the plan here is. What I understood from talking to @devoncarew is that the interim plan was that:

  • Kernel supports top level inference
  • Driver implements a subset of top level inference and warns about missing top level inference
  • Task model goes away

I suspect that restoring those warnings is non-starter, at least as more than hints, since the migration would be prohibitive.

@scheglov
Copy link
Contributor

scheglov commented Mar 9, 2018

Well, this is place where linker does not perform inference.
We probably can add inference for cases when arguments are not closures.

But it is explicitly disabled for function types.

if (Linker._initializerTypeInferenceCycle != null &&
Linker._initializerTypeInferenceCycle ==
compilationUnit.library.libraryCycleForLink) {
// We are currently computing the type of an initializer expression in the
// current library cycle, so type inference results should be ignored.
return _computeDefaultReturnType();
}

@devoncarew
Copy link
Member

I would change 'Task model goes away' to 'Task model doesn't guarantee correctness'.

@leafpetersen, should this be tracked in #32395?

@leafpetersen
Copy link
Member Author

@leafpetersen, should this be tracked in #32395?

It's already there.

@stereotype441
Copy link
Member

There are at least two root causes of this issue:

  1. When summarizing a method invocation, we aren't storing information about the arguments. I have a CL out to fix this: https://dart-review.googlesource.com/c/sdk/+/46301
  2. When doing type inference of a closure appearing inside an initializer expression, we don't appear to be passing the correct context information into the closure.

Once https://dart-review.googlesource.com/c/sdk/+/46301 lands, Leaf's example will give an error message, but it won't be entirely correct--it will say "A value of type 'List' can't be assigned to a variable of type 'String'", whereas the correct message is "A value of type 'List<String>' can't be assigned to a variable of type 'String'". The reason for the incorrect error message is root cause 2, which causes it to infer a type of Iterable for x, instead of the correct type, which is Iterable.

I will start working on the second root cause now.

dart-bot pushed a commit that referenced this issue Mar 13, 2018
…nference.

When summarizing an initializer expression that contains a method
call, we need to store the method arguments in case they are needed
for type inference.  They might be needed if the method in question is
a generic method and no type arguments are provided at the call site.

Since we do serialization based on an unresolved AST, we can't tell if
the method in question is generic at serialization time, so we
serialize the arguments of any method invocation that doesn't specify
explicit type arguments.

Addresses one of the root causes of #32394

Change-Id: Ibb50bdfc5ed29a518f51b13d2ffd72c484ab461d
Reviewed-on: https://dart-review.googlesource.com/46301
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
@stereotype441
Copy link
Member

I'm going to delay further work on this until #32525 is fixed, since fixing #32525 is likely to change the internal representation of expressions used during inference. If we're lucky, the fix for #32525 will be to share enough code with the AST-based type inference mechanism that this bug gets fixed in the process. If we aren't, I'll come back and look at this bug after #32525 is finished.

@devoncarew
Copy link
Member

currently blocked on #32525

@devoncarew
Copy link
Member

And, possibly the fix for #32525 will help here as well.

@stereotype441
Copy link
Member

@bwilkerson bwilkerson added this to the Dart2 Beta 3 milestone Mar 29, 2018
@bwilkerson bwilkerson added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

5 participants