Config specific imports don't work with analyzer/ddc

[jakemac53]

I have been working on a prototype to support config specific imports properly in package:build, and I have gotten something to work, but during compilation I get errors like this:

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

[error] Target of URI doesn't exist: 'default.dart'. (/web/main.dart, line 7, col 8)
[error] Target of URI doesn't exist: 'ui.dart'. (/web/main.dart, line 8, col 26)
[error] Target of URI doesn't exist: 'io.dart'. (/web/main.dart, line 9, col 26)

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

my imports look like this:

import 'default.dart'
    if (dart.library.ui) 'ui.dart'
    if (dart.library.io) 'io.dart'
    if (dart.library.html) 'web.dart';

Note that you don't see the error about web.dart, because I did in fact provide that one (because I knew it was the only one that would actually be loaded).

I can work around the issue with // ignore: URI_DOES_NOT_EXIST comments (although I need one for every single condition...), but that shouldn't be necessary :).

It looks like this error is probably coming from the analyzer, but I am unsure if I should assign it to the dev_compiler or analyzer areas - assigning to analyzer for now but please switch if you feel that is incorrect.

[jakemac53]

cc @vsmenon @jmesserly

[jakemac53]

I am marking this as p1 because supporting config specific imports is on my Q3 okrs, and this is effectively a blocker for that I think, but we could re-adjust if we think requiring the // ignore is acceptable.

[vsmenon]

Assigning to @jmesserly to triage. If we're moving to DDK, we might be able to avoid this. @jakemac53 - do you know if we have a similar issue with kernel?

OTOH, it's possible analyzer build mode will hit the analogous issue.

[jakemac53]

analyzer build mode does not appear to have this issue (when creating summaries) - but I don't know that they report errors at all.

[jakemac53]

I will also check with Kernel/DDK next week, if it does work we can probably not worry about fixing this.

[jakemac53]

The ddk pipeline is a bit broken right now (going to look into a fix in the SDK soon), but I was able to hack some stuff together and it looks like this is still an issue. I end up getting this error:

org-dartlang-app:/web/io.dart:1: Error: StandardFileSystem only supports file:* and data:* URIs

It shouldn't be trying to read that file at all, I think the multiroot file system falls back on the standard one if it can't actually find a file which is why you see this specific error.

[jmesserly]

yeah I had removed multi-root temporarily to eliminate variables. I'm trying to merge dartdevk into dartdevc (we need to only have one compiler binary), this requires making them support the same command line options.

To that end I removed options that were added to dartdevk, but unsupported by dartdevc. The theory is, nobody is using dartdevk yet (it was just our testing ground for kernel/CFE support), so options can be removed to simplify the merge. I agree that we need something like mutli-root long term (it is much nicer than existing dartdevc/dart2js/dartanalyzer support for build systems, which has undocumented options that differ between each compiler). I may be able to port it to dartdevc but I'm not sure. Analyzer's APIs for URI resolution and resource providers is pretty different from CFE's support. So we may need to wait for DDC to switch fully to CFE/Kernel.

Anyways, that's the story behind those options. :)

[jmesserly]

I think this is an Analyzer issue; I believe we have one tracking it, let me see if I can find it.

[jmesserly]

I think this is the same issue as #34179 ... LMK if I'm wrong about that, we can reopen this.

[jakemac53]

This one is a bit different, that one is about config specific exports (which I think matter less in general, but should work).

This one is pretty much a blocker for using config specific imports at all because it forces you to use the // ignore: on every single config specific import uri.

[jakemac53]

I do think it is an analyzer issue though, but possibly ddc could know enough to ignore these errors?

[jmesserly]

it's probably the same issue in Analyzer though ... there's not really a difference between import and exports (for the purposes of URI resolution). I guess we can just assign this to area-analyzer, but it's probably the same underlying bug.

[jakemac53]

