Remove factory body in *.fromEnvironment, and implement this same behavior

Remove factory body in *.fromEnvironment, and implement this same behavior
directly inside dart2js.

A few notes:
  * I ended up adding support for this feature also in dart2js with --use-kernel, it was not implemented correctly before. This is why more tests are passing.

  * I played with two other ideas:
    (a) making this a compile-time error instead
    (b) making it a warning if the arguments were constant and implement it as a constant, treat it as an error if they were not.

Unfortunately both are a breaking changes, so I backed out from them. IMO (a) is the cleanest thing to do long term, and we should raise this with the language team.

R=efortuna@google.com

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

[Siggi Cherem (dart-lang), 2017-05-24 17:39:21]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2017-05-24 17:59:27]

Description was changed from

==========
Remove factory body in *.fromEnvironment, and implement this same behavior
directly inside dart2js.

Note: I also played with two ideas:
 - make this a compile-time error: honestly this is likely the cleanest thing to
 do, but it is a breaking change, so I backed out from it.  warning and treat
 - make this a warning and resolve `new *.fromEnvironment` as a const
 constructor. This was more hacky, it had the potential of avoiding a runtime
 failure if the argument to the constructor happened to be a constant. The end
 result is that we also had a breaking change when arguments were not constant,
 so it was not worth it.

The fix was small enough that I lean towards the solution I have here.
==========

to

==========
Remove factory body in *.fromEnvironment, and implement this same behavior
directly inside dart2js.

A few notes:
  * I ended up adding support for this feature also in dart2js with
--use-kernel, it was not implemented correctly before. This is why more tests
are passing.

  * I played with two other ideas:
    (a) making this a compile-time error instead
    (b) making it a warning if the arguments were constant and implement it as a
constant, treat it as an error if they were not.


(a) is the cleanest thing to do long term, and we should revisit this.
Unfortunately both (a) and (b) make it a breaking change, so I backed out from
it.
==========

[Siggi Cherem (dart-lang), 2017-05-24 18:00:58]

Description was changed from

==========
Remove factory body in *.fromEnvironment, and implement this same behavior
directly inside dart2js.

A few notes:
  * I ended up adding support for this feature also in dart2js with
--use-kernel, it was not implemented correctly before. This is why more tests
are passing.

  * I played with two other ideas:
    (a) making this a compile-time error instead
    (b) making it a warning if the arguments were constant and implement it as a
constant, treat it as an error if they were not.


(a) is the cleanest thing to do long term, and we should revisit this.
Unfortunately both (a) and (b) make it a breaking change, so I backed out from
it.
==========

to

==========
Remove factory body in *.fromEnvironment, and implement this same behavior
directly inside dart2js.

A few notes:
  * I ended up adding support for this feature also in dart2js with
--use-kernel, it was not implemented correctly before. This is why more tests
are passing.

  * I played with two other ideas:
    (a) making this a compile-time error instead
    (b) making it a warning if the arguments were constant and implement it as a
constant, treat it as an error if they were not.

Unfortunately both are a breaking changes, so I backed out from them. IMO (a) is
the cleanest thing to do long term, and we should raise this with the language
team.
==========

[Siggi Cherem (dart-lang), 2017-05-24 18:01:05]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2017-05-24 18:06:16]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2017-05-24 18:08:14]

sigmund@google.com changed reviewers:
+ johnniwinther@google.com, sra@google.com

[Siggi Cherem (dart-lang), 2017-05-24 18:08:15]

/cc rnystrom - Bob, I believe you might have done some special support in DDC
for these const-factory-external cosntructors with a body. We might need to do a
similar fix there in the near future.

https://codereview.chromium.org/2896393003/diff/60001/tests/language/language...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/2896393003/diff/60001/tests/language/language...
tests/language/language_dart2js.status:317: black_listed_test/13c: Crash # Issue
27394
these seemed to be failing from before, not sure why they weren't covered by the
bots.

[Bob Nystrom, 2017-05-24 19:52:08]

rnystrom@google.com changed reviewers:
+ rnystrom@google.com

