Do not include TODO comments in generated code

[stevemessick]

Original issue: https://github.com/flutter/flutter-intellij/issues/6239

The request is to stop generating TODO comments when generating code for overridden methods. It's just more stuff to delete, and seems redundant given the throw statement.

[julemand101]

An important difference between the TODO: implement <function> and throw UnimplementedError(); is that the second might be fully valid code in situations where we think it makes no sense to override the method.

While the TODO are something you can setup your IDE to lookout for and notify that you have TODO's in your code:

Some projects have rules and checks to ensure a release does not contain any TODO's. But a TODO might be fine to have temporarily in your project.

Another observation is that, IntelliJ at least, does not add the throw UnimplementedError(); for methods that are defined to return void. So it would be problematic to just check for throw UnimplementedError(); to verify if there are methods that have been forgotten.

But we can't either just keep the TODO since the reason for throw UnimplementedError(); is added is because we must have this in methods that are specified to return something unless we have a default value for every type.

So in short, I think it is important to keep the current behavior. It could be considered to be consistent and add throw UnimplementedError(); every time. But I don't think that makes much sense since we more often just want overridden void method to just do nothing by default.

[gspencergoog]

Some projects have rules and checks that a TODO must have a particular form (e.g. include an author or issue link), and the default TODO doesn't conform to them, and can't conform to all possible formats.

I agree it would be nice to not include the throw on methods that return void (or Future<void>).

It sounds like the ideal thing would be to make it configurable as to whether TODOs are included or not in the suggestions.

[DanTup]

It was noted at https://github.com/flutter/devtools/pull/4488#discussion_r977918028 that when overriding, the TODO appears above the call to super which is probably not the most common place to add code (with some exceptions, like dispose).

This issue made me wonder if it's worth just swapping them around or if they may be removed (or whether there should be preferences for TODOs, throws, etc.)).

The analysis server doesn't currently have a way to express preferences (although it sometimes infers them from lints), but maybe this is something that would fit in there if it did get one.

[CoderDake]

would it be possible to autocomplete with snippets that have tabstops for traversal instead of straight TODOs?

That way the user could tab through to important parts and fill them in quickly. We could have the throw be a placeholder at the tabstop so the user could leave it if they want to as well.

[gspencergoog]

I am, of course, in favor of just removing them, since I always have to remove them manually each time I insert a snippet, which is a bore.

[bwilkerson]

I don't disagree that having TODO comments in generated code is sometimes inconvenient, but the reason we originally decided to add them is for the case where you write a class that implements an interface and you need to provide concrete implementations of a large number of methods. We have a quick fix that will add stubs for all of them, and we thought that adding a TODO in every stub would help users keep track of which methods they hadn't yet gotten to.

It's possible that that use case is too rare, or that the TODOs aren't as helpful in that case as we'd expected, but that's why it works the way it does. I do, however, think that it's important to consider the bulk addition of stubs as a use case when making a decision.

[gspencergoog]

I'm just one data point, but here is my experience of the feature:

When I use this feature, I most often use it in these cases:

1) I know there's a function I want to override, and I type the first few characters of it and ask for the suggestion to fill in: for instance, when I want to add an initState to a stateful widget's State. 2) I am adding a switch on an enum and I want to populate all of the cases. 3) I am implementing an interface or adding a mixin, and I want to get the stubs for all the missing functions.

Of all of these, I agree that the third case is the most likely to want a TODO in it, but even then each one of the added methods has a throw UnimplementedError() in it already to indicate that they aren't implemented, so the TODO is redundant.

In all three of these cases, the first thing I do is to remove all of the TODOs. For me (maybe not for everyone), they just feel like they're getting in the way. The IDE highlights them as analyzer warnings because they're not the right format for our project (and even if they were, it would highlight them as TODOs), and that makes it hard to read the code, so I just want them out of there so I have "space to work". So every time I do use the feature, I spend a while cleaning up after it before I can get down to actually implementing. It's still useful enough to use, but the TODOs definitely add friction.

If it's not easy to make it configurable, maybe this would be a good case for UXR to ask a few users what they think so we can make a data-based decision?

[bwilkerson]

