Create Missing Overrides produces linked edits with unexpected non-nullable type options

[DanTup]

@bwilkerson while fixing #4184 I noticed these edit groups being produced:

Sep-28-2022.16-24-21.mp4

Here, we're creating linked edit groups with suggestions for Object? and Object where it seems like only Object? is allowed. I believe it's because of withNullability: false here:

https://github.com/dart-lang/sdk/blob/e08c94a65147aaa3ec8c4e09fd1935f20599eec2/pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart#L1936

I think perhaps it should be more like:

withNullability: type.nullabilitySuffix != NullabilitySuffix.none

However, in the case where there are additional supertypes, that only includes the suffix on the original type. So I think we need to capture the suffix for the original type, and just manually append that onto the result of getDisplayString(withNullability: false) for each result.

Before I do this, a) do you know if there are any cases where the current behaviour is expected (or the described behaviour above would be worse), or whether there's something existing that's cleaner than manipulating strings here to get type.superclass where the result has the nullability suffix propagated?

(I'm also not entirely sure we should be adding tabstops on these types/param names.. you can tab through them, but not the bodies of the inserted methods... but that's another issue).

[bwilkerson]

I agree that we shouldn't be suggesting types (like Object) that aren't valid in the given location.

So I think we need to capture the suffix for the original type ...

I believe that that would solve the problem here, but I'm wondering whether there might not be a deeper problem.

@scheglov Given an InterfaceType representing int? (for example), what should superclass return? Should it be the type for num or num??

[scheglov]

A couple of ideas.

1. In _addSuperTypesAsSuggestions use InterfaceElement.allSupertypes instead of diving into get interfaces and using alreadyAdded.
2. The name _addSuperTypesAsSuggestions is not quite correct - it adds the type itself as well, not only its super-types.
3. withNullability must be true if the target library isNonNullableByDefault.
4. If the given InterfaceType nullability suffix is not none, the same nullability suffix should be applied to elements of allSupertypes.
5. There are tests in DartLinkedEditBuilderImplTest that should be updated, maybe more than it seems.

[bwilkerson]

I'm guessing that the answer to my question is that superclass is intentionally not equivalent to supertype.

[scheglov]

I actually find this terminology somewhat confusing, maybe because different people mean different things. For me, when I see "class", it means actual "class X {}" or mixin X<T> {}. But "type" is an instantiated "class", i.e. specific X<int> in class A extends X<int> {}. So, we return allSupertypes from which, if you want, you can ask InterfaceType.element(2) to get its "class".

And here we fill the linked group with types.

[DanTup]

I'm assuming that we should not output the *, and only the ? - (my understanding is that star is used in displays to indicate a type is legacy/unknown nullability, and it's never written in code?)

In which case, we can't use getDisplayString(withNullability: true) and need to instead add the ? suffix manually (which seems to be what _writeType does). I've made the change here:

https://dart-review.googlesource.com/c/sdk/+/261840/

I think we could've done this without updating allSupertypes (we could just prepare a suffix based on the original type), but based on the discussion about it sounds like allSupertypes should probably propagate the nullability suffix anyway, so I did that.

[DanTup]

Fixed by dart-lang/sdk@3f206da.