[Bob Nystrom, 2017-05-24 19:52:09]

https://codereview.chromium.org/2896393003/diff/60001/pkg/compiler/lib/src/di...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/2896393003/diff/60001/pkg/compiler/lib/src/di...
pkg/compiler/lib/src/diagnostics/messages.dart:1088: "abstract class A {} main()
{ new bool.fromEnvironment('X'); }"
What's the "abstract class A {}" part for?

[Siggi Cherem (dart-lang), 2017-05-24 20:37:19]

https://codereview.chromium.org/2896393003/diff/60001/pkg/compiler/lib/src/di...
File pkg/compiler/lib/src/diagnostics/messages.dart (right):

https://codereview.chromium.org/2896393003/diff/60001/pkg/compiler/lib/src/di...
pkg/compiler/lib/src/diagnostics/messages.dart:1088: "abstract class A {} main()
{ new bool.fromEnvironment('X'); }"
On 2017/05/24 19:52:09, Bob Nystrom wrote:
> What's the "abstract class A {}" part for?

Thanks for catching that, I copied code form a different example, modified main
and missed the extra class definition. There is no need for it.

[Siggi Cherem (dart-lang), 2017-05-25 21:34:24]

sigmund@google.com changed reviewers:
+ efortuna@google.com

[Emily Fortuna, 2017-05-26 18:04:02]

lgtm

https://codereview.chromium.org/2896393003/diff/80001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/members.dart (right):

https://codereview.chromium.org/2896393003/diff/80001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3855: // TODO(sigmund): consider
turning this into a compile-time-error.
why isn't this a compile time error?

[Siggi Cherem (dart-lang), 2017-05-26 18:08:55]

On 2017/05/26 18:04:02, Emily Fortuna wrote:
> lgtm
> 
>
https://codereview.chromium.org/2896393003/diff/80001/pkg/compiler/lib/src/re...
> File pkg/compiler/lib/src/resolution/members.dart (right):
> 
>
https://codereview.chromium.org/2896393003/diff/80001/pkg/compiler/lib/src/re...
> pkg/compiler/lib/src/resolution/members.dart:3855: // TODO(sigmund): consider
> turning this into a compile-time-error.
> why isn't this a compile time error?

Mainly it needs a decision from the language team to do so. To that extent,
maybe we shouldn't report a warning but a hint instead, since the spec doesn't
indicate that this should be a warning either.

It is supported by the VM, and technically we could support it if it shipped the
environment variables together with the app. So there is a way to make it not an
error. I don't think there is any need for that though.

[Siggi Cherem (dart-lang), 2017-05-26 21:49:25]

Description was changed from

==========
Remove factory body in *.fromEnvironment, and implement this same behavior
directly inside dart2js.

A few notes:
  * I ended up adding support for this feature also in dart2js with
--use-kernel, it was not implemented correctly before. This is why more tests
are passing.

  * I played with two other ideas:
    (a) making this a compile-time error instead
    (b) making it a warning if the arguments were constant and implement it as a
constant, treat it as an error if they were not.

Unfortunately both are a breaking changes, so I backed out from them. IMO (a) is
the cleanest thing to do long term, and we should raise this with the language
team.
==========

to

==========
Remove factory body in *.fromEnvironment, and implement this same behavior
directly inside dart2js.

A few notes:
  * I ended up adding support for this feature also in dart2js with
--use-kernel, it was not implemented correctly before. This is why more tests
are passing.

  * I played with two other ideas:
    (a) making this a compile-time error instead
    (b) making it a warning if the arguments were constant and implement it as a
constant, treat it as an error if they were not.

Unfortunately both are a breaking changes, so I backed out from them. IMO (a) is
the cleanest thing to do long term, and we should raise this with the language
team.

R=efortuna@google.com

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

[Siggi Cherem (dart-lang), 2017-05-26 21:49:26]

Committed patchset #2 (id:80001) manually as
dd70d8b655bc1187f942779dd5455fab1f2f8045 (presubmit successful).