Skip to content
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

Offer a migration tool to convert map/when to switch-cases #940

Open
rrousselGit opened this issue Jun 26, 2023 · 7 comments
Open

Offer a migration tool to convert map/when to switch-cases #940

rrousselGit opened this issue Jun 26, 2023 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@rrousselGit
Copy link
Owner

rrousselGit commented Jun 26, 2023

We could have:

dart run freezed migrate

And for the given Freezed class:

@freezed
sealed class Example with _$Example {
  factory Example.data(int data) = ExampleData;
  factory Example.error(Object error) = ExampleError;
}

This would convert:

obj.map(
  data: (example) => print(example.data);
  error: (example) => print(example.error);
);

obj.when(
  data: (data) => print(data);
  error: (error) => print(error);
);

obj.maybeWhen(
  data: (data) => print(data);
  orElse: () => print('default');
);

into:

switch (obj) {
  case ExampleData(): print(obj.data);
  case ExampleError() => print(obj.error);
}

switch (obj) {
  case ExampleData(:final data): print(data);
  case ExampleError(:final error) => print(error);
}


switch (obj) {
  case ExampleData(:final data): print(data);
  case _ => print('default');
}

Things to figure out:

There may be no local variable

Users may write:

doSomething().map(
  data: (obj) => print(obj.data),
  ...
)

In that scenario, there's no local "obj" variable equivalent when using a switch:

switch(doSomething()) {
  case ExampleData(): print(<where is "obj" here?>);
  ...
)

We could create a local variable, but that could get quite complex, as we need to figure out where to create that local variable exactly.

For instance, this may involve changing:

int fn() => doSomething().map(...);

into:

int fn() {
  final obj = doSomething();
  return switch (obj) {
  ...
  };
}

Another thing to consider with this approach is: What should the variable name be.
We could name it based on the interface class name. So here Example -> example

Sometimes we need a switch "statement", and sometimes a switch "expression"

When converting:

String fn() {
  obj.map(
    data: (obj) => print('data'),
    error: (obj) => print('error'),
  );
  return ob.map(
    data: (obj) => 'data',
    error: (obj) => 'error',
  );
} 

We would have to output:

String fn() {
  switch (obj) {
    case ExampleData(): print('data');
    case ExampleError(): print('error');
  }
  return switch (obj) {
    ExampleData() => 'data',
    ExampleError() => 'error',
  };
} 

Notice how:

  • switches with a case don't end with a ;, but those with => do
  • switches with a case use a ; at the end of every expression, but switches with => use ,

Sometimes the factory type is not public

In the example, both ExampleError/ExampleData are public. But they could be private, and we could be in a file that does not have access to the private type.

In that scenario, we cannot generate the when/map equivalent.
We could have a warning asking folks to make those objects public.

The freezed class may not be sealed

If the annotated Example class is not "sealed", then switches cannot be exhaustive. In this case, we'll need to consider all whens/maps as "maybeWhen/maybeMap". But we don't know how the current code should handle the default case.

We could generate:

switch (obj) {
  ExampleData() => ...
  ExampleError() => ...
  // TODO handle default cases or mark "Example" as sealed
  _ => throw UnimplementedError(),
}

Closures may use function modifiers

Currently, users may write:

int fn() {
  obj.map(
    data: () async {
      await something();
    },
   error: ...,
  );
}

We cannot simply generate:

int fn() {
  switch (obj) {
    case ExampleData():
      await something();
     ...
  }

Because the await keyword wouldn't work here.

Even if fn was async, the behavior after the refactor would be different. An unawaited function would now be awaited.

We could do:

  • If the closure is async and the returned value is awaited, generate the switch anyway.
  • If the closure is async but the returned value is not awaited, wrap the block in a Future(() async {...})

For sync*/async* functions, we could stick to considering them unsupported for now.

Callbacks may be function tear-offs

Users could write:

obj.map(
  data: _onData,
  error: _onError,
);

In that case, the migration would likely be to invoke the functions by hand:

  switch (obj) {
    ExampleData() => _onData(obj),
  }
@rrousselGit rrousselGit added the enhancement New feature or request label Jun 26, 2023
@rrousselGit rrousselGit self-assigned this Jun 26, 2023
@rrousselGit
Copy link
Owner Author

This could also be added to freezed_lint to migrate only one specific when/map instead of the whole project at once.

@rrousselGit
Copy link
Owner Author

If we were to offer such migration in freezed_lint, the various challenges are not as problematic.

We could do a dumb conversion, ignoring all those issues. And the user would fix those by hand.

@TimWhiting
Copy link
Contributor

Things to figure out:

There may be no local variable

In that scenario, there's no local "obj" variable equivalent when using a switch:

doSomething().map(
 data: (obj) => print(obj.data),
 ...
)
switch(doSomething()) {
  case ExampleData(): print(<where is "obj" here?>);
  ...
)

Actually there is

switch(doSomething()) {
  case ExampleData obj: print(obj.data);
// or
  case final ExampleData obj: print(obj.data);
// or
  case ExampleData() && final obj: print(obj.data)
  ...
)

Another thing to consider with this approach is: What should the variable name be. We could name it based on the interface class name. So here Example -> example

Just use the name in the map expression, or am I missing something?

Sometimes we need a switch "statement", and sometimes a switch "expression"

Notice how:

  • switches with a case don't end with a ;, but those with => do
  • switches with a case use a ; at the end of every expression, but switches with => use ,

This should be doable. Just a matter of determining whether any of the body of the lambdas are not an expression body, which you would need a switch statement for, otherwise just default to a switch expression. The more difficult part is if it is used in an expression context. In which case you need a switch expression. In this case, although it is less readable you can use an immediately invoked function application: ((){..body})().

Sometimes the factory type is not public

In the example, both ExampleError/ExampleData are public. But they could be private, and we could be in a file that does not have access to the private type.

In that scenario, we cannot generate the when/map equivalent. We could have a warning asking folks to make those objects public.

This is definitely a problem that needs a warning. However, the fix can potentially be automated.

The freezed class may not be sealed

If the annotated Example class is not "sealed", then switches cannot be exhaustive. In this case, we'll need to consider all whens/maps as "maybeWhen/maybeMap". But we don't know how the current code should handle the default case.

The default case for maybeWhen should obviously be a catch all. For the exhaustive when / map, a throw statement should be good, though I would also output a list of all the places this was added due to not having a sealed class - so the user knows which classes they need to make sealed, or have an option for the migration tool to do a dry run, and only output the warnings for the sealed classes, so they can be fixed prior to actually migrating. Some classes might potentially be outside the users control, so it still should be able to migrate without sealed classes.

Closures may use function modifiers

We could do:

  • If the closure is async and the returned value is awaited, generate the switch anyway.
  • If the closure is async but the returned value is not awaited, wrap the block in a Future(() async {...})

For sync*/async* functions, we could stick to considering them unsupported for now.

Wrapping in Future(()) should be fine for return values that are not awaited.

Callbacks may be function tear-offs

Your proposed solution looks fine.

Other Notes

As far as the way to offer the migration to the users, I just want to point out that when we used https://pub.dev/packages/codemod for riverpod 1.0 or 0.14.0?, we ran into lots of issues with overlapping edits due to nesting patterns. This causes the whole migration to fail unfortunately.

Either

  • The edits need to be done and applied to the files one at a time (which means you have to re-analyze the files that have changed after each edit)
  • You need to compute the subset of changes that are non overlapping and then apply all of those changes and iterate the migration from there until there are no more edits
  • You need to manage edit buffers and string manipulation for overlapping edits yourself - easy to mess up something

Or you just do it via a lint / IDE refactoring which is probably most reliable for users. (This would also make it easy for users to alter the code and make small updates while they are reviewing it). However, due to the pervasive nature of .map and .when when using freezed, it is likely a long conversion process especially with larger codebases.

Although there is some pushback on deprecating it (which I was initially opposed to as well), I do think that there is a large benefit to switching code to use dart's pattern matching -> especially when considering nested matches, more flexible orElse logic for dealing with union classes that have a common parent interface etc. That's before even mentioning the smaller generated code size. I think the option of generating the methods (not by default) should stick around at least till a macro version of freezed. The biggest thing that I would say about this is to make it very clear on the README that this has happened. Not everyone follows you on Twitter / GitHub, also not everyone looks at CHANGELOGs when running pub upgrade (even though they should).

@rrousselGit
Copy link
Owner Author

Actually there is

switch(doSomething()) {
  case ExampleData obj: print(obj.data);
// or
  case final ExampleData obj: print(obj.data);
// or
  case ExampleData() && final obj: print(obj.data)
  ...
)

Great!

Just use the name in the map expression, or am I missing something?

You're right. I was thinking that this could shadow existing variables. But when thinking about it that's what map does too anyway.

In fact the opposite case could be made. That the refactor could stop shadowing variables if we don't do this. So if someone does:

int value;
obj.whem(
  data: (value) {
    print(value++);
  }
)

Then doing:

int value;
switch (obj) {
  case ExampleData(data: final value):
    print(value++);
}

would not be equivalent. As the ++ is now changing a different variable.


About migrating non-sealed classes, I was thinking that:
If there's one (and only one) case where there's no used property, we could use that as last case.

An example is Riverpod's AsyncValue with its loading state;

It's not sealed quite yet, but folks can do:

switch (AsyncValue()) {
  AsyncData() =>...,
  AsyncError() => ...,
  _ => print('loading'),
}

@rrousselGit
Copy link
Owner Author

Your mention of codemod reminds me that the linter could technically work as a migration in itself.

There's an issue in custom_lint to offer a custom_lint fix command. invertase/dart_custom_lint#161

If this is implemented, then implementing a fix in the IDE would also work as a widespread migration tool too.

So maybe we should focus on the linter.

@TatsuUkraine
Copy link

Sometimes the factory type is not public
In the example, both ExampleError/ExampleData are public. But they could be private, and we could be in a file that does not have access to the private type.

In that scenario, we cannot generate the when/map equivalent. We could have a warning asking folks to make those objects public.

one thing to consider about private subtypes. It can actually be quite common case since it allows to avoid name conflicts as well as simplifies class names. Forcing devs to switch completely to pattern matching can make transition quite painful

@rrousselGit
Copy link
Owner Author

The migration probably should look for name conflicts. And if so, use import aliasing to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants