[Breaking change request] String.fromEnvironment and int.fromEnvironment const constructors will have a non-null defaultValue default value.

[lrhn]

Summary

The String.fromEnvironment and int.fromEnvironment const constructors will have a non-null defaultValue default value.

What is changing:

The String.fromEnvironment and int.fromEnvironment constructors both have a defaultValue optional parameter which has a default value of null.
This is changed to the empty string and the integer zero respectively.

Why is this changing?

These are factory constructors, and in Null Safe code, constructors cannot return null.
As such, having a default value of null will not work. We picked the most neutral value for the types as the new default value.

The bool.fromEnvironment constructor is unaffected because it already had false as default defaultValue.

Expected impact

Code which uses either constructor with no specified default value will get a different result if there is no declaration for the requested key.

If code checks for the presence of a key by checking whether String.fromEnvironment(key) evaluates to null, it will now fail.

Code like const int.fromEnvironment("port") ?? computedPort() will no longer work. It can't just use computerPort() as default value because the default value for a constant constructor invocation must be constant.

The impact can be large for code which uses the current feature, but it is not expected that the feature is used that much.

Checking whether a key is present can be done by something like:

  const bool keyPresent = String.fromEnvironment(key) != String.fromEnvironment(key, "_");

which is not as convenient.

Allowing a factory constructor to return null is not a viable choice in Null Safe code.
Changing the constructors to be something other than const constructors, but keep them being constant, would require adding such a feature because the language currently doesn't have any.

[franklinyow]

cc @Hixie @matanlurey @dgrove @vsmenon for review and approval.

[Hixie]

This seems like an undesireable change. String.fromEnvironment in particular seems like it should be able to distinguish a present empty value from an absent one, and Flutter does rely on this feature in some of its code (e.g. devicelab/lib/framework/utils.dart).

Could these factory methods be defined as factory? String.fromEnvironment() or some such to indicate that they can return null?

[lrhn]

The reason to have factory methods in Dart is to be able to switch between generative constructors and factory constructors without breaking clients instantiating the class^*. As such, providing a feature (nullable return type) which only works on one of them is not desirable.
The two fromEnvironment constructors have been the only two cases needing it so far, so adding a general feature just for those two does not seem worth the effort.

That is, this change is not made to improve the API, but to make it possible at all in a Null Safe setting. It's not something we can choose not to change. Either we need to add more features to the language (with a questionable cost/value ratio) or change the constructors in some other way, but we can't return null from a constructor in Null Safe code with the current Null Safe design.

The other option we have found is to make defaultValue a required parameter. That would make the API harder to use in almost all cases, and it would not provide any extra value. You can still always provide a defaultValue and get exactly the same behavior. With a default value of empty string/0, you can also omit the defaultValue parameter and get something which is a reasonable default in many cases.

If you want to distinguish an absent value from the default value, you still can do so precisely as described above(even if it's cumbersome and not something you can abstract over), or you can use a slightly less precise check like:

const String flag = String.fromEnvironment("flag key", defaultValue: "<NOT PRESENT>");
if (flag == "<NOT PRESENT>") { ... assume it wasn't there ... }
const int level = int.fromEnvironment("level key", defaultValue: -999999);
if (level == -999999) { ... assume it wasn't there ... }

These tests can be foiled, but in practice that will only happen with malicious intent, and the only effect the user gets is to act as if no value was passed, which the code should be ready to handle anyway.

So, there are workarounds.

The code linked in the Flutter utils.dart file is already questionable. It's unclear how the code would react to an empty string value, but it is possible to pass one as -DlocalEngine=.

The code would just need to change the absent-check for .isEmpty instead of == null, and it would have the added advantage of not trying to pass an empty string as a command line argument.

I believe that in most cases, an empty value can be treated the same as an absent key without any actual loss of user power because an empty value is already a questionable input. (A boolean declaration should use =true, not an empty string).

About allowing nullable return values from factories: Adding return types to constructors in general is a personal wish of mine, but not something we can do in the scope of the NNBD change. That would, if it ever happens, likely be part of a major revamp of constructors in general. Not sure whether it would actually allow you to return null, or if it must still be a subtype of the class type (but async constructors have been requested, so there is that).

^*: It may still break classes extending the class, so the abstraction isn't pure. It would be better if we could define generative constructors which could not be used for super-class constructor invocations.

[Hixie]

The reason to have factory methods in Dart is to be able to switch between generative constructors and factory constructors without breaking clients instantiating the class

Ever since we got rid of new, that use case sort of went away. Nowadays I don't think there's really any strong motivation left for factory methods, they're just weird syntax for static methods as far as I can tell. (Except for redirecting constructors, but it's weird that those get labeled "factory" anyway, we could identify those just by the "=" syntax.)

These tests can be foiled, but in practice that will only happen with malicious intent

That seems like a pretty terrible place to be in terms of API design. We shouldn't need to hope there's no malicious intent for our APIs to work. (What if the environment variable comes from the network, e.g. HTTP_xxx in a CGI context? Then it really could be malicious.)

It's unclear how the code would react to an empty string value, but it is possible to pass one as -DlocalEngine=

Why is it unclear? It seems pretty clear that that should be treated as an explicit local engine whose value is the empty string. Sure, it's unlikely to be useful, but there's plenty of strings that are unlikely to be useful.

To be honest, it seems a bit weird that we're saying factory methods can't return null any more. I guess in general we want people to switch to real static methods if they have that use case, and the only reason that doesn't work here is that we want to encourage people to use const with these constructors?

In any case, these constructors are already highly magical (they're const factory constructors with external bodies, which is not a thing a normal Dart program can express!). I recommend we just make them more magical for now, allowing them to continue to return null, until the day we return to environment variables in Dart in general to address them more rationally.

[matanlurey]

LGTM in terms of changing this particular API (and making factory methods return non-null).

[lrhn]

One option we have for making migration easier is to add a bool.hasEnvironment(String key) constructor too. Then you can write bool.hasEnvironment("key") ? String.fromEnvironment("key") : null if you want to keep a null for the non-existing case.

[vsmenon]

lgtm assuming we can land this without breaking.

The fromEnvironment constructors are somewhat magical, so I think it'd be reasonable to special case those as well if needed. (I.e., to make factories non-nullable in general.)

The reason to have factory methods in Dart is to be able to switch between generative constructors and factory constructors without breaking clients instantiating the class

Is this actually true for const constructors?

[lrhn]

Is this actually true for const constructors?

Yes. A class can have a const generative constructor, then change it to a const factory constructor which forwards to a subclass const constructor. As long as noone uses it as a superclass constructor, that still works. And if someone uses it as a superclass constructor, you're stuck with it, which is a hole in the entire theory.

(I prefer to only expose factory constructors where possible, to prevent people from extending classes not intended for extending).

[leafpetersen]

Where are we with this? I am fairly strongly opposed to adding an ad hoc "nullable" factory constructor at this point. If we want this feature, I think it should be part of a broader language feature around constructors with explicit return types.

If we want to special case the constructors for now, what I might suggest is that we specify that the expression const xxx.fromEnvironment(string) is syntactic sugar for const bool.hasEnvironment(string) ? xxx.fromEnvironment(string) : null for the appropriate values of xxx.

If we take that approach, should we consider having a non-nullable version as well, so that the 99% case that doesn't want to get null from xxx.fromEnvironment doesn't have to deal with it?

[leafpetersen]

cc @johnniwinther @stereotype441 @scheglov for any concerns about the syntactic sugar approach I mentioned above.

[scheglov]

@leafpetersen I don't have concerns about this approach and analyzer.

[munificent]

Somewhat tangential to this issue, but...

Nowadays I don't think there's really any strong motivation left for factory methods, they're just weird syntax for static methods as far as I can tell.

The reason to have factory methods in Dart is to be able to switch between generative constructors and factory constructors without breaking clients instantiating the class

The other reason for factory constructors is that they are a more natural way to create instances of generic classes. Compare:

Dictionary<String, int>.fromMap(map); // Factory constructor.
Dictionary.fromMap<String, int>(map); // Generic static method.

Back on topic...

I'm not crazy about nullable factory constructors. The only motivation I've seen for them is fromEnvironment() and that API is almost pathologically weird. It's always been a hack. At some point, someone wanted a way to shoehorn "look up a constant value at compile time by string name", realized a const constructor of the target type could technically do that even if the API was totally unintuitive, and here we are. I don't think that's good evidence that nullable const factory constructors are a generally useful feature.

For the kinds of strings and ints used with fromEnvironment(), I think there is almost always an available value you can use to mean "no useful value". It's not like people are doing arbitrary arithmetic at compile time with these and every integer value could be useful. Likewise, there are an infinite number of possible strings, so it shouldn't be too hard to conjure one up instead of using null as the "absent" sentinel value.

The syntactic sugar feels a little like digging ourselves further into the weird hole that is fromEnvironment() If it were up to me, I would go with giving default values to the named arguments.

[Hixie]

I agree that "that API is almost pathologically weird". That is why I think we should just leave it as-is until we find a more long-term sane API to deal with environment variables (whatever that is). I have no problem with making all other factory constructors unable to return null.

If we cannot make this already not-actually-expressible-in-normal-Dart magical API just that little bit more magical for some reason, then I could live with bool.hasEnvironment(string) and giving the fromEnvironment factories a default argument. Maybe the default could even be required. That would make migrating easier since you'd quickly find all the places you need to add the bool.hasEnvironment(string) logic to.

[leafpetersen]

Ok, so there seem to be three proposals on the table:

1. Magic syntactic sugar makes const xxx.fromEnvironment(string) a nullable value of type xxx?, no default value.
2. Add a default value, and convert code that actually checks for and does something meaningful with a null value to use a new bool.hasEnvironment constructor.
3. Make defaultValue a required parameter, and convert code that checks for and does something meaningful with a null value to use a new bool.hasEnvironment constructor.

I'm not super enthusiastic about 1. The main reason is that I actually think this ends up being a worse experience for most users. I just checked our internal code (code search for lang:dart (String|int)\.fromEnvironment -file:dart_lang), and most uses of the fromEnvironment constructors already have a default value specified. If we do option 1, then all of those uses get typed as nullable values, even though they can never actually be null. As a secondary concern, it's more work to implement, and provides, I think, minimal incremental value.

Either of 2 or 3 seem reasonable to me. Most code already seems to pass defaultValue, so I don't think making it required (option 3) is as onerous as @lrhn suggests above. One problem I see with this though is that it will be weird if we don't make the bool.fromEnvironment constructor also have a required defaultValue, instead of a default as it currently does. There are substantially more uses of bool.fromEnvironment without a specified defaultValue though, so changing this does feel like more of a regression for our users.

Option 2 still feels like the best option to me overall. Option 3 is appealing mostly in that it forces users who are currently not passing a defaultValue to decide what they want to do. Empirically, that's a minority of users though, and of the ones that aren't passing a defaultValue, even fewer of them are actually checking for null. The small set of users that don't pass a defaultValue and do check the resulting value for null are likely to get a warning about an unnecessary null check on a non-nullable value, so I think we may actually get some static tooling help there.

@Hixie any further thoughts in light of the above?

[lrhn]

The "required defaultValue parameter" for String and int was my original migration approach.
It would force all existing uses without a defaultValue argument to supply one, which ensures that no code would subtly break (it would break overtly instead when they tried to migrate to null safety).

I do think that a default defaultValue of empty string/zero is a better API moving forward, even if it is slightly more error prone during migration. Migration only happens once.

I also don't expect any change to the API in the near future. It's not high enough on our list of priorities because the feature isn't very widely used to begin with. It's still used enough that it must work, and should not suck too much. An API which is a consistent and usable now is better than maintaining a hack for a significant time.

[leafpetersen]

Ok, I'd like to propose that we move forward with the original proposal from @lrhn with the addition of a bool.hasEnvironment constructor for use in migrating code which does distinguish between explicitly set and unset environment variables. My reasoning for this is essentially as I described above. @Hixie are you ok with this?

[vsmenon]

@Hixie is out this week. Given that his last comment and @leafpetersen 's last comment align, I think we can go forward.

lgtm

[franklinyow]

Approved.

[franklinyow]

Change landed