dart-lang/sdk

Edge SDK fails to compile front_end 0.1.4+1

Closed this issue · 22 comments

jakemac53 commented

This looks like either a new error in the CFE or a mismatch in versions between front_end and kernel, which was fixed in front_end 0.1.5. However, you can't actually get that version without depending on the alpha version of the analyzer, which the world hasn't rolled to yet.

SDK version: Dart VM version: 2.1.0-edge.0cc7993ee356e7858dcc05964253b4a4e9a48ef9 (Thu Oct 4 14:54:28 2018 +0000) on "linux_x64"
Fetched from: https://storage.googleapis.com/dart-archive/channels/be/raw/latest/sdk/dartsdk-linux-x64-release.zip

Example failure:

file:///<redacted>/.pub-cache/hosted/pub.dartlang.org/front_end-0.1.4+1/lib/src/fasta/kernel/expression_generator.dart:789:14: Error: The method 'ErroneousExpressionGenerator::buildCompoundAssignment' has fewer named arguments than those of overridden method 'KernelExpressionGenerator::buildCompoundAssignment'.
  Expression buildCompoundAssignment(Name binaryOperator, Expression value,
             ^
file:///<redacted>/.pub-cache/hosted/pub.dartlang.org/front_end-0.1.4+1/lib/src/fasta/kernel/kernel_expression_generator.dart:180:14: Context: This is the overridden method ('buildCompoundAssignment').
  Expression buildCompoundAssignment(Name binaryOperator, Expression value,
             ^
file:///<redacted>/.pub-cache/hosted/pub.dartlang.org/front_end-0.1.4+1/lib/src/fasta/kernel/kernel_expression_generator.dart:1521:7: Context: Override was introduced when the mixin 'ErroneousExpressionGenerator' was applied to 'KernelGenerator'.
class KernelUnresolvedNameGenerator extends KernelGenerator
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
file:///<redacted>/.pub-cache/hosted/pub.dartlang.org/front_end-0.1.4+1/lib/src/fasta/kernel/expression_generator.dart:789:14: Error: The method 'ErroneousExpressionGenerator::buildCompoundAssignment' doesn't have the named parameter 'isPostIncDec' of overridden method 'KernelExpressionGenerator::buildCompoundAssignment'.
  Expression buildCompoundAssignment(Name binaryOperator, Expression value,
             ^
file:///<redacted>/.pub-cache/hosted/pub.dartlang.org/front_end-0.1.4+1/lib/src/fasta/kernel/kernel_expression_generator.dart:180:14: Context: This is the overridden method ('buildCompoundAssignment').
  Expression buildCompoundAssignment(Name binaryOperator, Expression value,
             ^
file:///<redacted>/.pub-cache/hosted/pub.dartlang.org/front_end-0.1.4+1/lib/src/fasta/kernel/kernel_expression_generator.dart:1521:7: Context: Override was introduced when the mixin 'ErroneousExpressionGenerator' was applied to 'KernelGenerator'.
class KernelUnresolvedNameGenerator extends KernelGenerator
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
file:///<redacted>/.pub-cache/hosted/pub.dartlang.org/front_end-0.1.4+1/lib/src/fasta/kernel/expression_generator.dart:952:14: Error: The method 'ContextAwareGenerator::buildCompoundAssignment' has fewer named arguments than those of overridden method 'KernelExpressionGenerator::buildCompoundAssignment'.
  Expression buildCompoundAssignment(Name binaryOperator, Expression value,
             ^
file:///<redacted>/.pub-cache/hosted/pub.dartlang.org/front_end-0.1.4+1/lib/src/fasta/kernel/kernel_expression_generator.dart:180:14: Context: This is the overridden method ('buildCompoundAssignment').
  Expression buildCompoundAssignment(Name binaryOperator, Expression value,
             ^
file:///<redacted>/.pub-cache/hosted/pub.dartlang.org/front_end-0.1.4+1/lib/src/fasta/kernel/kernel_expression_generator.dart:1612:16: Context: Override was introduced when the mixin 'ContextAwareGenerator' was applied to 'KernelGenerator'.
abstract class KernelContextAwareGenerator extends KernelGenerator
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
file:///<redacted>/.pub-cache/hosted/pub.dartlang.org/front_end-0.1.4+1/lib/src/fasta/kernel/expression_generator.dart:952:14: Error: The method 'ContextAwareGenerator::buildCompoundAssignment' doesn't have the named parameter 'isPostIncDec' of overridden method 'KernelExpressionGenerator::buildCompoundAssignment'.
  Expression buildCompoundAssignment(Name binaryOperator, Expression value,

Reproduction Instructions

Create a new package which depends on the analyzer at <0.33.0 version (which was just released and the world has to migrate to still), and a dart script that imports analyzer:

pubspec.yaml:

name: repro
  dependencies:
    analyzer: <0.33.0

main.dart:

import 'package:analyzer/analyzer.dart';
main() {}

Try to snapshot the script:

dart --snapshot=main.dart.snapshot main.dart
jakemac53 commented

cc @kmillikin @whesse This should block the next dev release of the sdk so bumping to p0

bwilkerson commented

We just published version 0.33.0 of analyzer, 0.1.6 of front_end, and 0.3.6 of kernel.

jakemac53 commented

Great @bwilkerson, but it will take some time to roll the world forward still. I updated the repro instructions to use analyzer <0.33.0 since I confirmed things do work with the latest version.

Also, if this is in fact a new error in the CFE (which it appears to be since things work fine on the current dev sdk) then we need to make sure we are intentionally releasing a breaking change in the SDK without a breaking version bump, for which the bar should be extremely high.

stereotype441 commented

A more immediate short term solution might be to cherry-pick 8a99ab0 to branch analyzer-0.32 (note: this branch isn't mirrored to github) and publish an intermediate point release of front_end (I believe the correct version number would be 0.1.4+2).

vsmenon commented

Is this still a P0 given the new analyzer?

stereotype441 commented

Correction to my previous comment: we would need to publish intermediate point releases of analyzer, front_end, and kernel, since their pubspecs refer to each other via exact version numbers.

keertip commented

This could be related to the cfe work in d3bd06

vsmenon commented

So, this looks like CFE added an error check that it itself was violating. That violation was fixed, but anything with an old version of the CFE (as a package) and new SDK will break.

Two questions:

  • @athomas @kmillikin - is the above correct and if so, what version dance do we need to do before pushing out 2.1?
  • Are we running analyzer on our own SDK hosted packages? Would it have caught this?
jakemac53 commented

Talked to @stereotype441 and @kevmoo - currently proceeding with the following actions:

  • @stereotype441 is going to drive publishing +1 releases of analyzer/kernel/front_end such that any existing user should be able to get the fix by doing a pub upgrade.
  • @kevmoo is driving an effort to attempt to analyze all packages on pub with the edge sdk, to identify how much of the existing world is broken by this change. This will hopefully inform the decision to roll back or keep the error. The expectation is that it affects very few packages.
stereotype441 commented

CL to create point releases of the analyzer/kernel/front_end is here: https://dart-review.googlesource.com/c/sdk/+/78149. Once that is reviewed and lands I'll do the point release.

Note that more fixes were required than I anticipated; in addition to cherry-picking 8a99ab0, I had to cherry-pick c95ef87, and make some minor changes to analyzer test code.

I talked to @keertip about whether any internal code is affected by this fix; she's investigating and doesn't have a definitive answer yet. I'm going to do some checking on my own as well.

jakemac53 commented

Great, checking internally is a really good step in addition to checking the external packages. Even if we can't end up checking external code the internal code should give a good signal.

stereotype441 commented

I've just published:

  • analyzer 0.32.6
  • front_end 0.1.4+2
  • kernel 0.3.4+2
  • 🎉2
kevmoo commented

@stereotype441 – looks like the updated packages made build happy - https://travis-ci.org/dart-lang/build/jobs/436858283

devoncarew commented

an update: Paul is working on a lint rule to catch other issues like this at analysis time

kevmoo commented
devoncarew commented

I believe that's to give us a better sense of the scope of the problem.

stereotype441 commented

That's right. The reason I'm doing a lint is because it's easier to cherry-pick into the internal codebase as an isolated change, so we can hopefully find internal code that is in danger of tripping up without having to do a full internal roll.

kevmoo commented
dgrove commented

Is this still a P0?

jakemac53 commented

We could possibly bump it down now that we at least have a fix for the front_end package itself (and a pub upgrade should fix issues for people).

However, we should complete the other evaluations of the impact of the change prior to the 2.1 release to ensure we don't break to much of the world, which should still be high priority (maybe just a p1 though?).

stereotype441 commented

Update: I ran my lint on internal code this weekend and it discovered no additional issues. So to my knowledge, this bug has been completely addressed and can be closed--the point releases of analyzer/front_end/kernel addressed the immediate issue, and the lint addressed the concern that there could be other failures lurking.

@jakemac53 does that sound correct to you?

jakemac53 commented

@stereotype441 yep, I agree

We should keep an eye out for users reporting this issue and make sure we are communicating quickly that they need to upgrade their packages to get the fix.

If users are stuck on older versions of the front_end package for one reason or another, they will have to stay on an older SDK until they can upgrade.

I also glanced at the flutter repo and it looks like their packages already pin the 0.1.5 front_end so that should already be ok as well.