If it's not easy to make it configurable

We could certainly make is configurable, but I personally think that configuration options should be a last resort. I'd much prefer it to just work well for everyone.

... each one of the added methods has a throw UnimplementedError() ...

I agree that we don't need two markers that perform the same purpose, and I'd much rather get rid of the comment. If we always have a throw or call to super, then I think we're in agreement about what it should do.

[DanTup]

would it be possible to autocomplete with snippets that have tabstops for traversal instead of straight TODOs?

That way the user could tab through to important parts and fill them in quickly. We could have the throw be a placeholder at the tabstop so the user could leave it if they want to as well.

I think for a single insertion we could set the TODO (or throw) as the selection which would work well (I just tested, and selected an override from completion currently just puts the cursor at the end of the TODO). I don't think it works well when there are multiple (eg. when you use a quickfix to add missing overrides) because VS Code has some awkward behaviour with non-final tabstops (with default settings, code completion and <tab> do not work as you would expect when you haven't exited "snippet mode").

I agree that we don't need two markers that perform the same purpose, and I'd much rather get rid of the comment. If we always have a throw or call to super, then I think we're in agreement about what it should do.

Assuming we're agreed we should drop the TODOs and always use throw, I have a question about whether we should also use throw when generating super calls? If we generate a method stub with a call to super without a throw (eg. code that is valid and runs), why not do the same for those without super calls (for ex. a void-returning method can be valid with an empty body)? Would it be more consistent to always add throw (including after a super call), or to instead only add it where it is necessary for valid code?

[bwilkerson]

... whether we should also use throw when generating super calls?

I don't know.

One of the goals we have is that our fixes should not produce invalid code when it's reasonable not to. That's why we decided to use a throw in non-overriding methods that return a non-null value. In most cases we can't return a valid default (not that that would be a good choice for other reasons), so we added the throw to avoid another diagnostic. It then seemed reasonable to add a throw to every non-overriding method; it makes the tool more consistent and it marks the method as needing a better implementation.

But for overriding methods, a super invocation, with our without the return as appropriate, is a perfectly valid way to avoid follow-on diagnostics (with the exception of one lint that might or might not be enabled, which we might want to take into account). It also has the advantage that it's semantics preserving.

There's some value in adding a throw in overriding methods: it would mark the method as needing a better implementation, it would be consistent, and it would prevent the lint from firing if it's enabled. I don't know that it's enough value to make it worth forcing users to delete the line.

I guess my inclination at this point would be to not add throw after a super invocation, but I might be wrong.

[DanTup]

Thanks - that all makes sense to me.

with the exception of one lint that might or might not be enabled, which we might want to take into account

I presume you mean unnecessary_overrides? Would you suggest including a throw in that case? One one hand, it avoids the lint, but on the other hand it feels like it's deliberately side-stepping it - perhaps even undermining it if it was intended to catch incomplete overrides (I don't know if that's the case though)?

[bwilkerson]

I presume you mean unnecessary_overrides?

Yes.

... it feels like it's deliberately side-stepping it ...

The question is: when does server generate an override? I can think of two cases, though there are probably more:

• when the user types part of the name of an inherited member at the class member level and code completes, and
• when the code has an error because a concrete override is required but not found.

In neither of these cases is the override unnecessary, it's just incomplete. Adding a throw to side-step the lint doesn't feel terrible to me, but I also don't expect it's too much of a burden for the user to remember to ignore the lint for a couple of minutes until they've filled in the implementation. I'd be fine with either.

[gspencergoog]

We have that lint enabled, and I would be fine with either, with a slight bias towards not adding a throw just because it means I don't have to delete anything in order to start implementing, even though it adds a lint warning. And in projects where that lint isn't enabled, there's no reason to add the throw, and it'll make your logic simpler anyhow.

[Lootwig]

Is there a reason that the class is correctly highlighted as "overrides missing",

but when I tell my IDE I want to "override methods..." (Ctrl + O in IntelliJ), the missing ones aren't available?

EDIT: asking because IntelliJ's internal override would let me create the method stub without the annoying TODO comments I didn't ask for.

EDIT: found the answer, note my addition below that reply.