strong mode's return type inference breaks in analysis_server

[jmesserly]

Ironically, this fails in our own type:

abstract class CoercionInfo extends StaticInfo {
  // ...
  toErrorCode() => new HintCode(name, message);

Invalid override. The type of CoercionInfo.toErrorCode (() → dynamic) is not a subtype of StaticInfo.toErrorCode (() → ErrorCode). lib/src/task/strong/info.dart, line 70

The error doesn't appear right away--it seems caused by my editing files. My guess is we're losing the "implicit return type" flag, since it's not tracked in the type system.

[jmesserly]

CC @leafpetersen

I haven't looked into this but a few ideas: Analyzer's element model already has a few special types that behave like dynamic (e.g. CircularTypeImpl), but otherwise preserve additional info, so we could make one for implicit dynamic that is distinct from explicit dynamic. Another option would be to always check in the AST whether a type is implicit (the AST should let us verify the absence of a TypeName in a reliable fashion.)

[jmesserly]

BTW, that's just speculation on my part. It could be that something totally different is going wrong.

[bwilkerson]

Another option would be to always check in the AST whether a type is implicit (the AST should let us verify the absence of a TypeName in a reliable fashion.)

We discard AST structures, so there's no guarantee that we'd have an AST to look at when we need the information. That's why we added the flag to the element mode. If it turns out that the flag is not being preserved, that's a bug we need to fix.

[jmesserly]

Hmmm, is that the case? It looks like we have the AST available to us in the InferInstanceMembersInUnitTask task:

void internalPerform() {
    //
    // Prepare inputs.
    //
    CompilationUnit unit = getRequiredInput(UNIT_INPUT);
    TypeProvider typeProvider = getRequiredInput(TYPE_PROVIDER_INPUT);
    //
    // Infer instance members.
    //
    if (context.analysisOptions.strongMode) {
      InstanceMemberInferrer inferrer = new InstanceMemberInferrer(typeProvider,
          typeSystem: context.typeSystem);
      inferrer.inferCompilationUnit(unit.element);
    }
    //
    // Record outputs.
    //
    outputs[RESOLVED_UNIT9] = unit;
  }

We aren't using it yet (instead we get the .element) but we could, right?

[bwilkerson]

Yes. If you've explicitly asked for it, then it won't go away. The problem arises if you were using Element.computeNode().

[leafpetersen]

In general I don't think we've paid enough attention to making sure that we're working correctly with the analyzer's incremental resolution mode. My guess here would be that there might be a missing dependency in InferInstanceMembersTask for the incremental case, or else that whatever is driving the incremental resolution is not set up to recompute all of the right things for us.

[jmesserly]

ah, that makes sense. So it's possible we're skipping the task entirely. That would definitely explain this behavior.

[jmesserly]

Not much figured out yet, but a few notes so far:

I can break when we're about to generate the invalid method override. hasImplicitReturnType is true, and the element looks fine, other than having a dynamic as its return type

From the analysis_server logs, it appears to be running both steps in the right order for this compilation unit. I see:

1449709583798:Task:/Users/jmesserly/src/dart/sdk/pkg/analyzer:InferInstanceMembersInUnitTask for source /Users/jmesserly/src/dart/sdk/pkg/analyzer/lib/src/task/strong/info.dart 1449709588725:Task:/Users/jmesserly/src/dart/sdk/pkg/analyzer:StrongModeVerifyUnitTask for source /Users/jmesserly/src/dart/sdk/pkg/analyzer/lib/src/task/strong/info.dart

All of the tasks we ran on info.dart look fine and seem to have been run in the correct order. The first one being BuildDirectiveElementsTask. (I wouldn't have necessarily expected it to start there.)

[jmesserly]

progress! I've got a simple, debuggable, fast iteration repro in dart_test.dart:

  void test_library_cycle_override_inference_incremental() {
    enableStrongMode();
    Source lib1Source = newSource(
        '/my_lib1.dart',
        '''
library my_lib1;
import 'my_lib3.dart';
''');
    Source lib2Source = newSource(
        '/my_lib2.dart',
        '''
library my_lib2;
import 'my_lib1.dart';
''');
    Source lib3Source = newSource(
        '/my_lib3.dart',
        '''
library my_lib3;
import 'my_lib2.dart';

class A {
  int foo(int x) => null;
}
class B extends A {
  foo(x) => null;
}
''');
    AnalysisTarget lib1Target = new LibrarySpecificUnit(lib1Source, lib1Source);
    AnalysisTarget lib2Target = new LibrarySpecificUnit(lib2Source, lib2Source);
    AnalysisTarget lib3Target = new LibrarySpecificUnit(lib3Source, lib3Source);

    computeResult(lib1Target, RESOLVED_UNIT);
    computeResult(lib2Target, RESOLVED_UNIT);
    computeResult(lib3Target, RESOLVED_UNIT);
    CompilationUnit unit = outputs[RESOLVED_UNIT];
    ClassElement b = unit.declarations[1].element;
    expect(b.getMethod('foo').returnType.toString(), 'int');

    // add a dummy edit.
    context.setContents(
        lib1Source,
        '''
library my_lib1;
import 'my_lib3.dart';
var foo = 123;
''');

    computeResult(lib1Target, RESOLVED_UNIT);
    computeResult(lib2Target, RESOLVED_UNIT);
    computeResult(lib3Target, RESOLVED_UNIT);
    unit = outputs[RESOLVED_UNIT];
    b = unit.declarations[1].element;
    expect(b.getMethod('foo').returnType.toString(), 'int',
        reason: 'edit should not affect member inference');
  }

fails:

FAIL: ComputeLibraryCycleTaskTest | test_library_cycle_override_inference_incremental
  Expected: 'int'
    Actual: 'dynamic'
     Which: is different.
  Expected: int
    Actual: dynamic
            ^
   Differ at offset 0
  edit should not affect member inference

[jmesserly]

Bingo. We aren't clearing the ClassElement.hasBeenInferred flag, but we are mutating the original MethodElementImpl (same ID) to have "dynamic" again as its return and parameter types. I'll see if I can figure out where the mutation is happening & if there's a logical place to update the hasBeenInferred flag.

I'm not entirely sure why the hasBeenInferred flag exists, as we're already processing this inference on a per-compilation unit level. So I'm not sure how we'd see the same type twice (except it cases like this, when we need to re-resolve). Maybe I can figure it out from the changelog.

[jmesserly]

Ah, it goes back to the original CL. Which used to have this comment: https://codereview.chromium.org/1306313002/diff2/20001:40001/pkg/analyzer/lib/src/task/strong_mode.dart

The comment is gone now, so I'm not sure if the original concern is gone as well? @bwilkerson @leafpetersen do y'all have any recollection of this? I'll keep digging in the meantime

[jmesserly]

FYI -- removing that flag fixes the bug & all existing tests pass.

[jmesserly]

Ah ... the issue is we'll do extra work because we're jumping up and inferring the supertype & mixins & interfaces. That's a bit sketchy. Hmmm.

[jmesserly]

https://codereview.chromium.org/1513143004/

[bwilkerson]

Ah ... the issue is we'll do extra work because we're jumping up and inferring the supertype & mixins & interfaces. That's a bit sketchy.

I don't know why the comment is gone. I think the concern is still valid, and you just ran into the "potential" problem it was referring to. It is indeed sketchy to store this kind of state in the element model, but we do need to prevent duplicated work. We need a better solution...