Fwiw analyzer summary generation does not have this issue - although I don't fully understand why. It might just not check for errors at all when generating summaries.

[jakemac53]

Oh, sorry I didn't catch what you meant about it likely being the same underlying bug. I actually think they are two separate issues though, the config specific imports do actually work if you silence this error. The exports on the other hand don't work at all.

[jmesserly]

Yeah that's interesting. From Paul's comment in issue 34179 it sounds like config specific imports/exports may not be fully implemented in Analyzer. As I mentioned in that issue it could be that DDC isn't setting some flag necessary in Analyzer to enable the feature, or perhaps the declared variables (e.g. "dart.library.ui") aren't getting passed through correctly. Not sure. The error is definitely coming from Analyzer though.

[jakemac53]

This is submitted now, although not released yet, so you can get a repro by doing the following:

• clone the build repo https://github.com/dart-lang/build
• cd _test
• pub run build_runner watch test
• edit the _test/test/config_specific_import_test.dart and remove the // ignore: uri_does_not_exist comment on one of the non-web imports

Note that it does appear that kernel is working properly here - we also build this test with the vm kernel pipeline and it appears to work just fine (removing the comment from the html import works fine).

[jakemac53]

AFAICT configurable imports are not yet in wide use

It is worth noting that we updated some of the core packages to use them though in order to make things truly platform-independent. Specifically, http, test, package_resolver, and I think one more I am forgetting.

So, while not a large number of packages use them some very core packages do.

[MichaelRFairhurst]

I'm going to change this to a DDC issue.

I also don't think it is an issue any more -- it's party a matter of opinion, but changing default.dart should absolutely invalidate the build IMO because as Jenny said, it should be validated against the interface.

Imagine:

default.dart

String get platformName;

web.dart

String get platformName => 'web';

my_lib.dart

import 'default.dart'
  if dart.library.web 'web.dart';

If you change default.dart to

enum Platform {
  web,
  server,
  ...
}

Platform get platform;

then the application should recompile, and subsequently fail.

While you can debate whether the failure should be in build or only in IDE/analyze, I think the fact that there's a debate makes it not a P1. I also don't see a reason why including default.dart in the build would create a P1 level performance decrease.

[MichaelRFairhurst]

I'm also downgrading to a P2.

And actually I'm not sure if it should be a DDC issue.... @jmesserly when you said a bug is to not validate against the interface, is that a DDC bug or analyzer bug as you see it?

[jakemac53]

The problem is that in default.dart you might be importing dart:io (which you want to use by default), and then importing dart:html in some conditional import. Because we have no restrictions on the default import, its perfectly valid to do that, and expect it to work for web projects.

In that scenario it doesn't make sense to provide the default file when compiling for web (it could also import dart:ui, or whatever other platform specific lib which likely won't exist at all). It also might have other transitive dependencies which it doesn't make sense to pull in, and could arbitrarily increase build times for no practical reason.

Note also that kernel does not read the default file. There is nothing in the spec that tells it to do so afaik.

I think this probably needs to be escalated to the language team though, cc @leafpetersen @lrhn @eernstg. The real issue is that there is no specification here.

[vsmenon]

@MichaelRFairhurst

In your example:

import 'default.dart'
  if dart.library.web 'web.dart';

I believe the analyzer either type checks against default.dart or web.dart but never both (and there is nothing to validate that the two are coherent). When invoked from DDC, it needs to use the latter - otherwise we have a soundness problem.

Similarly, when we use the analyzer modularly to generate summaries (as in google3), it needs to be modal - i.e., it needs to pick web.dart on web platforms, etc.

When the analyzer is run in this mode for a web platform, is it reading both files? If so, what does it actually do with default.dart?

[natebosch]

It's not DDC's responsibility to validate anything other than what we've asked it to compile. Checking for API compatibility across config specific imports is a different concern from compiling code.

The VM doesn't report any error when a default import is not present.

