Sdk: Config specific exports don't work with analyzer/ddc

[jakemac53]

Example gist https://gist.github.com/jakemac53/2d7449e8002589f2f93a828c5bf41915

Given a structure like the gist above, I get an error like this:

Error compiling dartdevc module:_test|web/main.ddc.js

[error] Undefined name 'platform'. (/web/main.dart, line 10, col 9)

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

So it doesn't appear that anything is actually being exported.

Note that in this scenario the platform.dart file does live in a separate module - so it could be an error on the analysis summary side of things. It _does_ work in my IDE and I can click through to the definition (it goes to the default.dart import).

cc @jmesserly @vsmenon @leafpetersen

[jakemac53]

I will check next week if this issue is present with kernel/ddk as well - if it isn't we might be able to skip fixing it.

[jakemac53]

Looks like I get a different error for DDK so I can't test this properly right now, will follow up.

[jmesserly]

Yeah we haven't implemented anything specific for this on DDC side (because Analyzer/CFE should resolve it for us). I don't know how config specific imports/exports are implemented in Analyzer though. It might require a flag to enable the feature?

[jakemac53]

It might be that this is a bug in the analyzer summaries or something... @stereotype441 any ideas?

[stereotype441]

No ideas off the top of my head. Even though config specific imports/exports aren't fully implemented in the analyzer IIRC, your example seems like it should work anyhow, since even if you disregard the config-specific imports, the name platform should be resolvable in default.dart.

Note that in your gist, default.dart has a compile-time error (missing ;). I'm assuming that's an unrelated problem.

[yjbanov]

Added customer-flutter label as it might be the cause of https://github.com/flutter/flutter/pull/27668#issuecomment-462569375. Feel free to remove the label if I'm wrong.

[natebosch]

cc @stereotype441 - we're getting new user reports about this. Can we prioritize a fix?

[natebosch]

See also #35672 which highlights additional confusing UX around this lack or support.

[vsmenon]

@stereotype441 - some quick thoughts here. The following:

import 'dart:io'
    if (dart.library.html) 'dart:html_misspelled';

void main() {
  // print(HttpRequest);
}

compiles without error in DDC. The code above also doesn't show an error in the IDE (but if I misspell dart:io it does). The analyzer doesn't check the right (or any non-default) imports. I don't think there is a soundness issue though. The right file is eventually loaded and used for resolution. If I uncomment the code above, I'll get there right error there.

If this is time consuming to fix, I think (for now) suppressing this check altogether when multiple configurations are present may be best short term step.

[jumperchen]

any update?
We are waiting for this to upgrade to Dart 2.2. :)

[jakemac53]

We are waiting for this to upgrade to Dart 2.2. :)

Fwiw this shouldn't be blocking migration to 2.2, it hasn't ever worked properly

[jumperchen]

But, it works with 2.1.0 version for us, we cannot use d2j to compile our source code for 3~5 minutes only single line change with 2.2 version in development phase.

[natebosch]

But, it works with 2.1.0 version for us

I'm not aware of any regression between 2.1.0 and 2.2.0 around this issue. Can you explain the problem you hit when you tried to upgrade to 2.2.0 that was not happening in 2.1.0?

[jumperchen]

@natebosch Yes, there is a simple reproducible example in #35672 which was failure to run with 2.1.0, but I found a workaround by adding ignore: export_of_non_library to avoid DDC analyzer error, and then it works well with 2.1.0. After we upgrade to 2.1.1 or 2.2.0-dev.2.1 the problem appears again (mentioned in #36029), and it seems not to work starting from 2.1.1-dev.0.0.

[jumperchen]

@jakemac53 I try with your example and add my workaround, it can work well with Dart 2.1.0 version.
For example,

// ignore: URI_DOES_NOT_EXIST, export_of_non_library
export 'default.dart'
// ignore: URI_DOES_NOT_EXIST
if (dart.library.io) 'io.dart'
// ignore: URI_DOES_NOT_EXIST
if (dart.library.html) 'web.dart';

