Skip to content

Problem: Adding a 2nd (or later) type parameter to a class breaks clients #283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
srawlins opened this issue Mar 21, 2019 · 14 comments
Open
Labels
request Requests to resolve a particular developer problem

Comments

@srawlins
Copy link
Member

and incrementally migrating is not easy; it must be done atomically with the type-parameter-increasing change.

This is not a problem when adding the first type paramter:

class C {}

// This is a non-breaking change:
class C<T> {}

// This is also non-breaking:
class C<T, U, V> {}

However, when adding any type parameters beyond the first, the change is breaking for any clients which specify any type arguments:

class C<T> {}

// This is a breaking change:
class C<T, U> {}

// For code such as the following:
new C<int>();
C<int> c;
class D extends C<int> {}

Introducing a second class while renaming the original class is one way to "deprecate" the previous number of type parameters, and expose the "new" number of type parameters:

class C<T> {
  T f1;
  dynamic f2;
}

// Non-breaking change:
class C_2<T, U> {
  T f1;
  U f2;
}

@Deprecated("Now with more type parameters! Use C_2!")
class C<T> extends C_2<T, dynamic> {}

You can ship this non-breaking change, announcing the deprecation, allowing clients to migrate, and later ship a breaking change where you remove C.

The big problem with this is that no one wants to rename classes. If you look at functions, it is a breaking change to introduce or remove required parameters (or remove or rename named parameters), but this is mitigated by making optional parameters available. Or perhaps the purpose of a function has changed enough with the changed parameters that you can offer it as a new function with a new name, and keep the old function around, marking it @Deprecated().

Renaming a class seems like a much bigger and unfortunate change. Classes often do not involve more than two words; typically a few nouns, rather than an English predicate expression. "ContainerViewModel" means the Model of a View of a Container, which should not be changed based on the number of type parameters.

@eernstg
Copy link
Member

eernstg commented Mar 22, 2019

Would the following approach be sufficiently helpful? (It relies on generalized type aliases, but that feature is coming, it is just sitting in the pipeline a bit longer than we thought because of other features with a higher priority).