On dartpad I can compile and run this code with dart2js:

import 'dart:not_real'
  if (dart.library.html) 'dart:html';

void main() {
  print(Element);
}

[MichaelRFairhurst]

Thanks for chiming in everyone! I'm going to keep on pushing on this and welcome pushback as people see it necessary.

It also might have other transitive dependencies which it doesn't make sense to pull in

It does make sense (and is required) to pull them in if you want to validate the API implements the required interface.

and could arbitrarily increase build times for no practical reason.

That's not a P1 issue, even in the case that there is no practical reason.

I believe the analyzer either type checks against default.dart or web.dart but never both (and there is nothing to validate that the two are coherent). When invoked from DDC, it needs to use the latter - otherwise we have a soundness problem.

That's fine, but I don't see a soundness problem in using both. In fact, we currently have a soundness problem by not using both, which Jake points out here:

The problem is that in default.dart you might be importing dart:io (which you want to use by default), and then importing dart:html in some conditional import.

This is certainly a big problem. That code cannot be valid, if I understand correctly. Either the web version doesn't implement default.dart correctly, or the web version also imports dart:io which it can not do. @leafpetersen @munificent for confirmation here.

It's not DDC's responsibility to validate anything other than what we've asked it to compile. Checking for API compatibility across config specific imports is a different concern from compiling code.

We can keep this an analyzer bug if that's what you're saying.

It's absolutely, though, DDCs job to refuse to compile invalid code. False negatives are a bigger concern for maintaining Dart as a language than false positives are. If DDC compiles code that is invalid Dart, that weakens our ability to iterate on Dart. @leafpetersen for thoughts here.

I think the fact that dart2js and the vm don't report this is also a problem, a remnant from dart 1 style thinking.

To draw an admittedly extreme comparison, we're debating a variant of this case:

interface.dart

class Interface {
  int get foo;
}

implementation.dart

import 'interface.dart';
class Implementation implement Interface {
  int get foo;
}

There should never be a situation in Dart 2 where any of the compilers decide to ignore interface.dart. The tools should not see that Implementation is the only implementation of Interface and ignore interface.dart. There should be no, say, annotation, that you can put on interface.dart to mark it as not necessary for build because it's just an interface.

And if there were no validation that Implementation implements Interface, then that would be the issue. Which is what I see here.

If this is causing extreme performance hits then it may be a P1.

More likely I would treat it as a P1 because we are compiling code that may be invalid...and I'm seeing a lot of pushback about that being an issue. @leafpetersen again, really is who I see being able to make this call.

If the language team disagrees with me, then I don't see a P1 because I don't see it causing any major issues for any major customers.

Welcome to be corrected. I could absolutely be missing context, but I'm just going off of what I read here, and could have missed something.

[jakemac53]

This is certainly a big problem. That code cannot be valid, if I understand correctly. Either the web version doesn't implement default.dart correctly, or the web version also imports dart:io which it can not do. @leafpetersen @munificent for confirmation here.

I think this is all a big misunderstanding between the original DEP and what made it into the actual language spec. I can't find anything about a default import being special in any way.

Since the default has no meaning in itself (its only the fallback import if none of the conditions were true), it is totally valid to have a dart:io import in the default implementation and a dart:html import in the web implementation.

It does make sense (and is required) to pull them in if you want to validate the API implements the required interface.

Again isn't part of the language spec. It isn't implemented in any frontend. This only existed in the original dep and if I recall was rejected because of the potential complications it would bring.

The only things that are in the spec are the definition of a configurable uri, which only results in a single actual uri being chosen out of the list, and the definition of an import. The import specification does specify a compile time error if its uri doesn't point at a valid library (unless its a deferred import), but that uri is the single uri that was selected by the configurable uri, not all the possible ones and not the default one plus the selected one.

If the language team disagrees with me, then I don't see a P1 because I don't see it causing any major issues for any major customers.

