dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.21k stars 1.57k forks source link

"Move to file" should move `StatefulWidget`s and `State`s together #56392

Open FMorschel opened 2 months ago

FMorschel commented 2 months ago

If you have the following code (sealed classes), it asks if you want to move them all together to a new file:

sealed class A {}

final class B extends A {}

image

Now if you have a StatefulWidget it doesn't do that with the State related to it.

Sample:

import 'package:flutter/material.dart';

void main() => runApp(const MaterialApp(home: Home()));

class Home extends StatefulWidget {
  const Home({super.key});

  @override
  State<Home> createState() => _HomeState();
}

class _HomeState extends State<Home> {
  @override
  Widget build(BuildContext context) {
    return const Scaffold();
  }
}

I'm asking that this "Move to file" action should bring the StatefulWidget/State together.

CC: @DanTup

dart-github-bot commented 2 months ago

Summary: The "Move to file" refactoring currently doesn't group StatefulWidgets and their corresponding State classes together when moving them to a new file. This behavior is inconsistent with how other types of classes are handled, leading to potential code organization issues.

FMorschel commented 3 weeks ago

To note, today if you have the following and you try to extract the method, you get:

for (int i = 0; i < 1; i++)
Switch(
  value: true,
  onChanged: (v) {
    debugPrint('Switch value: $v - $i');
  },
),
Cannot extract the closure as a method,it references the external variable 'i'.

(the missing space is on the message, I've seen some similar things and will post a CL to fix those)

But if you have a class that depends on any form of a private class (like a state that most of the time starts with _) we get no errors, and the new file created gets the import for the original file even though it will trigger unused_import if the only class from it that it has a dependency is private.

P.S.: Should this be considered in a new issue or using this one to consider this is fine?

DanTup commented 3 weeks ago

@FMorschel they sound unrelated to me - if "Move to File" is updated to treat StatefulWidget+State as a pair, I don't think it'll impact either of the other things you mentioned (and I also don't think the restriction on extracting closures that reference variables is related to references between classes when moving to files).

FMorschel commented 3 weeks ago

My point in raising this here is because of States usually being created as private classes.

My point in relating the extract method is that when trying to extract a method that references something it will miss if extracted, it gives a warning and stops the process. But if we do the same with the new "Move to file", currently we don't receive any warning and when extracted we can get lots of missing references.

I'll open a new issue.

DanTup commented 3 weeks ago

My point in raising this here is because of States usually being created as private classes.

Oh, gotcha. I think probably moving StatefulWidget+State together as a pair is probably a simpler problem than moving things with references to private classes, so probably they're two requests (for example we can probably assume that StatefulWidget+State should move together, but it might be better to ask if moving a class would bring 5 other private classes with it).

it gives a warning and stops the process. But if we do the same with the new "Move to file", currently we don't receive any warning

Ah, I see what you mean. I'm not certain if these things are equivalent though, one is a failure and the operation was aborted, but the other (I think) is something where we did the best we could and think you'd rather fix up the errors than us just abort. Perhaps there should be a prompt though to tell you that the operation will result in analysis warnings and let you choose to proceed (like we do with renaming something that would shadow, and give a "Rename anyway?" button). That probably is worth its own issue.