Before the change we have this (I'll focus on the change from one to two type parameters because that one embodies the harder issue):

// Library 'useful_stuff.dart'.
class C<X> { ... }

// Library 'client.dart'.
import 'useful_stuff.dart';
... // Lots of occurrences of `C<T>` for some `T`.

Then we change C to have two type arguments, and we'd like to have a per-library migration. We rename the library 'useful_stuff.dart' to 'new_useful_stuff.dart' and provide a new 'useful_stuff.dart' library to make the transition easier:

// Library 'new_useful_stuff.dart'.
class C<X, Y> { ... }

// Library 'useful_stuff.dart'.
import 'new_useful_stuff.dart' as p show C;
export 'useful_stuff.dart' hide C;
typedef C<X> = p.C<X, dynamic>;

Client libraries stay unchanged, but their imports will now refer to the new "bridge" library 'useful_stuff.dart' rather than the library that actually declares C.

Each client can then make the choice to change its import to `new_useful_stuff.dart' and adjust the code to provide the new type argument in useful ways.

Of course, if the name 'useful_stuff.dart' must remain unchanged then we could also use a new name for the bridge library, and then clients would have to adjust their imports. That trade-off could be OK, because it is nearly a textual operation (a global search and replace from importing 'useful_stuff.dart' to importing 'bridge_useful_stuff.dart').

@srawlins
Copy link
Member Author

This definitely offers an alternative to class renaming.

If the class is defined in a /src/ library which is already only accessed via a public export, then this solution may be more awkward than the class renaming.

import 'package:foo/foo.dart';

becomes

import 'package:foo/foo.dart'; // old C is hidden by foo.dart
import 'package:foo/foo_transitional_c.dart';

@eernstg
Copy link
Member

eernstg commented Mar 22, 2019

Of course, this might not work in practice (if there are several classes with changes like this, and transitioning might not happen all at once, etc), but in the simple case where we only change C it seems like foo_transitional_c.dart might as well export the same name space as foo.dart used to export (that is, the typedef'ed C, plus all other things with no changes), and clients would then only need to change

import 'package:foo/foo.dart';

to

import 'package:foo/foo_transitional_c.dart';

When they are ready to update that particular library to use the new C they'll switch back to foo.dart.

Again, the naming could be swapped such that clients don't have to change anything initially, if it has top priority to avoid changes in client code initially, and if it is acceptable to use a new name for the library when the transition is over: So, clients would do nothing at first, but when they are ready to update a library that imports foo.dart then they change the import to new_foo.dart and fix the code.

@munificent
Copy link
Member

This is not a problem when adding the first type parameter:

Technically, it can be a problem here too if the first type parameter has a bound that inference can't satisfy, because then using the raw type becomes an error. More generally, for users that enable "strict raw types", adding a type parameter will become an error.

C# handles this like it handles adding parameters to methods: it lets you overload. You can define multiple classes with the same name but different type parameter arity:

class C {}
class C<T> {}
class C<T1, T2> {}

@caseycrogers
Copy link

caseycrogers commented May 11, 2021

I have a related use case for the same requested feature:

You can't have a class with a Type Parameter and a field member with a default value. eg the following throws a compile error because String is not type T:

class Foo<T> {
  Foo({this.bar = 'default_value'});

  final T bar;
}

The desired behavior would be, if the user doesn't specify bar, T takes on the type of the default value. Alternatively, to be more explicit, this could be written as:

class Foo<T = String> {
  Foo({this.bar = 'default_value'});

  final T bar;
}

The only current workaround I'm aware of is to create a default class that extends the base class, which works but is non-ideal as it requires API users to be aware of and use the alternate class if they want to override default behavior:

class Foo extends GenericFoo<String> {
  Foo(): super(bar: 'default_value');
}

class GenericFoo<T> {
  GenericFoo({required this.bar});

  final T bar;
}```

@eernstg
Copy link
Member

eernstg commented May 12, 2021

It would be possible (and might be useful ;-) to allow a type parameter to have a default value. We'd need to deal with a couple of difficulties, however:

There is a syntactic conflict, because default values are otherwise specified using '=' <expression> after the parameter name, and that works rather well for a value parameter because the type (if specified) occurs before the parameter name: foo({int p = 1}).

For a type parameter, the type is specified as a bound. So X extends num means that X belongs to the set {T | T <: num} (whereas int p means that p belongs to the set int), and this means that the location just after the parameter name is already occupied. But we might be able to use a word like default to create the connection: X extends num default int looks more readable to me than X extends num = int or X = int extends num.

So we'd have this:

class Foo<X default String> {
  Foo({required this.bar});
  final X bar;
}

The missing part is the default value. Default values could be generalized a lot, cf. #140, so we could use a computeDefaultValue getter to satisfy the constraint that the chosen value must be typable as an X:

class Foo<X default String> {
  Foo({this.bar = computeDefaultValue});
  final X bar;
  X get computeDefaultValue {
    const defaultString = 'default_value';
    const defaultInt = 42;
    if (defaultString is X) return defaultString;
    if (defaultInt is X) return defaultInt + X.toString().length; // We don't have to return a constant.
    ... // Handle some selected set of other values for `X`.
    throw new ArgumentError("No suitable default value found for `Foo.bar`.");
  }
}

This illustrates that having a default value that does not have the required type for all values of X is tricky, but it would be possible to create sort of a solution by doing some case analysis on the value of X.

I don't see how we could have an actual default value that satisfies all the possible values of the type variable, except for cases where we can use Never:

class A<X> {
  final List<X> xs;
  A({this.xs = const <Never>[]});
}

There has been a proposal for defining default values for types, #1227, but that's probably not useful when we wish to define suitable default values for a specific parameter. So it looks like you'd end up having the kind of unsafe case analysis that we have in computeDefaultValue above.

@caseycrogers
Copy link

caseycrogers commented May 12, 2021

Your analysis/proposal is a lot more expansive than mine-my assumption/use case is that the default value would be used only if the default type had also been used. eg Foo<num>() would/should be a compile time error because default_value is not a num. Foo(), Foo<String>(), Foo(bar: 4) or Foo<num>(bar: 4) would all be valid. When a user specifies bar, T is inferred by the type of bar. To me it'd be intuitive for T to be inferred from the type of 'default_value' when bar is omitted (the first of the two proposals in my comment above).

That said, your proposal (assuming default values were first allowed to be non const) sounds like it could be useful . Eg if you're creating a class for storing and manipulating data types, you might have a list of types with supported zero-values and want to allow users to skip specifying an explicit value for any class with a valid zero value. I don't think there's a way to get around the unsafe computeDefaultValue code though....

Also I definitely agree on default over =, that's much more readable.

@eernstg
Copy link
Member

eernstg commented May 12, 2021

It's an interesting idea that the default value is essentially considered to exist only when the typing situation allows it to be used (in all other situations the parameter would be considered to be required).

It's not trivial to define that approach, however, because many types are only known by an upper bound:

class A<X> {
  void m([X x ?= 42]);
}

void main() {
  var a = A<int>();
  a.m(10); // OK.
  a.m(); // OK, `42 is X`.
  A<num> a2 = A<double>();
  a2.m(); // Run-time error, `42 is X` does _not_ hold!
}

So we can basically only rely on having a default value when we have a lower bound L for the value of the type variable X, and that is just about the same thing as knowing the exact type. It would also work if we introduce declaration site variance and X is contravariant:

// Requires `--enable-experiment=variance`.

class A<in X> {
  void m([X x ?= 42]);
}

void main() {
  var a = A<int>();
  a.m(10); // OK.
  a.m(); // OK, `42 is X`.
  A<double> a2 = A<num>(); // Note that A<num> <: A<double>.
  a2.m(); // Compile-time error, `42 is X` is not known to hold.
}

So it might actually work quite well together with sound variance. Interesting! ;-)

