Skip to content

Analyzer summary type logic doesn't do downward type inference #32525

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.

Sign up for GitHub

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

Jump to bottom
Closed
stereotype441 opened this issue Mar 14, 2018 · 4 comments
Closed

Analyzer summary type logic doesn't do downward type inference #32525

stereotype441 opened this issue Mar 14, 2018 · 4 comments
Assignees
@stereotype441
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
Dart2 Beta 3

Comments

@stereotype441
Copy link
Member

Consider the following code:

T f<T>(T x) => x;
var x = [1].map(f);

The correct type to infer for x is Iterable<int>. As of 28cdeb3, the analyzer summary logic infers Iterable<dynamic>. The reason this happens is that the analyzer summary logic doesn't do downward inference at all; it only does upward inference. So the type of f in map(f) is assumed to be (dynamic) -> dynamic, and therefore the return type of the map call is inferred to be Iterable<dynamic>.
@stereotype441 stereotype441 added P1 A high priority bug; for example, a single project is unusable or has many test failures legacy-area-analyzer Use area-devexp instead. labels Mar 14, 2018
@stereotype441 stereotype441 self-assigned this Mar 14, 2018
@leafpetersen leafpetersen mentioned this issue Mar 14, 2018
Closed
32 tasks
@stereotype441
Copy link
Member Author

Unfortunately this is going to be non-trivial to fix because the summary representation of expressions is a postfix representation. In order to push type information both upwards and downwards we'll need to deserialize to a temporary tree representation. Hopefully we can use analyzer ASTs as the temporary representation, so that we can re-use as much of the existing type inference logic as possible. I'll start looking into this today.

This was referenced Mar 14, 2018
Closed
Closed
@dgrove dgrove added this to the Dart2 Beta 3 milestone Mar 22, 2018
dart-bot pushed a commit that referenced this issue Mar 24, 2018
@stereotype441
Allow type propagation to be disabled in StaticTypeAnalyzer.
ca47fd7 
I intend to re-use StaticTypeAnalyzer for performing type inference
during summary linking as part of fixing #32525.  Allowing type
propagation to be disabled will allow the linker to do less work, and
avoid the need to implement the type-propagation-related element
methods in the linker's reduced element model.

There should be no functional change with this CL, since
StaticTypeAnalyzer is not used in linking yet.

Change-Id: I5f644e9869ab89dee21b03a3a54bf809926142f5
Reviewed-on: https://dart-review.googlesource.com/48082
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
@stereotype441
Copy link
Member Author

This was accidentally auto-closed due to the phrase "as part of fixing #X" being misinterpreted by the auto-closer as meaning that the commit fixed the bug.

@stereotype441 stereotype441 reopened this Mar 24, 2018
@stereotype441 stereotype441 mentioned this issue Mar 26, 2018
Closed
@stereotype441
Copy link
Member Author

In order to complete this we're going to need to fill in the rest of the implementation of ConstExprBuilder so that it can handle the types of subexpressions that appear outside of constants. I've filed #32674 to track this.

dart-bot pushed a commit that referenced this issue Mar 26, 2018
@stereotype441
More refactoring in preparation for #32525
af4ec9b 
This CL makes the following changes:

- Extracts the implementation of ClassElementImpl.getNamedConstructor
  to a static method so that it can be re-used by
  ClassElementForLink_Class.

- Adds a UnitResynthesizerMixin class so that unit resynthesis methods
  can be shared between the resynthesizer and the linker.

Change-Id: I694d6fef511b5bb734ea78095834d6c68e22bf63
Reviewed-on: https://dart-review.googlesource.com/48380
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
dart-bot pushed a commit that referenced this issue Mar 27, 2018
@scheglov
Resynthesize more expressions in ExprBuilder.
28c4de8 
One thing that occurred to me is that maybe we don't need precise
resynthesis of some expressions, e.g. `a is T`, because it does not
have dependencies, and has known type. Thoughts?

We already kind of went this way by not serializing arguments of
invocations with type arguments, and cascades.

OTOH, it might be simplier to resynthesize where it is easy.

R=brianwilkerson@google.com, paulberry@google.com

Bug: #32525
Change-Id: I742b1adc6ae22d8743ef127af7d6a33446e53449
Reviewed-on: https://dart-review.googlesource.com/48466
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
dart-bot pushed a commit that referenced this issue Mar 28, 2018
@scheglov
Fix for building sequence of identifiers and several tests.
0e8457c 
R=brianwilkerson@google.com, paulberry@google.com

Bug: #32525
Change-Id: Iddc0a4825e98bf186e6c3da0fd24c81a95b96522
Reviewed-on: https://dart-review.googlesource.com/48501
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
dart-bot pushed a commit that referenced this issue Mar 28, 2018
@stereotype441
More prep work to allow the linker to re-use resolution logic.
ebdee7e 
See #32525.

This CL makes the following changes:

- Extracts the logic from AbstractClassElementImpl.getSetter to a
  static method so it can be re-used by the summary linker.

- Modifies ParameterElementImpl._resynthesizeTypeAndParameters so that
  it avoids unnecessary calls to resolveLinkedType.  This is crucial
  because resolveLinkedType is not available when linking.

- Makes it possible to suppress reporting of const evaluation errors
  when doing resolution.  This is necessary because in order to report
  these errors we must do constant evaluation, which is not possible
  during summary linking.

- Modifies ResolverVisitor._hasSerializedConstantInitializer to avoid
  unnecessary calls to LibraryElementImpl.hasResolutionCapability.
  This is crucial because the linker contains an implementation of
  LibraryElementImpl that's incompatible with
  LibraryElementImpl.hasResolutionCapability.

Change-Id: Iec91bd8d6a68193e091c2438bafcd7cb15488d89
Reviewed-on: https://dart-review.googlesource.com/48681
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
@stereotype441
Copy link
Member Author

Fix out for review: https://dart-review.googlesource.com/c/sdk/+/48741/1..2

dart-bot pushed a commit that referenced this issue Mar 29, 2018
@stereotype441
Further prep work for #32525.
494b861 
This CL refactors the bodies of the following methods so that they can
be re-used by the summary linker:

- CompilationUnitElementImpl.getType
- LibraryElementImpl.createLoadLibraryFunction
- LibraryElementImpl.getImportsWithPrefix
- LibraryElementImpl.getType

Change-Id: I9b398d87323eb9ec6deb9a9a76bd6e9a1aa6bb76
Reviewed-on: https://dart-review.googlesource.com/48740
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stereotype441 stereotype441

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
Milestone
Dart2 Beta 3
Development

No branches or pull requests
2 participants
@stereotype441 @dgrove