And it will load web.dart in browser only with DDC mode.

[jakemac53]

@jumperchen it is a bit complicated but I think you only see this issue if the library with the exports exists in a different module - which you don't have direct control over.

You can make that happen though by moving the platform.dart file and the configurable exports under the lib dir. I confirmed that still reproduces this issue with the latest sdk.

[jumperchen]

FYI: I try to verify this by myself, and I found the request arguments from build_web_compilers are the same between 2.1.0 vs 2.1.1 as follows

2.1.1

[WARNING] build_web_compilers|ddc on test/example/main.dartdevc.module:
arguments: --dart-sdk-summary=/Users/jumperchen/Downloads/dart-sdk-2.1.1/lib/_internal/ddc_sdk.sum
arguments: --modules=amd
arguments: -o
arguments: /private/var/folders/y_/y0dr0qpj1_ngqmd6y71h2kf80000gn/T/scratch_spacejr2b1x/test/example/main.ddc.js
arguments: --module-root=.
arguments: --library-root=/test
arguments: --summary-extension=dartdevc.linked.sum
arguments: --no-summarize
arguments: --options=/private/var/folders/y_/y0dr0qpj1_ngqmd6y71h2kf80000gn/T/scratch_spacejr2b1x/packages/build_modules/src/analysis_options.default.yaml
arguments: --source-map
arguments: --source-map-comment
arguments: --inline-source-map
arguments: -s
arguments: /private/var/folders/y_/y0dr0qpj1_ngqmd6y71h2kf80000gn/T/scratch_spacejr2b1x/packages/foo/web.dartdevc.linked.sum
arguments: -s
arguments: /private/var/folders/y_/y0dr0qpj1_ngqmd6y71h2kf80000gn/T/scratch_spacejr2b1x/packages/foo/platform.dartdevc.linked.sum
arguments: --url-mapping=file:///test/example/main.dart,test/example/main.dart
arguments: file:///test/example/main.dart

2.1.0

[WARNING] build_web_compilers|ddc on test/example/main.dartdevc.module:
arguments: --dart-sdk-summary=/usr/local/Cellar/dart/2.1.0/lib/_internal/ddc_sdk.sum
arguments: --modules=amd
arguments: -o
arguments: /private/var/folders/y_/y0dr0qpj1_ngqmd6y71h2kf80000gn/T/scratch_spaceeOu0LZ/test/example/main.ddc.js
arguments: --module-root=.
arguments: --library-root=/test
arguments: --summary-extension=dartdevc.linked.sum
arguments: --no-summarize
arguments: --options=/private/var/folders/y_/y0dr0qpj1_ngqmd6y71h2kf80000gn/T/scratch_spaceeOu0LZ/packages/build_modules/src/analysis_options.default.yaml
arguments: --source-map
arguments: --source-map-comment
arguments: --inline-source-map
arguments: -s
arguments: /private/var/folders/y_/y0dr0qpj1_ngqmd6y71h2kf80000gn/T/scratch_spaceeOu0LZ/packages/foo/web.dartdevc.linked.sum
arguments: -s
arguments: /private/var/folders/y_/y0dr0qpj1_ngqmd6y71h2kf80000gn/T/scratch_spaceeOu0LZ/packages/foo/platform.dartdevc.linked.sum
arguments: --url-mapping=file:///test/example/main.dart,test/example/main.dart
arguments: file:///test/example/main.dart

So I also try to diff the two sources between 2.1.0 and 2.1.1-dev.0.0, in the dev_compiler, and this commit https://github.com/dart-lang/sdk/commit/1e186ad9cbebbf363da655698b07bdba96713b2d may relate to this issue (just guess, because I cannot run dev_compiler locally).
From that change, it seems not to add all summary links into the link result. (about why it causes Undefined name 'platform')

If someone can tell me about how to run dev_compiler with source locally, I will help you to identify this issue.

[vsmenon]