@mateusfccp
Copy link
Contributor

mateusfccp commented Sep 4, 2024

@eernstg I'm considering this as a viable alternative to introduce phantom types to a class without breaking it.

In this case, it wouldn't require any default values, but its usefulness would also be reduced.

Aside from the default values problem, is there any other problem that you see that may arise from this issue?

@eernstg eernstg added the request Requests to resolve a particular developer problem label Sep 4, 2024
@eernstg
Copy link
Member

eernstg commented Sep 4, 2024

I don't think this issue presents a specific proposal, it's a 'request' rather than a 'feature' issue. Of course, some comments may contain the beginnings of a proposal, including type parameters with default values, or workarounds, like the use of type aliases, and each of those would have pros and cons.

By the way, I don't think anybody mentioned that the additional type parameter could be added in a subtype:

// Current version.
class A<X> {
  final X x;
  A(this.x);
}
// Next version has a phantom type.

class A<X> {
  factory A(X x) = A2<X, String>; // Assuming a "default" of `String` for `Y`.
}

class A2<X, Y> implements A<X> {
  final X x;
  A2(this.x);
}

This would work as long as the constructor change is acceptable: In the first version, A has a generative constructor, which means that it is possible to create subclasses of it. In the second version we use a factory constructor, which would break the subclasses, but on the other hand we do get an instance of A2 which should work as a replacement for A, and it will use the specified default value for Y. This may or may not be a workaround that actually works, but it should be pretty safe if the class is interface (because subclasses declared in the same library can be fixed, and subclasses in other libraries cannot exist).

@mateusfccp
Copy link
Contributor

mateusfccp commented Sep 4, 2024

I don't think this issue presents a specific proposal, it's a 'request' rather than a 'feature' issue.

Yeah, I am aware.

I asked because I thought of writing a proposal with what I had in mind, but I didn't consider the default value question.

