new lint: no_state_constructor

[pq]

Example and docs could use some improvement (cribbed largely from the issue).

@rrousselGit @jonahwilliams ideas for improvement?

/cc @bwilkerson

Fixes dart-lang/sdk#58075

[jonahwilliams]

This is great!

[rrousselGit]

Looks good!

[rrousselGit]

It's unlikely to really matter, but instead of checking that State doesn't have constructor it might make more sense to check that the createState method doesn't do anything fancy

Theoretically, folks could still do:

final int property;

@override
MyState createState() {
  return MyState()
    ..property = property;
}

which shouldn't be allowed either.

[bwilkerson]

Would it be better to prevent State subclasses from declaring any instance fields? That would prevent them from having any state no matter how that state gets set (via constructors, field assignments, etc.). It would also prevent them from caching computed values, which might or might not be a good thing.

[jonahwilliams]

State subclasses definitely need instance fields. For example:

class MyTextEditor extends StatefulWidget {
  ...
}

class _MyTextEditorState extends State<MyTextEditor> {
  TextEditController _controller;

  @override
  void initState() {
    _controller = TextEditController();
  }

  ...

}

[pq]

It's unlikely to really matter, but instead of checking that State doesn't have constructor it might make more sense to check that the createState method doesn't do anything fancy

This is interesting. My inclination is that this might be a separate lint? (And possibly a bit more prone to false positives.)

One advantage of the current advice (as you proposed it) is that it's easy to articulate and reliably enforce.

Unless there are any strong objections, I'll go ahead and land this and we can consider refinements down the road.

Just let me know!

[bwilkerson]

it would be interesting to run some experiments to see whether the createState checks cause false positives. I think it might be interesting to have a combination of the two. For example, it seems like it would be a better UX to tell users to not define constructors with parameters than to tell them not to use them.

[rrousselGit]

This is interesting. My inclination is that this might be a separate lint? (And possibly a bit more prone to false positives.)

I suggested no_state_constructor as name, but it's not necessarily the only name possible.

Strictly speaking, it's not having constructors on State that causes issues, but having logic inside createState.
Not having a constructor on State is more of a consequence of having createState do nothing.
People can have constructors on State if they want to. It's just that since it should take 0 parameter, it's useless.

In the end, there's never a situation where we'll want a custom createState/State constructor. People should use initState instead, so on both cases there's no false positive

If anything, "no state constructor" is just slightly a bit less helpful than "no fancy createState".
I've also seen people doing:

MyState global;

class MyStateful extends StatefulWidget {
  @override
  MyState createState() {
    global = MyState();
    return global;
  } 
}

which is anti-pattern too but doesn't use constructors.

Ultimately, createState has just a single possible implementation, that is:

@override
MyState createState() {
  return MyState();
}

With maybe replacing {} + return with () =>.
But other than that, it shouldn't change, ever.

But i don't have any strong opinion on that one. A "no State constructor" may be easier to implement and cover most scenarios.
A "no fancy createState" is just closer to the actual rule

[pq]

I'm inclined to keep these ideas as separate rules.

I'd like to land this and then consider the "no fancy createState" one.

[rrousselGit]

I'm not sure it's useful to have both. But it's your call

[pq]

Closing in favor of #1877.