Analyzer instantiate to bounds for typedefs is inconsistent

[leafpetersen]

The following program shows that the analyzer is mostly (but not entirely) not doing instantiate to bounds on typedefs. The assignments in the body of main cause errors which print out the types and show that the inferred type of globalInferred is correctly instantiated to bounds, but the annotated type on global is not. Most other uses of typedefs don't seem to be instantiated to bounds.

This is mostly being hidden right now since fuzzy arrows will allow the assignment to exist as an implicit downcast. If I disable fuzzy arrows, then the assignment of global to globalInferred gives a type error!

I can work around this in the short term, but this is fairly high priority since it will block us from turning off fuzzy arrows entirely, and is also probably causing some spurious fuzzy arrow warnings.

typedef T Foo<T extends int>(T x);

Foo global;
// With fuzzy arrows disabled, gives an error 
var globalInferred = global;

void main () {
  int causeError;
  // Prints type of global as dynamic -> dynamic
  causeError = global;
  // Prints type of globalInferred as int -> int
  causeError = globalInferred;
}

cc @stereotype441 @scheglov @devoncarew

cc @stefantsov to verify that the front end is doing the right thing here

cc @srawlins

[chloestefantsova]

Looks like the common front end handles this situation correctly:

file:///tmp/fuzzy.dart:10:16: Error: A value of type '(dart.core::int) → dart.core::int' can't be assigned to a variable of type 'dart.core::int'.
Try changing the type of the left hand side, or casting the right hand side to 'dart.core::int'.
  causeError = global;
               ^
file:///tmp/fuzzy.dart:12:16: Error: A value of type '(dart.core::int) → dart.core::int' can't be assigned to a variable of type 'dart.core::int'.
Try changing the type of the left hand side, or casting the right hand side to 'dart.core::int'.
  causeError = globalInferred;
               ^
file:///tmp/fuzzy.dart:10:16: Error: A value of type '(dart.core::int) → dart.core::int' can't be assigned to a variable of type 'dart.core::int'.
Try changing the type of the left hand side, or casting the right hand side to 'dart.core::int'.
  causeError = global;
               ^
file:///tmp/fuzzy.dart:12:16: Error: A value of type '(dart.core::int) → dart.core::int' can't be assigned to a variable of type 'dart.core::int'.
Try changing the type of the left hand side, or casting the right hand side to 'dart.core::int'.
  causeError = globalInferred;
               ^

[leafpetersen]

Looks like the common front end handles this situation correctly:

👍

[leafpetersen]

@devoncarew Not sure what milestone to call this, but this may be a blocker for cleaning up internal code.

[leafpetersen]

@scheglov @bwilkerson Any thoughts on how much work this would be to fix? The impact on internal code isn't huge, but the spurious errors are very perplexing. If necessary I can probably fix up internal code to work around this, but it's not great.

[bwilkerson]

No, sorry. I'll try to dig into it a bit tomorrow.

[bwilkerson]

Just to keep you updated...

I'm not very familiar with the type system anymore, so it's being quite a challenge to figure this out, but I think that at least part of the problem is the use of GenericTypeAliasElementImpl.typeAfterSubstitution. It's currently used in four places (in three methods).

For example, two of the uses are in TypeNameResolver.resolveTypeName. Without changes, the type name Foo in the declaration of global has type (dynamic) -> dynamic. But if I replace the if statement on 8209 with just the else body (type = typeSystem.instantiateToBounds(type);), then the type name has the type (int) -> int. Replacing the other two uses allows me to get the test case above working.

However, when I replace all uses with either instantiateType or instantiateToBounds, I end up with close to a hundred test failures in other tests. So there's some other factor that I'm not thinking of that requires the old behavior some times. I'm going to take a break from looking at it.

[leafpetersen]

One possible clue is that I believe that the Driver path implements this correctly (or more correctly).

In checker_test.dart, the test

test_fuzzyArrowLegacyAssignability_GlobalInference()

passes with the driver, but not otherwise.

If you pare down that test, you might be able to isolate what the driver is doing, which might tell you what the other code paths should be doing?

[bwilkerson]

@scheglov Any ideas about what might be different between Driver and task model for this case?

[scheglov]

So far I found that we perform instantiateToBounds during linking summaries when compute implicit type. I guess we don't do this for explicit types.

[bwilkerson]

I would have guessed that explicit types would have already been instantiated to bounds before the summary is generated. Is that not the case?

In any case, I think that linking summaries is something we do in the driver case (where types are being computed correctly), but not in the task model, so a bug in summary linking wouldn't explain the problem we're seeing in the task model. Right?

[scheglov]

I would have guessed that explicit types would have already been instantiated to bounds before the summary is generated. Is that not the case?

Hm...
Yes, we probably should store all types in summaries.

In any case, I think that linking summaries is something we do in the driver case (where types are being computed correctly), but not in the task model, so a bug in summary linking wouldn't explain the problem we're seeing in the task model. Right?

Well, I does not look that even driver case is correct - Foo global is still inferred as Foo<dynamic>, but var globalInferred = global as Foo<int>.

[bwilkerson]

@leafpetersen

Any thoughts on how much work this would be to fix?

It looks like we would need to fix both the Driver and the task model implementations, and it seems likely that doing so would require two independent fixes. Given that, and our current lack of understanding of where and how this needs to happen, I suspect that we're looking at a fairly significant effort to try to fix it. If I had to guess (which is all it would be given my lack of familiarity with this code) I'd probably estimate a full week.

[leafpetersen]

