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.
cc @vsmenon @jmesserly
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.
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.
analyzer build mode does not appear to have this issue (when creating summaries) - but I don't know that they report errors at all.
I will also check with Kernel/DDK next week, if it does work we can probably not worry about fixing this.
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.
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. :)
I think this is an Analyzer issue; I believe we have one tracking it, let me see if I can find it.
I think this is the same issue as https://github.com/dart-lang/sdk/issues/34179 ... LMK if I'm wrong about that, we can reopen this.
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.
I do think it is an analyzer issue though, but possibly ddc could know enough to ignore these errors?
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.
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.
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.
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.
This is submitted now, although not released yet, so you can get a repro by doing the following:
cd _test
pub run build_runner watch test
_test/test/config_specific_import_test.dart
and remove the // ignore: uri_does_not_exist
comment on one of the non-web importsNote 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 is this still a P1 issue for you? If so, can you work with @scheglov to get him a repro of the issue so that we can help fix it? At the moment there's so much going on in this bug discussion that we're a little unsure what you need from us.
@stereotype441 @scheglov
Yes, I would still consider this a P1 issue - nobody can use conditional imports internally or externally without the // ignore: uri_does_not_exist
comments today.
The above repro instructions still work.
Basically, if you attempt to summarize a library that uses conditional imports, you have to provide the analyzer with _all_ the possible imports, not only the ones that are required to compile, or else you get these errors. You can work around it with the ignore comment, and everything works as expected because it does not actually have to look at those files in order to summarize properly.
While we could just provide all those files, that defeats the purpose as it doesn't allow us to trim the dependencies to only the ones required for the target platform.
Just double checking: I assume you tried it using current HEAD, since you said the repro instructions still work? DDC no longer uses the task model, and should be working very similar to analyzer_cli build mode. I was hoping maybe that would fix it, but I guess not.
(FWIW, when I ported the linker code from analyzer_cli to DDC, I made sure that it doesn't issue any errors for missing source files. It just skips them. Analyzer already reports those errors later on, so it wasn't necessary to do any error checking in the linking step. The configurable imports tests are passing in DDC, but my guess is they supply all of the files for every configuration.)
Strange, I tried to reproduce it with this patch, but it didn't work (test still passed):
diff --git a/tests/language_2/config_import_test.dart b/tests/language_2/config_import_test.dart
index 2e15460cbe..c255bb4b86 100644
--- a/tests/language_2/config_import_test.dart
+++ b/tests/language_2/config_import_test.dart
@@ -15,6 +15,7 @@ import 'config_import_lib2a.dart'
if (not.set.either) 'config_import_lib2c.dart';
import 'config_import_lib3a.dart'
+ if (dart.library.io) 'file_that_does_not_exist.dart'
if (dotted.id == "some_string") 'config_import_lib3b.dart'
if (id) 'config_import_lib3c.dart';
I can repro with Jake's instructions though. Is it possible dart.library.io is somehow getting set to "true"?
I'll see if I can add some logging to the compiler to see what's going on.
@jmesserly I tried with the most recent dev release, but not HEAD.
Actually though, I just noticed that the behavior does appear to be a bit better than it used to be - I can now only reproduce this if I remove the // ignore:...
comment from the _default_ import. I don't know if that is because the ignore is automatically applying to all the configurations now or if the logic changed.
Either way though, you shouldn't have to provide the default import either if you know it won't be selected.
Also, I stated things a bit incorrectly previously, this fails for the ddc action not the summary action.
Ah gotcha! That makes sense--that's the behavior I'd expect from the implementation. In library_analyzer.dart, _validateUriBasedDirectives, it only checks the default import, not any configurations.
Either way though, you shouldn't have to provide the default import either if you know it won't be selected.
Oh wow, you don't even have the default import available? How does IDE support work?
https://github.com/munificent/dep-interface-libraries/blob/master/Proposal.md
While it's not technically required, we did want the possibility of validating that a configuration matches the interface. Without the interface library we can't do that.
In any case, we should be validating the URI exists for the currently active configuration, and that check seems to be missing from _validateUriBasedDirectives. (_resolveUriBasedDirectives seems to have a similar problem?)
Oh wow, you don't even have the default import available? How does IDE support work?
https://github.com/munificent/dep-interface-libraries/blob/master/Proposal.md
We do have it available - we just don't want to provide it as an input to the build action. Otherwise, we are invalidating actions based on files they will never read.
While it's not technically required, we did want the possibility of validating that a configuration matches the interface. Without the interface library we can't do that.
That isn't how it is implemented today though - if it provided value we could add this file as an input later. Today, the default implementation could just as easily be specific to a certain platform, and then you could use a conditional to handle just one other platform in some other way. The current implementation is extremely flexible and we aren't making any assumptions around how people use the imports.
Otherwise, we are invalidating actions based on files they will never read.
but the fact that the file isn't read is a bug :) ... there's supposed to be validation, even though it hasn't been implemented yet.
Does omitting the interface definition really save a lot of time in the builds? If so, why?
but the fact that the file isn't read is a bug :) ... there's supposed to be validation, even though it hasn't been implemented yet.
This is all very confusing because there is no formal specification or anything officially approved afaik :D. In the initial DEP that you link you were required to implement some special interface, and I think we all wish that is what we had implemented. However, that isn't what _did_ implement, and as far as I know there are no plans to implement that in the future (it can't happen until dart 3.0 in any case....). Instead we went with the much simpler route of just allowing the different imports to implement/expose whatever they want. The default
import is used by the analyzer in the context of IDEs because they have to make some arbitrary choice, and that seems like the most obvious one :). From that perspective it does encourage people to do the right thing (otherwise they likely get analyzer errors), but they don't actually have to implement the exact same thing as the default import - essentially what you get is duck typing.
Fwiw, kernel does _not_ look at that default import at all.
Does omitting the interface definition really save a lot of time in the builds? If so, why?
_If_ the default import pulls in extra dependencies, then yes it does (your library will be rebuilt more often than it should), and your application will be larger than it should.
There is also a more serious issue of possibly pulling in deps that aren't supported on the current platform, which is the primary thing we use config specific imports to avoid.
However, that isn't what did implement, and as far as I know there are no plans to implement that in the future (it can't happen until dart 3.0 in any case....).
I think it would be beneficial for one or both of you to file a request to the language team (dart-lang/language/issues/new
), AFAICT configurable imports are not yet in wide use, so if there are changes we can make in Dart 3.0.0 that significantly make them better/faster, we should request those changes.
There is also a more serious issue of possibly pulling in deps that aren't supported on the current platform, which is the primary thing we use config specific imports to avoid.
fair point! I would hope the "default" one doesn't depend on a platform (otherwise it wouldn't work great for Dart Analyzer). It's certainly not supposed to be platform specific.
I think it would be beneficial for one or both of you to file a request to the language team (dart-lang/language/issues/new), AFAICT configurable imports are not yet in wide use, so if there are changes we can make in Dart 3.0.0 that significantly make them better/faster, we should request those changes.
Yeah that's an option. Personally I think we landed in an okay place already: language team approved that approach, we implemented Phase 0, did not implement Phase 1 yet. Analyzer folks have been/are quite busy with CFE and such, so it made sense to defer the static checking work until later.
Anyways, if Analyzer folks want to change this to skip verification that the "interface" library file exists, it would be a fairly simple change to Analyzer's LibraryAnalyzer (to the methods I mentioned in a comment above). I'll leave that to them.
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.
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.
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?
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.
@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
?
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);
}
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.
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 importsdart: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).
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?
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.
Why isn't supplying
default.dart
a solution?
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.
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.
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 https://github.com/dart-lang/build/pull/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.
@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?
@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
orweb.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! ;-)
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.
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.
+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.
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.
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.
Most helpful comment
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.