This is causing issues for real customers because today on the web if any of your transitive dependencies use configurable imports then you get a compilation error and are completely blocked (unless they add the ignore comments to all the configurable imports).

[MichaelRFairhurst]

This is causing issues for real customers because today on the web if any of your transitive dependencies use configurable imports

Thanks for linking to this issue

then you get a compilation error and are completely blocked (unless they add the ignore comments to all the configurable imports).

Why isn't supplying default.dart a solution?

[natebosch]

It does make sense (and is required) to pull them in if you want to validate the API implements the required interface.

We specifically don't want to validate that in DDC.

It's absolutely, though, DDCs job to refuse to compile invalid code.

The code is not invalid. DDC is not the tool users want to check their code for presubmit errors. DDC is a tool the user wants to compile their code and let them iterate on the web.

Imagine the situation where I'm an author working on a brand new package that will work on VM and web. I want to write the web implementation first and leave default.dart for later. I do not expect DDC to disallow iteration and progress on my implementation because I chose to do it first there instead of first on the VM.

I think the fact that dart2js and the vm don't report this is also a problem

I should not need to have a fully fleshed out web implementation to test my code on the VM, or vice versa.

This is causing issues for real customers because today on the web if any of your transitive dependencies use configurable imports then you get a compilation error and are completely blocked (unless they add the ignore comments to all the configurable imports).

Note also that the users who this is impacting may have no control over the code that would need the comment. If the author of the package with configurable imports is using any other backend than DDC they don't even know it's a problem.

[jakemac53]

Why isn't supplying default.dart a solution?

• if somebody imports a platform specific library in that file it would cause us to think the application doesn't support any platforms, and not even attempt to compile it (both internally and externally).
• it adds a dependency on the default dart import and all its transitive imports, which you don't want.
• we should be solving the root of the problem not working around it

[MichaelRFairhurst]

Still going to push on this.

DDC is not the tool users want to check their code for presubmit errors

How is this different than any other error that DDC reports? DDC has options to force compile in invalid code. What distinguishes this case from, say, code that isn't strong-mode clean?

if somebody imports a platform specific library in that file it would cause us to think the application doesn't support any platforms, and not even attempt to compile it (both internally and externally).

Supporting such a dubious pattern does not strike me as a P1

it adds a dependency on the default.dart file and all its transitive imports, which you don't want.

Also doesn't strike me as a P1

we should be solving the root of the problem not working around it

I agree but we disagree what working around it means.

[MichaelRFairhurst]

For context, we have many many many P1s and are having trouble supporting NNBD. I don't see value in us working on this over some other things that are really really high priority.

[jakemac53]

As far as prioritization, I am ultimately fine with whatever decision the analyzer team wants to make here. Note that his bug is already >6 months old, and the world isn't on fire, likely because conditional imports aren't used widely at this time.

If this isn't going to be prioritized, we can work around it in a more general fashion (for instance dart-lang/build#1984 is one option).

I would like to reach an agreement whether or not this is considered a bug however. If the language team wants to change the spec, I am completely on board with that. It would be a breaking change almost certainly though to do so.

How is this different than any other error that DDC reports? DDC has options to force compile in invalid code. What distinguishes this case from, say, code that isn't strong-mode clean?

Strong mode is Dart 2. It is required to output strong mode errors at compilation time. This case involves valid code that only the analyzer reports as an error.

Supporting such a dubious pattern does not strike me as a P1

It isn't dubious, at least not by an agreed upon definition. It isn't for instance required to put all dart:io imports behind a conditional import, which would be a comparable use case. Since the default import is just that (the default implementation, not the interface), its totally ok for the default to import dart:io, and then only handle other specific platforms in other conditional imports. Maybe that isn't how everybody would structure it, but it isn't fundamentally broken or unsound, and it is supposed to be supported.

You could even argue that this is a better pattern than putting an unimplemented interface in the default import, because it will lead to static compilation errors for platforms that have no implementation, instead of runtime errors.