I'm not sure whether or not this needs to be fixed in the task model given other conversations, but I'm worried about not fixing it in the driver given our current plans to keep this around for multiple quarters. It's hard to know without knowing scope, but the symptoms of this bug are really inexplicable to users.

[leafpetersen]

cc @srawlins who hit this internally.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/46381

[vsmenon]

It looks like we're still hitting this. I'm seeing it internally in the presence of summaries and exports. If I compile the below file by file (i.e., 3 build steps), I get:

[error] Invalid override. The type of 'B.func' ('() → (dynamic) → dynamic') isn't a subtype of 'A<O>.func' ('() → (O) → O'). (override.dart, line 5, col 3)

Since's Func's bound is O, B.func should not be () → (dynamic) → dynamic.

override3.dart:

class O {
}

typedef T Func<T extends O>(T e);

override2.dart:

import 'override3.dart';

export 'override3.dart' show Func;

abstract class A<T extends O> {
  Func<T> get func;
}

override.dart:

import 'override2.dart';

class B extends A {
  Func get func => (x) => x;
}

void main() {
  print(new B().func);
}

[vsmenon]

@bwilkerson @scheglov - i'm hitting this internally while fixing runtime type errors in user code.

[MichaelRFairhurst]

Wasn't able to repro this via unit test:

  void test_issue32114() {                                                       
    addNamedSource('/a.dart', '''                                                
class O {}                                                                       
                                                                                 
typedef T Func<T extends O>(T e);                                                
''');                                                                            
    var bundleB = createPackageBundle('''                                        
import 'a.dart';                                                                 
export 'a.dart' show Func;                                                       
                                                                                 
abstract class A<T extends O> {                                                  
  Func<T> get func;                                                              
}                                                                                
''', path: '/b.dart');                                                           
    addBundle('/b.ds', bundleB);                                                 
    createLinker('''                                                             
import 'b.dart';                                                                 
                                                                                 
class B extends A {                                                              
  Func get func => (x) => x;                                                     
}                                                                                
''');                                                                            
    LibraryElementForLink library = linker.getLibrary(linkerInputs.testDartUri); 
    library.libraryCycleForLink.ensureLinked();                                  
    ClassElementForLink_Class cls = library.getContainedName('B');               
    expect(cls.accessors, hasLength(1));                                         
    expect(cls.accessors[0].type.toString(), '() → (O) → O');                    
  }   

Will have to dig a bit deeper here.

[MichaelRFairhurst]

Also could not repro using bazel, reached out to @vsmenon over email on tips to repro

[leafpetersen]

Do the repro instructions here: #32114 (comment) not work any more?

[MichaelRFairhurst]

Its likely I'm not "compiling it file by file" the same way. But I tried two means already and neither worked

[leafpetersen]

You can reproduce as follows:

leafp-macbookpro:sdk leafp$ /Users/leafp/src/dart-repo/sdk/sdk/bin/dartdevc --modules=node --library-root=/Users/leafp/tmp/intellij-test-directory --dart-sdk-summary=/Users/leafp/src/dart-repo/sdk/xcodebuild/ReleaseX64/gen/utils/dartdevc/ddc_sdk.sum -o override2.sum /Users/leafp/tmp/intellij-test-directory/override2.dart
leafp-macbookpro:sdk leafp$ /Users/leafp/src/dart-repo/sdk/sdk/bin/dartdevc --modules=node --library-root=/Users/leafp/tmp/intellij-test-directory --dart-sdk-summary=/Users/leafp/src/dart-repo/sdk/xcodebuild/ReleaseX64/gen/utils/dartdevc/ddc_sdk.sum -o override1.sum /Users/leafp/tmp/intellij-test-directory/override1.dart -s override2.sum
leafp-macbookpro:sdk leafp$ /Users/leafp/src/dart-repo/sdk/sdk/bin/dartdevc --modules=node --library-root=/Users/leafp/tmp/intellij-test-directory --dart-sdk-summary=/Users/leafp/src/dart-repo/sdk/xcodebuild/ReleaseX64/gen/utils/dartdevc/ddc_sdk.sum -o out.js /Users/leafp/tmp/intellij-test-directory/override0.dart -s override2.sum -s override1.sum
[error] Invalid override. The type of 'B.func' ('() → (dynamic) → dynamic') isn't a subtype of 'A<O>.func' ('() → (O) → O'). (../../../tmp/intellij-test-directory/override0.dart, line 4, col 3)

Please fix all errors before compiling (warnings are okay).

[MichaelRFairhurst]

Thanks Leaf! I am not familiar enough with ddc apparently.

I suspect this is a task model only bug, and that we won't need to fix, since I can't repro it with tools that are on the driver model. I will confirm that.

[leafpetersen]

I thought that summaries were not driven by the task model, and that seems to be the key bit here?

[leafpetersen]

Any updates on this?

[JekCharlsonYu]

any updates on this?

[MichaelRFairhurst]

I have 4 test failures on a solution to this which I'm working through. It's a small number but don't read too much into that -- my last solution to those 4 failures introduced ~80 new ones :) I rolled that back and I'm trying a different approach, but it might have been the right one and I may end up going back through that path.

[MichaelRFairhurst]

Looks like those four failures were do to me being overly defensive in my code. Now I'm just trying to get a clean repro in a unit test, not only to make the CL clean + future proof it, but also to be able to have test coverage around the changes I'm making. It doesn't seem to want to be exposed via unit test. Currently mucking around in the debugger to see why I can't get it down the pathway that is fairly obviously wrong once it gets there.

[MichaelRFairhurst]

Fixed in https://dart-review.googlesource.com/c/sdk/+/62483

[srawlins]

Woohoo thanks @MichaelRFairhurst this was a doozy!