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

InferStaticVariableTypeTask does not pass in an error reporter #27053

Closed
jmesserly opened this issue Aug 11, 2016 · 6 comments
Closed

InferStaticVariableTypeTask does not pass in an error reporter #27053

jmesserly opened this issue Aug 11, 2016 · 6 comments
Assignees
Labels
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

@jmesserly
Copy link

jmesserly commented Aug 11, 2016

this means we lose a lot of AnalysisErrors that are produced while the initializer is being resolved

for example:

var x = <String>[].map((x) => "");

will not produce a hint about the inferred closure type

here's the code from InferStaticVariableTypeTask:

      ResolutionContext resolutionContext = ResolutionContextBuilder.contextFor(
          initializer, AnalysisErrorListener.NULL_LISTENER);
      ResolverVisitor visitor = new ResolverVisitor(variable.library,
          variable.source, typeProvider, AnalysisErrorListener.NULL_LISTENER,
          nameScope: resolutionContext.scope);
@jmesserly jmesserly added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-strong-mode P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Aug 11, 2016
@jmesserly
Copy link
Author

@bwilkerson @scheglov mind helping with this one? I just spent some time on it and I figured out how to add a task output, but the errors would be per-VariableElement I'm not sure how to aggregate them all together in the task model.

@scheglov scheglov self-assigned this Aug 11, 2016
@scheglov
Copy link
Contributor

Yes, of course.
I will do it.

@scheglov
Copy link
Contributor

https://codereview.chromium.org/2239613002

Not quite the same test case though.
So, not sure if this is the errors you were looking for.

@leafpetersen
Copy link
Member

Just a note of caution: the resolution that happens in this pass is not final: the code is in an only partially inferred state, and will also be re-resolved. I think my original thinking in not threading through an error reporter was that any meaningful errors that got produced there would get re-produced when we re-resolved the expression after inference was finalized, and so it was not important to keep the errors (other than the anomaly of not seeing inferred type hints), and in fact might be bad since you might see duplicates and/or incorrect errors.

It may be that this thinking was wrong, but it would be good to understand why, and to be sure that the two things I was worried about above can't happen.

For what it's worth, one of the outcomes of the language meetings this week is a proposal for a simplified global inference story, so hopefully some of this complexity can be reduced.

@jmesserly
Copy link
Author

[...] I think my original thinking in not threading through an error reporter was that any meaningful errors that got produced there would get re-produced when we re-resolved the expression after inference was finalized, and so it was not important to keep the errors [...]

Interesting. Can't say I've personally seen any "re-resolution" when debugging our tests. Do you know when the second resolution should be happening? Maybe I need a couple of top-level vars or libraries that depend on each other in an interesting way?

[...](other than the anomaly of not seeing inferred type hints), and in fact might be bad since you might see duplicates and/or incorrect errors.

Well the problem is, if we want to issue errors from inference (as requested by #26992) then we have to fix this situation, because otherwise the error won't show up. That seems bad.

@scheglov
Copy link
Contributor

296d81f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

3 participants