Implement fromEnvironment on bool, int, String in dart2js.

Implement fromEnvironment on bool, int, String in dart2js.

R=johnniwinther@google.com, kasperl@google.com

Committed: https://code.google.com/p/dart/source/detail?r=29675

[ahe, 2013-10-30 13:03:08]

DBC

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:179: Match m = new
RegExp('^-D(.+)=(.+)').firstMatch(argument);
Terminate with $.

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:504: Supported options:
Please update the documentation.

[ngeoffray, 2013-10-30 13:08:05]

+rico and martin for the test script change.

[Lasse Reichstein Nielsen, 2013-10-30 13:34:48]

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:179: Match m = new
RegExp('^-D(.+)=(.+)').firstMatch(argument);
Terminating isn't necessary. The last capture is greedt.
Consider not using "." since command line parts can contain newlines and '.'
doesn't match newlines.
For a regexp, I'd recommend
   /^-D([^=])+=/
and then do
 String name = m[1];
 String value = argument.substring(m.end);
Anything after the equals is valid, so there is no need to check it.

Or better yet, avoid regexps entirely (then you only have one problem!):
 if (argument.startsWith("-D")) {
   int eqIndex = argument.indexOf("=", 2);
   if (eqIndex >= 0) {
     String name = argument.substring(2, eqIndex);
     String value = argument.substring(eqIndex + 1);
     .... do stuff ...
   } else {
    // error, no = on the line.
  }

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:183: environment[name] =
true;
Why parse the arguments here?
Even if the value is "true", it may be wanted as a string.
For integers, parsing can be wrong. If the argument is "-Dfoo=0x1a", it might
not be intended as an integer, and const String.fromEnvironment("foo") should
return "0x1a".

In other words: The environment can be plain String->String. The constructors
add the interpretation, not the environment itself.

https://codereview.chromium.org/48323003/diff/30001/tests/corelib/string_from...
File tests/corelib/string_from_environment_test.dart (right):

https://codereview.chromium.org/48323003/diff/30001/tests/corelib/string_from...
tests/corelib/string_from_environment_test.dart:11: Expect.equals('ccc', const
String.fromEnvironment('c'));
Do mixed tests as well:

With -Da=true -Db=0x10

Expect.equals('true', const String.fromEnvironment('a'));
Expect.equals(true, const bool.fromEnvironment('a'));
Expect.equals('0x10', const String.fromEnvironment('b'));
Expect.equals(16, const int.fromEnvironment('b'));
Expect.equals(false, const bool.fromEnvironment('b'));

[Lasse Reichstein Nielsen, 2013-10-30 13:34:48]

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:179: Match m = new
RegExp('^-D(.+)=(.+)').firstMatch(argument);
Terminating isn't necessary. The last capture is greedt.
Consider not using "." since command line parts can contain newlines and '.'
doesn't match newlines.
For a regexp, I'd recommend
   /^-D([^=])+=/
and then do
 String name = m[1];
 String value = argument.substring(m.end);
Anything after the equals is valid, so there is no need to check it.

Or better yet, avoid regexps entirely (then you only have one problem!):
 if (argument.startsWith("-D")) {
   int eqIndex = argument.indexOf("=", 2);
   if (eqIndex >= 0) {
     String name = argument.substring(2, eqIndex);
     String value = argument.substring(eqIndex + 1);
     .... do stuff ...
   } else {
    // error, no = on the line.
  }

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:183: environment[name] =
true;
Why parse the arguments here?
Even if the value is "true", it may be wanted as a string.
For integers, parsing can be wrong. If the argument is "-Dfoo=0x1a", it might
not be intended as an integer, and const String.fromEnvironment("foo") should
return "0x1a".

In other words: The environment can be plain String->String. The constructors
add the interpretation, not the environment itself.

https://codereview.chromium.org/48323003/diff/30001/tests/corelib/string_from...
File tests/corelib/string_from_environment_test.dart (right):

https://codereview.chromium.org/48323003/diff/30001/tests/corelib/string_from...
tests/corelib/string_from_environment_test.dart:11: Expect.equals('ccc', const
String.fromEnvironment('c'));
Do mixed tests as well:

With -Da=true -Db=0x10

Expect.equals('true', const String.fromEnvironment('a'));
Expect.equals(true, const bool.fromEnvironment('a'));
Expect.equals('0x10', const String.fromEnvironment('b'));
Expect.equals(16, const int.fromEnvironment('b'));
Expect.equals(false, const bool.fromEnvironment('b'));

[kasperl, 2013-10-30 13:35:24]

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:665: if
(value == null) {
Not sure I understand why we want to be so forgiving here. If the environment
contains something weird we just use the default value (except for bool which
seems to not have a default value). I'm afraid this is going to lead to some
pretty nasty hard to debug issue. Why not just throw a compile-time error? It's
basically a wrong constant.

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:666: if
(constructor == compiler.boolEnvironment) {
I'd add a few comments here to explain the behavior.

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:670:
return arguments[1];
Add local variable for arguments[1] and call it defaultValue?

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:860: new
Selector.callConstructor('fromEnvironment', null, 1));
Again, I don't really see why bool.fromEnvironment shouldn't allow a default
value.

[kustermann, 2013-10-30 13:49:06]

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:777: }
If you do it here, do it for dart2dart below as well.

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:826: }
There have to be other places where we setup the options for
content_shell/dartium.
Could you add this there as well?

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:1457: 'More than one "// DartOptions=" line
in test $filePath');
DartOptions => SharedOptions

[kasperl, 2013-10-30 13:49:56]

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:183: environment[name] =
true;
On 2013/10/30 13:34:48, Lasse Reichstein Nielsen wrote:
> Why parse the arguments here?
> Even if the value is "true", it may be wanted as a string.
> For integers, parsing can be wrong. If the argument is "-Dfoo=0x1a", it might
> not be intended as an integer, and const String.fromEnvironment("foo") should
> return "0x1a".
> 
> In other words: The environment can be plain String->String. The constructors
> add the interpretation, not the environment itself.

I agree with Lasse here. It seems cleaner to do the interpretation later.

[ricow1, 2013-10-30 13:51:19]

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:797: args = new List.from(args);
we may need to add support for dart2dart as well

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:1457: 'More than one "// DartOptions=" line
in test $filePath');
DartOptions -> SharedOptions

[Søren Gjesse, 2013-10-30 13:59:13]

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:665: if
(value == null) {
In the VM implementation we throw an error if the value from the embedder is not
String or Null.

https://codereview.chromium.org/48323003/diff/30001/tests/corelib/string_from...
File tests/corelib/string_from_environment_test.dart (right):

https://codereview.chromium.org/48323003/diff/30001/tests/corelib/string_from...
tests/corelib/string_from_environment_test.dart:11: Expect.equals('ccc', const
String.fromEnvironment('c'));
Updated tests in https://codereview.chromium.org/50983002

[ngeoffray, 2013-10-30 14:39:48]

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:777: }
On 2013/10/30 13:49:07, kustermann wrote:
> If you do it here, do it for dart2dart below as well.

Done.

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:797: args = new List.from(args);
On 2013/10/30 13:51:19, ricow1 wrote:
> we may need to add support for dart2dart as well

Done.

https://codereview.chromium.org/48323003/diff/30001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:826: }
On 2013/10/30 13:49:07, kustermann wrote:
> There have to be other places where we setup the options for
> content_shell/dartium.
> Could you add this there as well?

As discussed, we don't want to test this in drt/dartium.

[ngeoffray, 2013-10-30 15:13:58]

Thanks for all the comments. PTAL.

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:670:
return arguments[1];
On 2013/10/30 13:35:25, kasperl wrote:
> Add local variable for arguments[1] and call it defaultValue?

Done.

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:860: new
Selector.callConstructor('fromEnvironment', null, 1));
On 2013/10/30 13:35:25, kasperl wrote:
> Again, I don't really see why bool.fromEnvironment shouldn't allow a default
> value.