@stereotype441 - adding this to 2.4 for now. This may be a blocker to flutter web integration (even without analyzer-ddc).

[vsmenon]

Is this still an open (with DDK)?

[jakemac53]

Exports work with DDK (as do imports, without the // ignore hack)

[yjbanov]

Is this ready to be used? This is blocking integration of DDC into the Flutter SDK.

[jakemac53]

With the latest build_web_compilers (which uses kernel), yes.

Analyzer is still not functioning which might be a blocker still, since you presumably need things to work internally as well.

[yjbanov]

Thanks! This is only for the open-source projects. Internal workflow is not affected.

[stereotype441]

I just spoke with @jakemac53 about this. There's a number of interacting issues:

• Internally, we only build one summary for each build target, even if it supports multiple configurations. This is a problem, because summaries store constant values, inferred types, and the targets of exports, and they can be different from one configuration to the next.
• Both internally and externally, we only build one summary of the SDK that's shared by all configurations. This is especially a problem because most of the config-specific differences are in the SDK.
• We can't fix this bug just for the external workflow because of the bullet point above.
• Internally, we are experimenting with a summary-to-kernel-outline conversion layer (so that we don't do redundant work between the summary and kernel pipelines); for correctness, kernel requires different outlines for each configuration, so this is incompatible with having a single summary for each configuration.

For config specific imports, we're currently at a point of delicate stability where they're used infrequently enough that we don't happen to hit any bugs due to differences in constants, inferred types, or export targets. (We have to ignore some stray analyzer errors though, due to the analyzer trying to visit a file that's not correct for the given configuration). It's likely that we can hobble along a little longer in this condition, but with each passing day we're taking a risk of the house of cards falling over.

For config specific exports, we might be able to reach a similar point of delicate stability, but then again we might not, because the consequences of the analyzer trying to visit the wrong exported file are worse than for trying to visit the wrong imported file; DDC will fall over trying to compile not just the file with the config-specific export, but any file that imports it and makes use of the exported symbols.

Jake and I don't believe we'll be able to address these issues in the D24 timeframe, and I don't think it makes sense to hold up the D24 release for this. So I'm reassigning the milestone. But I'm keeping the P1 designation because the situation is delicate enough that we need to fix it as soon as we can.

Jake is setting up a follow-up meeting to discuss what steps we should take next.

[yjbanov]

@stereotype441

Both internally and externally, we only build one summary of the SDK that's shared by all configurations. This is especially a problem because most of the config-specific differences are in the SDK.
We can't fix this bug just for the external workflow because of the bullet point above.

There's no Web support in the Flutter SDK yet, so we won't be breaking anyone by adopting something new that works.

[jumperchen]

@stereotype441, we are still waiting for this fix to upgrade dart SDK from 2.1.0 version, hope you can find a workaround for us, thanks.

[stereotype441]

I discussed this with @jakemac53, @yjbanov, and @jonahwilliams yesterday. Since the Flutter code that triggered this problem is going to be compiled using kernel, we believe this is no longer necessary to fix on the analyzer. We'll reopen the issue if that changes.

[natebosch]

For clarification, the problem wasn't specific to flutter web and the usage of kernel to compile is not specific to flutter web. All external projects using build_web_compilers version 2 will be using the kernel codepath and this issue will not impact them. With the latest SDK we've fixed the bugs with JS interop and we expect all users to migrate to the latest build_web_compilers.

[jumperchen]

@natebosch
Thanks for the clarification, after we upgrade to Dart version 2.3.2 and build_web_compilers version 2, the error of conditional exports disappears, but it brings a new issue https://github.com/dart-lang/sdk/issues/36944 and @jakemac53 suggested on https://github.com/dart-lang/webdev/issues/409 to ask us to drop back to build_web_compilers: ^1.0.0, so ... any suggestion for now?

[jakemac53]

The issue with js interop should be resolved with the latest sdk dev release (2.4.0-dev.0.0) if you want to try that.

[jumperchen]

@jakemac53
Thanks, it works.