Make dart2js not consider dart:io there if the library is unsupported.

Make dart2js not consider dart:io there if the library is unsupported.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/bcdf0a1f125816b97590734370a9e6ef99a10881

[Lasse Reichstein Nielsen, 2016-09-08 12:16:25]

lrn@google.com changed reviewers:
+ floitsch@google.com, johnniwinther@google.com, sigmund@google.com

[Lasse Reichstein Nielsen, 2016-09-08 12:16:26]

I have no idea how to test this, but it fixes my problem.

[Siggi Cherem (dart-lang), 2016-09-08 15:47:09]

The change in apiimpl looks good to me.

For testing, we can add this as a language test or a dart2js_extra test:

import "package:expect/expect.dart";

/// Whether importing dart:io is supported
const ioSupported = const bool.fromEnvironment("dart.library.io");

/// Whether we are running in the vm or in dart2js. To detect this, we exploit
/// here the fact that number semantics are different in the two systems.
const runningInVm = !identical(1.0, 1);
main() {
  Expect.equals(runningInVm, ioSupported);
}

https://codereview.chromium.org/2321973002/diff/1/pkg/compiler/lib/src/apiimp...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/2321973002/diff/1/pkg/compiler/lib/src/apiimp...
pkg/compiler/lib/src/apiimpl.dart:391: if
(compiler.resolvedUriTranslator.sdkLibraries[libraryName].scheme ==
nit: consider loading the uri once and share that here and above in line 384,
for example:

Uri location = compiler.resolvedUriTranslator.sdkLibraries[libraryName];
// Or, actually, the equivalent and possibly more readable call:
Uri location = compiler.resolvedUriTranslator.lookupLibraryUri(libraryName);

if (location != null) {
  ... // check for mirrors
  if (location.scheme == 'unsupported') return null;
}

[Lasse Reichstein Nielsen, 2016-09-12 06:56:35]

On 2016/09/08 15:47:09, Siggi Cherem (dart-lang) wrote:
> The change in apiimpl looks good to me.
> 
> For testing, we can add this as a language test or a dart2js_extra test:
> 
> import "package:expect/expect.dart";
> 
> /// Whether importing dart:io is supported
> const ioSupported = const bool.fromEnvironment("dart.library.io");
> 
> /// Whether we are running in the vm or in dart2js. To detect this, we exploit
> /// here the fact that number semantics are different in the two systems.
> const runningInVm = !identical(1.0, 1);
> main() {
>   Expect.equals(runningInVm, ioSupported);
> }

That would fail in Dartium, but as a dart2js test it would work. OTOH, then
there is no need to test for the VM.

I'm thinking something like:

import "this_test.dart"
    if (dart.library.io) "dart:io"
    if (dart.library.html) "dart:html" 
    deferred as d show HttpRequest;  // All three libraries have an HttpRequest
class.

class HttpRequest {}

main() {
  asyncStart();  
  () async {
    var io = const bool.fromEnvironment("dart.library.io");
    var html = const bool.fromEnvironment("dart.library.html");
  
    await d.loadLibrary();  // Shouldn't fail.
    if (io) {
      Expect.throws(() => new d.HttpRequest());  // Class is abstract in dart:io
    } else if (html) {
      dynamic r = new d.HttpRequest();  // Shouldn't throw
      var o = r.open;  // Shouldn't fail, the open method is there.
    } else {
      dynamic r = new d.HttpRequest();
      Expect.isTrue(r is HttpRequest);
    } 
    asyncEnd();   
  }
}

[Lasse Reichstein Nielsen, 2016-09-12 08:51:16]

https://codereview.chromium.org/2321973002/diff/1/pkg/compiler/lib/src/apiimp...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/2321973002/diff/1/pkg/compiler/lib/src/apiimp...
pkg/compiler/lib/src/apiimpl.dart:391: if
(compiler.resolvedUriTranslator.sdkLibraries[libraryName].scheme ==
I was considering doing that, but I decided to change as little as possible. So,
will do.

[Lasse Reichstein Nielsen, 2016-09-12 12:38:26]

PTAL

[Siggi Cherem (dart-lang), 2016-09-12 15:53:40]

changes to dart2js lgtm, just a few suggestions below for the tests

https://codereview.chromium.org/2321973002/diff/40001/tests/language/conditio...
File tests/language/conditional_import_string_test.dart (right):

https://codereview.chromium.org/2321973002/diff/40001/tests/language/conditio...
tests/language/conditional_import_string_test.dart:9: import
"conditional_import_test.dart"
did you mean to import conditional_import_string_test.dart here? (otherwise line
33 below would fail, right?)

https://codereview.chromium.org/2321973002/diff/40001/tests/language/conditio...
tests/language/conditional_import_string_test.dart:23: if (io) {
I fear that the use of these constants and the imports above are too correlated:
it gives me less assurance that we have full test coverage. In particular,
suppose if we had a bug in the VM and we never define dart.library.io as true.
This test would always pass.

Some ideas:
 - use more than 1 test (or a multi-line test), so we can use the .status files
to mark one the correct expectations on each embedder.
 - to keep things simpler, I'd use `dart.library.io/.html` but not use the
actual dart:io/dart:html libraries. 

Here is a suggestion:

  default.dart:
    final x = "def";

  io.dart:
    final x = "vm";

  html.dart:
    final x = "browser";

  conditional_import_test.dart:
    import 'default.dart'
      if dart.library.io 'io.dart'
      if dart.library.html 'html.dart'
      show x;

    main() {
      // The following check is like the test you had before, we are validating
      // that the constants in the code and in the imports are evaluated
      // the same way.
      var io = const bool.fromEnvironment("dart.library.io");
      var html = const bool.fromEnvironment("dart.library.html");
      Expect(d.x, io ? "vm" : (html ? "browser : "def"));

      // Here we test each scenario/embedder directly
      Expect(d.x, "def");     // default: OK
      Expect(d.x, "vm");      // standalone: OK
      Expect(d.x, "browser"); // browser: OK
    }

Then we can mark the status file like:

[ $runtime == vm ]
conditional_import_test/default: Fail, OK
conditional_import_test/browser: Fail, OK

[ $browser ]
conditional_import_test/default: Fail, OK
conditional_import_test/standalone: Fail, OK

Thoughts?

[Lasse Reichstein Nielsen, 2016-09-12 17:58:15]

https://codereview.chromium.org/2321973002/diff/40001/tests/language/conditio...
File tests/language/conditional_import_string_test.dart (right):

https://codereview.chromium.org/2321973002/diff/40001/tests/language/conditio...
tests/language/conditional_import_string_test.dart:9: import
"conditional_import_test.dart"
Ack, yes.

https://codereview.chromium.org/2321973002/diff/40001/tests/language/conditio...
tests/language/conditional_import_string_test.dart:23: if (io) {
On 2016/09/12 15:53:39, Siggi Cherem (dart-lang) wrote:
> I fear that the use of these constants and the imports above are too
correlated:
> it gives me less assurance that we have full test coverage. In particular,
> suppose if we had a bug in the VM and we never define dart.library.io as true.
> This test would always pass.

True. This was intended as a regression test for having false positives, like
dart2js having dart.library.io being true. If the platform says it's there, it
should be loadable.

It's not testing that it won't be loaded if the value is false.
(That's also slightly harder - it would depend on the loadLibrary call to throw
asynchronously).


> 
> Some ideas:
>  - use more than 1 test (or a multi-line test), so we can use the .status
files
> to mark one the correct expectations on each embedder.
>  - to keep things simpler, I'd use `dart.library.io/.html` but not use the
> actual dart:io/dart:html libraries. 

That wouldn't catch the error I'm fixing - that dart2js said dart.library.io was
true, but dart:io wasn't loadable.

> 
> Here is a suggestion:
> 
>   default.dart:
>     final x = "def";
> 
>   io.dart:
>     final x = "vm";
> 
>   html.dart:
>     final x = "browser";
> 
>   conditional_import_test.dart:
>     import 'default.dart'
>       if dart.library.io 'io.dart'
>       if dart.library.html 'html.dart'
>       show x;
> 
>     main() {
>       // The following check is like the test you had before, we are
validating
>       // that the constants in the code and in the imports are evaluated
>       // the same way.
>       var io = const bool.fromEnvironment("dart.library.io");
>       var html = const bool.fromEnvironment("dart.library.html");
>       Expect(d.x, io ? "vm" : (html ? "browser : "def"));

That just shows that the environment agrees with the configured import, which it
did. It was just wrong because dart:io wasn't actually loadable.

> 
>       // Here we test each scenario/embedder directly
>       Expect(d.x, "def");     // default: OK
>       Expect(d.x, "vm");      // standalone: OK
>       Expect(d.x, "browser"); // browser: OK
>     }
> 
> Then we can mark the status file like:
> 
> [ $runtime == vm ]
> conditional_import_test/default: Fail, OK
> conditional_import_test/browser: Fail, OK
> 
> [ $browser ]
> conditional_import_test/default: Fail, OK
> conditional_import_test/standalone: Fail, OK
> 
> Thoughts?

It's a valid test of configurable imports, but it's not what I'm trying to test
here - I am actually trying to check that the environment and the availability
of the platform library agrees (at least positively, I'm not checking that I
can't load it if the value is false - the compiler won't let me unconditionally
import dart:io when compiled with dart2js, not even deferred).

[Siggi Cherem (dart-lang), 2016-09-12 19:13:45]

I see - thanks for clarifying. LGTM

[Lasse Reichstein Nielsen, 2016-09-13 08:27:19]

Description was changed from

==========
Make dart2js not consider dart:io there if the library is unsupported.
==========

to

==========
Make dart2js not consider dart:io there if the library is unsupported.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/bcdf0a1f125816b97590734370a9e6ef99a10881
==========

[Lasse Reichstein Nielsen, 2016-09-13 08:27:22]

Committed patchset #4 (id:60001) manually as
bcdf0a1f125816b97590734370a9e6ef99a10881 (presubmit successful).