Done.

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:179: Match m = new
RegExp('^-D(.+)=(.+)').firstMatch(argument);
On 2013/10/30 13:34:48, Lasse Reichstein Nielsen wrote:
> Terminating isn't necessary. The last capture is greedt.
> Consider not using "." since command line parts can contain newlines and '.'
> doesn't match newlines.
> For a regexp, I'd recommend
>    /^-D([^=])+=/
> and then do
>  String name = m[1];
>  String value = argument.substring(m.end);
> Anything after the equals is valid, so there is no need to check it.
> 
> Or better yet, avoid regexps entirely (then you only have one problem!):
>  if (argument.startsWith("-D")) {
>    int eqIndex = argument.indexOf("=", 2);
>    if (eqIndex >= 0) {
>      String name = argument.substring(2, eqIndex);
>      String value = argument.substring(eqIndex + 1);
>      .... do stuff ...
>    } else {
>     // error, no = on the line.
>   }
> 
>  

Done.

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:183: environment[name] =
true;
On 2013/10/30 13:34:48, Lasse Reichstein Nielsen wrote:
> Why parse the arguments here?
> Even if the value is "true", it may be wanted as a string.
> For integers, parsing can be wrong. If the argument is "-Dfoo=0x1a", it might
> not be intended as an integer, and const String.fromEnvironment("foo") should
> return "0x1a".
> 
> In other words: The environment can be plain String->String. The constructors
> add the interpretation, not the environment itself.

Done.

https://codereview.chromium.org/48323003/diff/30001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart2js.dart:504: Supported options:
On 2013/10/30 13:03:09, ahe wrote:
> Please update the documentation.

Done.

[Søren Gjesse, 2013-10-30 15:21:48]

https://codereview.chromium.org/48323003/diff/240001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/48323003/diff/240001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:689:
return (number == null)
Re-reading the specification non-parseable integer returns the default value, so
this is the right thing I guess. The VM implementation throws in this case, but
I can change that.

[Lasse Reichstein Nielsen, 2013-10-31 06:58:19]

https://codereview.chromium.org/48323003/diff/240001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/48323003/diff/240001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:688: int
number = int.parse(value, onError: (_) => null);
It should be fine to just do:
  return constantSystem.createInt(
      int.parse(value, onError: (_) => defaultValue));

https://codereview.chromium.org/48323003/diff/240001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:689:
return (number == null)
I'm open to changing it. I was working on the assumption that we wanted as few
errors as possible.

[ngeoffray, 2013-10-31 07:41:18]

https://codereview.chromium.org/48323003/diff/240001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/48323003/diff/240001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:688: int
number = int.parse(value, onError: (_) => null);
On 2013/10/31 06:58:20, Lasse Reichstein Nielsen wrote:
> It should be fine to just do:
>   return constantSystem.createInt(
>       int.parse(value, onError: (_) => defaultValue));

Done.

[Johnni Winther, 2013-10-31 08:14:20]

lgtm

[kasperl, 2013-10-31 09:11:51]

LGTM.

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:693:
return (value == 'true')
return constantSystem.createBool(value == 'true');

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart2js.dart:507: Define an
environment variable.
Maybe we need to update this description. I think we're leaning towards calling
these properties or something like it.

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/inferrer/type_graph_nodes.dart
(right):

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/type_graph_nodes.dart:304:
giveUp(inferrer);
Why do you give up in the int and bool case, but not for strings? That at least
deserves a comment.

[ngeoffray, 2013-10-31 09:32:23]

Thanks Kasper.

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:693:
return (value == 'true')
On 2013/10/31 09:11:51, kasperl wrote:
> return constantSystem.createBool(value == 'true');

Done.

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/inferrer/type_graph_nodes.dart
(right):

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/type_graph_nodes.dart:304:
giveUp(inferrer);
On 2013/10/31 09:11:51, kasperl wrote:
> Why do you give up in the int and bool case, but not for strings? That at
least
> deserves a comment.

Good catch. I should actually give up for the string case as well. Not giving up
will not make the analysis unsound, but will potentially analyze this node again
and again.

[kasperl, 2013-10-31 12:33:05]

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:693:
return (value == 'true')
On 2013/10/31 09:32:24, ngeoffray wrote:
> On 2013/10/31 09:11:51, kasperl wrote:
> > return constantSystem.createBool(value == 'true');
> 
> Done.

I just discussed this with Lars and we'd prefer to change this to give the
defaultValue if value isn't true or false. We also need to fix this in the VM.

[ngeoffray, 2013-10-31 12:53:17]

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/48323003/diff/430001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:693:
return (value == 'true')
On 2013/10/31 12:33:05, kasperl wrote:
> On 2013/10/31 09:32:24, ngeoffray wrote:
> > On 2013/10/31 09:11:51, kasperl wrote:
> > > return constantSystem.createBool(value == 'true');
> > 
> > Done.
> 
> I just discussed this with Lars and we'd prefer to change this to give the
> defaultValue if value isn't true or false. We also need to fix this in the VM.

Done.

[ngeoffray, 2013-10-31 14:41:56]

Committed patchset #11 manually as r29675 (presubmit successful).