Repository navigation
Create Missing Overrides produces linked edits with unexpected non-nullable type options #4185
Description
@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:
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).
Activity
bwilkerson commentedon Sep 28, 2022
I agree that we shouldn't be suggesting types (like
Object) that aren't valid in the given location.I believe that that would solve the problem here, but I'm wondering whether there might not be a deeper problem.
@scheglov Given an
InterfaceTyperepresentingint?(for example), what shouldsuperclassreturn? Should it be the type fornumornum??scheglov commentedon Sep 28, 2022
A couple of ideas.
_addSuperTypesAsSuggestionsuseInterfaceElement.allSupertypesinstead of diving intoget interfacesand usingalreadyAdded._addSuperTypesAsSuggestionsis not quite correct - it adds the type itself as well, not only its super-types.withNullabilitymust betrueif the target libraryisNonNullableByDefault.InterfaceTypenullability suffix is notnone, the same nullability suffix should be applied to elements ofallSupertypes.DartLinkedEditBuilderImplTestthat should be updated, maybe more than it seems.bwilkerson commentedon Sep 28, 2022
I'm guessing that the answer to my question is that
superclassis intentionally not equivalent tosupertype.scheglov commentedon Sep 28, 2022
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. specificX<int>inclass A extends X<int> {}. So, we returnallSupertypesfrom which, if you want, you can askInterfaceType.element(2)to get its "class".And here we fill the linked group with types.
DanTup commentedon Sep 29, 2022
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_writeTypedoes). 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 likeallSupertypesshould probably propagate the nullability suffix anyway, so I did that.[analyzer] Propagate nullability in InterfaceType.allSupertypes
DanTup commentedon Nov 18, 2022
Fixed by dart-lang/sdk@3f206da.