For my specific use case, it's irrelevant. As I was thinking about phantom types, no value is used. In this case, we could simply ignore this and decide that:

  • A type parameter with a default value must be an interface (no implementation);
  • A type parameter with a default value does not need to be an interface, but can't use the type parameter with default value as a final field.

I don't know if this would be enough, I didn't think it through.

However, as I said, I don't know if this kind of proposal would bring much value, so I'm trying to identify which potential problems arise from this issue.

By the way, I don't think anybody mentioned that the additional type parameter could be added in a subtype.

I didn't though about this workaround. It is a little limited, though, and wouldn't work for my case.

Consider that A has a subclass B, which after introducing the phantom type, should "default" to B extends A<C>. Introducing A2 extends A<C> and a factory on A would still be breaking, as in the current version B extends A will infer something else for the type parameter of A.

@eernstg
Copy link
Member

eernstg commented Sep 5, 2024

@mateusfccp wrote:

Consider that A has a subclass B, which after introducing the phantom type, should "default" to B extends A<C>.

I think the use of a type alias could be the most versatile gradual transition tool for this. For example:

// --- Auxiliaries.

class C {}

// --- A client. Should just keep working.

class B extends A {}

// --- Version 1.0.

class A {/*...A stuff...*/}

// --- Version 1.1: Introduce the new class.

class NewA<X> {/*...NewA stuff...*/}
typedef OldA = NewA<C>;
typedef A = OldA;

// --- Version 2.0: Default is now the new class.

class NewA<X> {/*...NewA stuff...*/}
typedef OldA = NewA<C>;
typedef A<X> = NewA<X>;

// --- Version 2.1: Clean up.

class A<X> {/*...A stuff...*/}

Assume version 1.0 of the package that provides A. Then it's updated to 1.1. Class B still works (assuming that NewA<C> is a compatible replacement for A, but that should be a safe bet if it just adds an unused type parameter). The maintainers of B are now aware of the upcoming change to A.

They can use extends NewA<Something> if they want to be on the bleeding edge, or they can do nothing (and expect to be forced to fix it later), or they can use extends OldA in order to delay the transition as much as possible.

Assume version 2.0. With extends NewA<Something>, everything just continues to work, but changing it to extends A<Something> is the approach that will continue to work for a long time. With an unchanged extends A, the code is now broken, it must be updated to extends A<Something>. With extends OldA it still works, but it won't work when those type aliases are cleaned up; so they should probably update it to extends A<Something>.

Finally, version 2.1 deletes the transitional names, and A now has a type parameter.

Does that work for your case?

@mateusfccp
Copy link
Contributor

mateusfccp commented Sep 5, 2024

@eernstg wrote:

Does that work for your case?

Hmm, not really.

This would work if the client wouldn't ever want a different phantom type, but let's introduce a new class in the client.

// --- Library.

class A { /* ... */ }

// --- A client.

class B extends A {}

class C extends A {}

Now, I want to introduce the phantom type to A. I want B to use the "default" value, while C should use a different type.

// --- Library.

class NewA<T extends Tag> {}

typedef A = NewA<DefaultTag>;

abstract interface class Tag {}

final class DefaultTag implements Tag {}

final class OtherTag implements Tag {}

Now, even if B does not break, for C I will have to implement NewA instead of A.

// --- A client.

class B extends A {} // Does not break, uses `DefaultTag` correctly

class C extends NewA<OtherTag> {} // Can't extend `A`, have to extends `NewA`

@eernstg
Copy link
Member

eernstg commented Sep 6, 2024

Right, we would be able to make the transition more smooth with default values for formal type parameters.

On the other hand, it seems likely that you're changing the semantics of the class C, because (presumably) the meaning of A should be the same as the meaning of NewA<DefaultTag>, and you want to use extends NewA<OtherTag> with C. So it doesn't demonstrate a need to use a different superclass than A with a class that should work exactly as it has worked until now.

Anyway, I have no doubts that default values of formal type parameters can make some evolutionary changes to an API easier to roll out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

5 participants