@davidmorgan is there something I need to do for this PR?

@davidmorgan are there any problems implementing this PR? it's an important step towards having const constructors for all built values

Apologies for the slow response.

Indeed, making constructors const does not help the factory, so I'm not sure this makes sense yet.

built_value can't be const because of memoized fields, anyway, so that also needs more work.

Maybe it would be helpful to back up a bit :) and ask what you want const built_value for, please? That would help motivate the changes :)

I'm just now going on vacation for a week so my next response might also be slow, sorry in advance for that.

in general we need const constructors for the ability to have a default value when providing an API to consumers.

consider this example:

myFunc({
    BuiltList<String> scopes = BuiltList(["hello", "world"])
}) {}

this is currently not possible, and the current alternative is to change the parameter type to nullable, like this:

myFunc({
    BuiltList<String>? scopes,
}) {
scopes??=BuiltList(["hello", "world"]);
}

this is bad for the following reasons:

1. scopes appears to accept null, but in reality it will never have null as a value.
2. we hide the default value from the caller, so they are forced to inspect the function's source code to discover the default value.

this of course extends to other scenarios, like

myFunc({
JsonObject body = JsonObject.fromNum(42),
});

our specific usage is that I am part of a team that maintains the dart generator of openapi generator which has an option to use built_value as a serialization library.

we are currently doing a rewrite that aims to improve the experience of api consumers by providing them with proper defaults.

hi @davidmorgan did you have a chance to look at this ?

Thanks.

I'm not super excited about this direction for a few reasons, I will explain what I'm thinking :)

First off, I'm not fully convinced by the defaults mechanism in the language itself. Now that we have null safety, this pattern works well:

/// Do something.
///
/// By default do it one time, for a different number set [times].
void doSomething({int? times}) {
  optional ??= 1;
  // optional has now been promoted to non-nullable type `int` and is ready to go :)

This has a couple of advantages over specifying the default in the signature:

• It's better for forwarding values to this method, because you can pass around null to mean default. Otherwise, you would have to copy the default to the place that does the forwarding to use the default.
• It's better for overriding this method, again because it works without having to copy the default to the code that does the overriding.

Second, adding const reduces what the implementation can do but increases what its users can do, meaning it's a problem for maintenance: you might want to do something not compatible with const later, but removing it would be a breaking change for users.

And third, this PR is only a partial fix, it doesn't make it so all of built_collection/built_value can be const, but I'm not (currently) convinced that it's both worthwhile and attainable to make that happen for the packages fully.

So, I'm not yet convinced about landing this.

Happy to discuss further though :)

Thanks.

This has a couple of advantages over specifying the default in the signature:

• It's better for forwarding values to this method, because you can pass around null to mean default. Otherwise, you would have to copy the default to the place that does the forwarding to use the default.
• It's better for overriding this method, again because it works without having to copy the default to the code that does the overriding.

These might be advantages, but having a parameter as non-null and with a default value is a functional requirement of the API, especially when generating OAS schemas.

Second, adding const reduces what the implementation can do but increases what its users can do, meaning it's a problem for maintenance: you might want to do something not compatible with const later, but removing it would be a breaking change for users.

const doesn't disable all processing, it just disables processing on construction, if all processing is done lazily and cached (using Expando for example) it won't be a problem.
I would actually argue that const is an important feature of built values, since it strictly implies immutability.

And third, this PR is only a partial fix, it doesn't make it so all of built_collection/built_value can be const, but I'm not (currently) convinced that it's both worthwhile and attainable to make that happen for the packages fully.

I can make a PR to fully transform all of built_value and built_collection if that's a problem :)

These might be advantages, but having a parameter as non-null and with a default value is a functional requirement of the API, especially when generating OAS schemas.

Why does that happen, if you don't mind me asking--e.g. does the default end up in the schema?

const doesn't disable all processing, it just disables processing on construction, if all processing is done lazily and cached (using Expando for example) it won't be a problem. I would actually argue that const is an important feature of built values, since it strictly implies immutability.

const does imply immutability, but it's too strong; as you know it required that the value is computable at compile time, but there's no particular reason to want that for most built_value usage.

Really the main valid use of const is to enable compile-time optimizations such as AOT tree-shaking; other uses are more like accidental features/misfeatures of the language and how Dart has evolved.

Immutability in the sense of strict value immutability is also too strong: "immutable" data should be free to mutate as an optimization as long as that's hidden. You can use expandos for that, but there is a performance cost.

Really what's wanted is that the values are "stable" in the sense of this language proposal

dart-lang/language#1518

if this language proposal moves forward--and I hope it does--then it should be the case that default values don't have to be const, they can just be stable.

This would be the right concept for built_value and would also work for your use case, I think.

Unfortunately there's no ETA on the proposal, but with Dart 3 done I guess the language team has to work on something ;)

And third, this PR is only a partial fix, it doesn't make it so all of built_collection/built_value can be const, but I'm not (currently) convinced that it's both worthwhile and attainable to make that happen for the packages fully.

I can make a PR to fully transform all of built_value and built_collection if that's a problem :)

I'd be interested to see it, but I think it's unlikely to actually land as I'm not convinced about the tradeoffs and direction.

Thanks :)

Why does that happen, if you don't mind me asking--e.g. does the default end up in the schema?

consider the case of a nullable input with a default value that's not null, e.g.:

void myFunc({int? myInt = 5}) {
}

using built value, it's not currently possible to represent this in any way.

While it's true that you can't easily replicate the behavior of

void myFunc({int? myInt = 5}) {

I'm not actually sure that's a good behaviour to want to replicate :) ... most often when I see this it's a bug, not a feature. e.g. just yesterday I fixed a case where a null value was being accidentally passed, wiping out the default; the correct behaviour was to have no default then set the default in the body, so null turns into default.

For what it's worth, you can actually do exactly this with the same trick freezed uses in its generated code:

abstract class A {
  void myFunc({MyValue? myValue}) {
    // write myFunc as normal
  }
}

class AImpl extends A {
  static const _sentinal = Object();
  
  @override
  void myFunc({Object? myValue = _sentinal}) {
   // MyValue.default() behaves as a default without needing to be const!
    super.myFunc(myInt: myvalue == _sentinal ? MyValue.default() : myValue as MyValue?);
  }
}

But, I don't particularly like this trick.

The other way that is not fully equivalent, but that I prefer, is the builder pattern. A builder can tell whether a field has been set or not, so it can choose to behave differently. This whole area was actually part of the inspiration for writing built_value in the first place :)

the problem is that we are directly translating OAS spec directly into dart code, and in OAS it's normal and common to have a nullable field have a non-nullable value.

but back to the main discussion, I think having const constructors are essential for a package that is all about immutability, and while dart's own classes aren't perfect (e.g. DateTime doesn't have const constructors), it would help greatly if built_value starts leaning towards this.

It might be possible to make the code you generate do the right thing anyway by using the same trick as freezed or some other trick.

I don't want to encourage using const to mean "immutable", they are different in important ways. I hope there will be more support in the language for a concept of immutability that actually matches what is wanted here.