Analyzer summaries missing generic types for DartObject

[matanlurey]

We have a class called MultiToken and another called ExistingProvider:

https://github.com/dart-lang/angular/blob/1abcb5c62fc8a8267f1abdb2081d07f40567c56f/angular/lib/src/core/di/opaque_token.dart#L62-L74

https://github.com/dart-lang/angular/blob/1abcb5c62fc8a8267f1abdb2081d07f40567c56f/angular/lib/src/di/providers.dart#L241-L265

This pattern was created in order to correctly "infer" what the type of a given DI provider should be, but also not require manual type annotations (use flowing type inference from OpaqueToken<T> -> ExistingProvider<T> -> Provider<T> (T):

const NG_VALUE_ACCESSOR = const MultiToken<ControlValueAccessor>(
  'NgValueAccessor',
);

const DEFAULT_VALUE_ACCESSOR = const ExistingProvider.forToken(
  NG_VALUE_ACCESSOR,
  DefaultValueAccessor,
);

When analyzing this during our compilation, T of DEFAULT_VALUE_ACCESSOR is dynamic internally (with bazel_codegen) and (correctly) ControlValueAccessor externally (with build_runner). This is causing our sync to fail on travis:

https://api.travis-ci.org/v3/job/342583283/log.txt

If I manually type (don't rely on inference), it works as intended in both places:

const DEFAULT_VALUE_ACCESSOR = const ExistingProvider<ControlValueAccessor>.forToken(
  NG_VALUE_ACCESSOR,
  DefaultValueAccessor,
);

It's also possible this is due to different analyzer versions/configuration? I know top-level inference is inconsistent without using Dart 2 semantics, but I'm very confused why this works properly externally but not internally.

/cc @natebosch @scheglov @alorenzen

[natebosch]

Both places are using an AnalysisContext. Both places set strongMode to true and no other analysis options. (bazel, build_runner)

In bazel we use summaries for files outside the current package, in build_runner we always read Dart sources, never summaries.

[matanlurey]

Interestingly enough, this worked internally a while back :-/

[natebosch]

Could be a regression in the AnalysisContext in that case - I think we're likely getting older versions of the analyzer package externally than what is rolled internally. If you can build a test case you might want to see if you can try against older versions of analyzer internally.

[matanlurey]

For folks reading this thread:

INTERNALLY = 0.31.2-alpha.0 EXTERNALLY = ^0.31.0+1

[MichaelRFairhurst]

My understanding was that top levels don't have inference, they get implicit dynamic. It seems like for const values we do some kind of inference, though...

We did just change const values around a bit in order to support asserts in initializers?

[matanlurey]

@MichaelRFairhurst:

My understanding was that top levels don't have inference, they get implicit dynamic. It seems like for const values we do some kind of inference, though...

Yeah, it works as of 0.31.0+1 for this limited case. If I'm relying on something "bad", let me know, but my assumption is that it should continue to work indefinitely.

If you need a better reproduction case, I'm happy to write one.

[MichaelRFairhurst]

Its my understanding that this does not have any inference...const ExistingProvider.forToken(...) but rather gets implicit dynamic...I think that's the problem

[matanlurey]

@MichaelRFairhurst:

Its a little hard to see here, but the failing travis sees it:

https://api.travis-ci.org/v3/job/342583283/log.txt

     Which: is different.
            Expected: ... ;\n  List<dynamic> _ ...
              Actual: ... ;\n  List<import15.C ...
                                    ^
             Differ at offset 8427
  ./build/test/_files/directives/directive_wrapper.template_debug.golden is out of date. See ./build/test/_files/directives/directive_wrapper.template_debug.check.

i.e. externally, we read this provider as T=ControlValueAccessor, but internally it is read as T=dynamic. We obviously want the former.

EDIT: I will add that one of these uses summaries of dependencies (internally, with Bazel) and one of these does not (externally, with Build Runner). That may or may not matter?

[matanlurey]

In order to avoid this blocking the sync, I'll add a work-around for now @alorenzen.

[matanlurey]

Ping @MichaelRFairhurst @scheglov could I get some help?

I added a regression test here: https://github.com/dart-lang/angular/blob/master/_tests/test/regression/908_analyzer_inference_test.dart

The skipped case passes externally, but fails internally.

[leafpetersen]

This should almost certainly be inferred. We do toplevel inference, and this looks like a straightforward example of something we should catch, unless I'm missing something. There are a lot of known bugs and unimplemented bits in the analyzer (since it was intended to only be implemented in the new front end), but we certainly shouldn't be regressing here.

Unfortunately this is sometimes very sensitive to multiple files and analysis order, but if there's any way you can get a small repro that would be great. Also, file an issue in the sdk if you haven't already.

[matanlurey]

@leafpetersen I tried implementing the simplest repro possible:

import 'provider.dart';
import 'token.dart';

const tokenOfString = const Token<String>();
const implicitProviderOfString = const Provider(tokenOfString);
const explicitProviderOfString = const Provider<String>(tokenOfString);

@TestOn('vm')
import 'package:analyzer/dart/constant/value.dart';
import 'package:build/build.dart';
import 'package:build_test/build_test.dart';
import 'package:test/test.dart';

void main() {
  final asset = new AssetId('top_level_analyzer', 'lib/library.dart');

  DartObject explicitProviderOfString;
  DartObject implicitProviderOfString;

  setUpAll(() async {
    final library = await resolveAsset(asset, (r) => r.libraryFor(asset));
    final fields = library.definingCompilationUnit.topLevelVariables;;
    explicitProviderOfString = fields[1].computeConstantValue();
    implicitProviderOfString = fields[2].computeConstantValue();
  });

  test('should resolve explicitProviderOfString', () async {
    expect(explicitProviderOfString.type.toString(), 'Provider<String>');
  });

  test('should resolve implicitProviderOfString', () async {
    expect(implicitProviderOfString.type.toString(), 'Provider<String>');
  });
}

... and this works externally, just fine, with both:

• analyzer: 0.31.1
• analyzer: 0.31.2-alpha.0

... so now I'm thinking its related to summaries (Bazel uses). I'll try and write the same test internally and see if I get different results tomorrow, I hope I can figure something out.

[matanlurey]

@stereotype441 took a look at this.

I'm going to port the above test internally, where it fails, and then share with the team to see if we can make some progress. Update a little later today.

[matanlurey]

From what I can tell... this works fine internally, too.

It's likely something very subtle and not directly related to the analyzer, or build, but I really appreciate everyone looking at this and trying to help diagnose the problem(s). I'll ping again if I have a fix or further questions.

[matanlurey]

I'm closing this, as we are just landing a workaround instead.

[matanlurey]

Re-opening. Even with the workaround... sometimes it doesn't work :-/

[matanlurey]

This test fails internally when DDC enforces type checks for List:

https://github.com/dart-lang/angular/blob/master/_tests/test/core/linker/deferred_component_ngmodel_test.dart

... it shouldn't.

[stereotype441]

I believe the underlying cause is https://github.com/dart-lang/sdk/issues/32290.

[matanlurey]

Wow! Thanks @stereotype441!

I think I can add a workaround, again, until we have a fix.