[breaking-change-request] Report all top-level inference circularities

[eernstg]

Cf. dart-lang/language#1650.

Change in behavior

The proposed change is that top-level inference shall report a compile-time error for all cyclic dependencies, based on references from the body of one declaration to another declaration. The previous behavior included ignoring subterms (expressions or statements) in cases where inference on said subterms would not influence the type which is currently being inferred.

Justification/rationale

The previous behavior is complex, it is not consistent among all tools, and the complexity of the approach has been rising as flow analysis and other language mechanisms have evolved over time.

A software engineering rationale exists as well: In the situations where top-level type inference would succeed because of a subtle analysis that allowed certain parts of the code to be ignored, the types of the declarations in the given cycle would be rather difficult to discern for a human reader as well. With this change, types must be given explicitly in those situations, and this could improve both the readability, comprehensibility, and maintainability of the code.

Expected impact

The impact is that new compile-time errors will be emitted for dependency cycles that used to be accepted (because some subterms of the declarations were ignored).

The number of locations which would be affected is expected to be very low. Internally it is believed that only a couple of such locations exist.

Mitigation

For each dependency cycle which was previously accepted and which is now a compile-time error, add a declared type to at least one of the declarations in the cycle.

Example

Here is an example showing that a cyclic dependency can exist, and the program can be executed without entering into an infinite loop, but the circularity does not actually matter in relation to the inferred types:

var a = () {
  print(b);
  return 1;
}();

var b = () {
  print(a);
  return true;
}();

void main() {
  b = false;
  print(a);
}

With the proposed change, an explicit type must be given to either a or b.

This program is currently accepted by the common front end (version 2.19.0-331.0.dev accepts it), but work is ongoing (and the bleeding edge CFE rejects it). This breaking-change procedure is needed in order to make sure that the breakage is announced and discussed as it should be. If the change is accepted then the implementation in the CFE is basically ready. The analyzer currently rejects the above program, but needs some adjustments as well.

[itsjustkevin]

@vsmenon @Hixie @grouma fresh breaking change request.

[Hixie]

What message will we show in the analyzer and what message will we show in the compiler?

[scheglov]

The analyzer:

$ dart analyze /Users/scheglov/dart/test/bin/test.dart
Analyzing test.dart...                 0.4s

  error • test.dart:1:5 • The type of 'a' can't be inferred because it depends on itself through the cycle: a, b. Try adding an explicit type to one or more of the variables in the cycle in
          order to break the cycle. • top_level_cycle
  error • test.dart:6:5 • The type of 'b' can't be inferred because it depends on itself through the cycle: a, b. Try adding an explicit type to one or more of the variables in the cycle in
          order to break the cycle. • top_level_cycle

2 issues found.

[Hixie]

SGTM.

[vsmenon]

lgtm

[itsjustkevin]

@grouma have you had a chance to take a look at this?

[itsjustkevin]

@MaryaBelanger and @atsansone FYI.

[grouma]

Internally it is believed that only a couple of such locations exist.

Can these be preemptively cleaned up?

[johnniwinther]

The internal problems have been fixed.

@scheglov Can you land the change in the analyzer before the stable cut?

[scheglov]

It was already landed as dab3697.

[eernstg]

I see no arguments against performing this breaking change.

@johnniwinther, @scheglov, it sounds like this work is already well underway, and #46579 was used to handle some work on the analyzer. Is there a need for any further separate implementation issues?

[scheglov]

My understand is that I implemented all what was necessary in the analyzer.

[johnniwinther]

This has landed.

[mraleph]

I have just hit the introduced error when trying to analyze a sample code from "Real-World Flutter by Tutorials: Materials"

https://github.com/kodecocodes/rwf-materials/blob/editions/1.0/08-deep-linking/projects/final/lib/main.dart#L79-L89

gives:

  error • The type of '_favQsApi' can't be inferred because it depends on itself through the cycle: _favQsApi, _userRepository at ../../temp/rwf-materials/08-deep-linking/projects/final/lib/main.dart:79:14 • (top_level_cycle)
  error • The type of '_userRepository' can't be inferred because it depends on itself through the cycle: _favQsApi, _userRepository at ../../temp/rwf-materials/08-deep-linking/projects/final/lib/main.dart:86:14 • (top_level_cycle)
  error • The type of '_routerDelegate' can't be inferred because it depends on itself through the cycle: _routerDelegate at ../../temp/rwf-materials/08-deep-linking/projects/final/lib/main.dart:91:14 • (top_level_cycle)

I think the error message message could be improved in both analyzer and CFE (the latter reports: "Can't infer the type of '...': circularity found during type inference.").

Error message should probably link to some documentation on dart.dev or be more elaborate: it is obvious that the type of the variable does not depend on anything else, so the error should be more elaborate --- it probably should not say that compiler can't infer the type, but rather that it is not allowed to infer the type, e.g. something along the lines of:

  error • Two fields without explicit types _favQsApi and _userRepository refer to each other through their initializers. Due to this circularity compiler is not allowed to infer their types. Please specify the type of one or more fields explicitly. at ../../temp/rwf-materials/08-deep-linking/projects/final/lib/main.dart:79:14 • (top_level_cycle)

[MaryaBelanger]

Error message should probably link to some documentation on dart.dev

@mraleph I had the same concern a couple months ago.

Some diagnostic messages point to the Fixing type promotion failures page on dart.dev, where I think this explanation could go.

In flow_analysis.dart, you can see:

• this example of a class that points to a go link, and
• how that go link redirects
• to a corresponding Fixing type promo page section.

I'm happy to add the section to the dart.dev page based on the details in your comment. In fact, I think it needs to be done.

What I'm not capable of is adding a whole class to flow_analysis.dart. If that was already done as part of this development (~see note), then I'd be happy to just add the @override ... documentationLink => ... lines to it too. Just need to know where to look.

~ nothing in this issue points me to the actual commit that corresponds with this work so I can't find that out myself either, is it this class?

[bwilkerson]

@scheglov Who can probably answer the question of where the addition of the context message would need to happen.

[scheglov]

In the analyzer we report this as CompileTimeErrorCode.TOP_LEVEL_CYCLE.