[breaking change] Report type inference errors in top-level fields

[chloestefantsova]

Intended change in behavior

Both the CFE and the Analyzer don't report the error on the inferred type arguments that don't satisfy the type variable bound constraints. Consider the following program.

class TableContext {}

class Column {}

class TableSchema<F extends Column, C extends TableContext> {
  factory TableSchema({required Iterable<F> fields, C? context}) =>
      new TableSchema._();

  TableSchema._();
}

var schema1 = TableSchema(fields: []); // Here the error is reported only with the applied fix.

void method() {
  var schema2 = TableSchema(fields: []); // The error was reported here before and continues to be reported after the fix is applied.
  // The error message is: Inferred type argument 'dynamic' doesn't conform to the bound 'Column' of the type variable 'F' on 'TableSchema'.
}

Here the type arguments inferred for the initializing expressions of either the schema1 or schema2 are dynamic and TableContext, and the first of those inferred type arguments is not a subtype of the corresponding bound Column of the type variable F.

However, both the CFE and the Analyzer report the error for the initializer of schema2, but not for that of the top-level variable schema1. This proposal suggests reporting the error in all contexts, including the initializers of top-level variables as shown in the example above and other kinds of contexts, for example, initializers of static class fields.

Rationale for making the change

Disallowing programs where some types don't respect their corresponding constraints removes potential soundness issues in the type system. Additionally, the proposed change treats all types that don't satisfy the corresponding bounds uniformly.

Expected impact of this change.

Since both tools are failing to report the erroneous code as a compile-time error, some programs containing top-level fields as described in the motivational example may fail to compile after the change is implemented.

Steps for mitigating the change

One way of adjusting the code that will fail to compile after the change is implemented is to supply the type arguments that satisfy their corresponding bounds in place of the inferred ones. In the example above, the line for schema1 can be corrected as follows:

var schema1 = TableSchema<Column, TableContext>(fields: []);

The line for schema2 can be corrected in a similar way.

[keertip]

@chloestefantsova is there a timeline for this change? What is the priority?

[chloestefantsova]

@chloestefantsova is there a timeline for this change? What is the priority?

I would defer to @leafpetersen and @johnniwinther to determine that.

[keertip]

/cc @leafpetersen

[jcollins-g]

Tagging @leafpetersen and @johnniwinther again for prioritization/timelines. Marking as P2 for now.

[itsjustkevin]

@Hixie @grouma @vsmenon @leafpetersen @johnniwinther could you take a look at this breaking change request?

[grouma]

This seems like a beneficial change. This does not directly impact ACX but it may have some impact to internal customers. Presumably any errors caught will be fairly easy and beneficial to fix. When this is ready to land and roll into Google3 you'll want to give a heads up to @iinozemtsev.

[Hixie]

I don't understand why the inferred type for F isn't Column?

[chloestefantsova]

I don't understand why the inferred type for F isn't Column?

As I understand it, it's a limitation of the constraint generation algorithm currently used by the type inference. It doesn't take the bounds of the type variables into account in some cases, which results in dynamic being inferred in this particular example. Basically, the list literal [] remains unconstrained, which gives <dynamic>[], and that dynamic is then used as the value for F.

[Hixie]

But even if the inference doesn't figure out the type of the list literal, surely there's no point ever inferring a type that isn't valid. Should the types the inference system considers be limited to the types that will fulfill the bounds?

I guess what I'm saying is I would expect the error to be that the List isn't valid for the Iterable<Column> fields parameter to the TableSchema<Column, TableContext> factory constructor.

[chloestefantsova]

Yes, I agree, the error message emitted by the compiler is less helpful than it could have been. The Analyzer provides more context in this case, explaining the logic behind the inference:

  error • Couldn't infer type parameter 'F'.

Tried to infer 'dynamic' for 'F' which doesn't work:
  Type parameter 'F' is declared to extend 'Column' producing 'Column'.
The type 'dynamic' was inferred from:
  Parameter 'fields' declared as     'Iterable<F>'
                     but argument is 'List<dynamic>'.

Consider passing explicit type argument(s) to the generic.

[Hixie]

Ironically while it's one of the most helpful analyzer messages it's also one of my least favourites because it contains newlines and so messes up the layout of many things that process analyzer output. :-)

[johnniwinther]

Change LGTM. Regarding timeline: The CFE has the fix ready, so we can land this ASAP. I don't know how difficult this is to fix in the analyzer.

[leafpetersen]

@srawlins @bwilkerson @scheglov Is the analyzer side of this something we can get done for the second beta for 2.19 (October 5 is the cutoff date)?

[srawlins]

I expect we could do this in September without messing up other priorities too much

[johnniwinther]

@srawlins Can you make this change before October 5? The CFE already has fix for this (a reverted CL that I need to rebase).

[srawlins]

Sorry this fell off my radar. @scheglov should have the most context of whether we could do this in the next week. WDYT, @scheglov ? I could also take a look.

[scheglov]

I don't have any specific knowledge about inference here, so if you can do it, go for it.

[srawlins]

@johnniwinther I guess we missed the Beta 2 cutoff last week, but I can still land this by tomorrow. Should I do that? https://dart-review.googlesource.com/c/sdk/+/262540

[johnniwinther]

I haven't relanded the CL in CFE either. Let's land this for Beta 3, i.e. when the candidate for Beta 2 has been picked, which I don't know if it has yet.

[srawlins]

I think you're right; branch alignment is this week.

[srawlins]

Oops I am seeing breakages in google3. At least one anyway in the sass package; I'll start mailing fixes today.

[srawlins]

@johnniwinther can you review go/dart-could-not-infer-top-level-lsc as a "Domain Reviewer" and then I can fix all of the internal errors (in every case by adding an explicit type argument).

[srawlins]

I think I've cleaned up google3 and Sass, the only 3p package that seems to be affected. Running another check in google3 today before landing.

[johnniwinther]

@srawlins Sounds good. Let me know when the analyzer change has landed, then I'll revive and land the CFE fix.

[srawlins]

Landed.