[vsmenon]

@MichaelRFairhurst - I'm fine with the prioritization, but I'm still confused on what the analyzer is actually checking here. See my comment above. If the analyzer (when called by DDC or generating a summary for a specific platform) is using the default version, that's a soundness problem. Is that the case here?

[eernstg]

@jakemac53 wrote:

The real issue is that there is no specification here.

It is specified, but the configurable import mechanism that Dart has now does not provide encapsulation. That is, the configurable import clause will import one of the specified libraries, and all the consequences of doing that are exactly the same as they would have been if we had edited a regular import.

In particular, it's worth noting that basically nothing is known about the static properties of a program with respect to a non-default configurable import based on an analysis using the default.

In other words, if some library L is imported (directly or indirectly) from your entry point main.dart and L contains something like this:

import 'default.dart'
    if (dart.library.ui) 'ui.dart'
    if (dart.library.io) 'io.dart'
    if (dart.library.html) 'web.dart';

.. then an analysis based on the assumption that default.dart will be imported says absolutely nothing about your program when the chosen import is anything other than default.dart, e.g., web.dart. Any expression could have a different type, any identifier could be looked up somewhere else, any error could exist with default.dart and disappear with web.dart, or vice versa.

OK, you can find some exceptions, e.g., 2+2 will not be an error and it will have type int in both cases. But the differences are certainly not confined to the library where the configurable import is located, and since it can change the set of members of a given class (because, e.g., different imports can give completely different meanings to its superinterfaces), it will not just change the namespaces available at the top level.

So we cannot assume that an analysis based on the default of a configurable import will be valid in any sense for a compilation where some other choice is made. The only way we can get a correct analysis is by specifying exactly the configuration that we want to compile.

Of course, it would be possible to define a notion of 'the type of a library', and allowing for a configurable import construct to select one among a set of libraries that can be given a specific type. This would encapsulate the configuration specific choice of a library (for instance, we could then perform static analysis and generate code that would be valid, both when default.dart is chosen and when web.dart is chosen).

But that is a non-trivial exercise (for instance, check Russo's approach for SML), and we haven't done it for Dart.

@vsmenon wrote:

I believe the analyzer either type checks against default.dart or web.dart
but never both (and there is nothing to validate that the two are coherent).
When invoked from DDC, it needs to use the latter - otherwise we have a
soundness problem.

If the analyzer works as described then it will work correctly, and as specified. Woohoo! ;-)

[jakemac53]

It is specified

Right sorry I think this had not made it into the spec when the issue was originally opened but it is in the spec now.

It doesn't explicitly specify whether or not any or all of the imports must be available to any given compiler explicitly or not, but it only treats a conditional uri as resolving to a single uri (as I read it at least), so only that single one it resolves to should have to be available imo. Otherwise we can not properly exclude the code from one platform as a dependency when compiling for another platform.

If the analyzer works as described then it will work correctly, and as specified. Woohoo! ;-)

It will read the correct one, yes, but it will also attempt to read all the other ones before it selects the correct one, and give an error for each one that isn't available to it. That is specifically what this bug is trying to address.

[eernstg]

That should just work with the current specification: Section 'URIs' specifies how to select one of the choices in a configurable import, and this does not involve any actions for the non-chosen ones (like checking whether there is such a library). On top of that, the interpretation of a URI is mainly implementation specified.

[leafpetersen]

+1 to what @eernstg said. The specified behavior for this is that there is no required relationship between the contents of the different imports. Checking against any one of them (including the default import) tells you nothing about errors you may or may not get when checking against any other.

[MichaelRFairhurst]

Thanks for weighing in!

OK, glad to have that clarified and I was thinking from a non-realistic point.

This definitely makes the correct solution here for the analyzer to not report missing URI on default.dart, and that's what we'll do once we get to this.

[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.