Code completion shows getter type as nullable for type that overrides to non-nullable

Describe the bug
When a class has an implementation that overrides a getter (and also marks the type as non-nullable), the getter type is still shown as nullable in the autocomplete suggestion.

https://github.com/dart-lang/sdk/blob/fb43937e201b76a3a53cb5bb6a001b4c79463939/pkg/analyzer/lib/dart/element/element.dart#L1706

To Reproduce
Start typing name prop of InterfaceOrAugmentationElement.

Please complete the following information:

Operating System and version: macOS
VS Code version: 1.77.3
Dart extension version: v3.63.20230405
Dart/Flutter SDK version: 2.19.6

@bwilkerson

@bwilkerson I reduced this down to the following code - a class that implements two other classes, one with a nullable getter and one with a non-nullable getter:

void f(MyItem item) {
  item.na^ // invoke completion here
}

abstract class NullableName {
  String? get name;
}

abstract class NotNullableName implements NullableName {
  @override
  String get name;
}

abstract class MyItem implements NotNullableName, NullableName {}

The class MyItem implements both NotNullableName and NullableName (in the real analyzer code this is essentially InterfaceOrAugmentationElement and Element) which means it has both String? get name and String get name. It's not clear to me what the rules are for resolving this (but I presume it's allowed for getters and the narrowest type wins).

Completion discovers both getters. First it finds String?, but then it skips String because _shouldAddSuggestion returns false. Specifically, this code:

        // Getters, fields, and methods shadow a getter.
        if ((alreadyGenerated & _COMPLETION_TYPE_GETTER) != 0) {
          return false;
        }

I'm not sure I understand the logic here though. The comment suggests everything but a setter can shadow a getter, but if I understand correctly, this code will return false if both items are getters (whereas it seems like setter is the exception). I also noticed that if NotNullableName doesn't also implement NullableName the issue doesn't occur (though I'm not sure certain why).

It's not clear to me what the rules are for resolving this (but I presume it's allowed for getters and the narrowest type wins).

Your instincts are correct. An instance of a subclass of MyItem can be passed where a NotNullableName is expected, and for soundness reasons it is therefore required to implement the narrower interface (String get name;). You can see this experimentally by attempting to define String? get name; in MyItem and being told that it isn't a valid override.

Completion discovers both getters.

Completion is probably either walking the type hierarchy manually, or it's using InheritanceManager3. My guess is that it's the former, and that that's where the problem is. If it's doing the latter, then there's probably a bug in InheritanceManager3.

I'm not sure I understand the logic here though.

And I'm not sure the logic is correct here. :-)

But I believe that the intent is to prevent us from displaying multiple suggestions for the same string. It doesn't really matter much to the user whether they're inserting a getter named foo or a setter named foo, it's the same characters being inserted. But a decision was made at some point that if there's both a getter and a setter it was better to display the icon for a getter rather than the icon for a setter, so that's what this is trying to do.

Completion is probably either walking the type hierarchy manually, or it's using InheritanceManager3. My guess is that it's the former, and that that's where the problem is. If it's doing the latter, then there's probably a bug in InheritanceManager3.

Aha, seems you're right. Switching it to InheritanceManager3 seems to resolve the issue. I don't know about the override code being correct, but now it'll only find the correct accessor so it doesn't need to figure out which one to use.

However, I think I'm using InheritanceManager3 incorrectly or I need an extra step to ensure type arguments are retained because there are a few test failures. I'll push a CL with an example marked for your input. Thanks!

Actually, there are some other failures with this change. One test does this:

class Base {
  int get x => null;
}
class Derived extends Base {
  int x;
}
void f(Derived d) {
  d.x^
}

And the test (test_shadowing_getter_over_field) expects to get the Base version but now we get Derived. I'm not sure I understand why the test expects Base though, as if I go-to-definition on x at that location I would be taken to the Derived field.

I can't think of any reason to expect the getter in Base. Konstantin postulated that the test might have been written to test the implementation rather than the correct semantics, and that seems to me to be a reasonable guess. Just update the test and I think it's fine.

[analysis_server] Switch TypeMemberContributor to using InheritanceMa…

Fixed by dart-lang/sdk@ee178b2. It'll show up in a future SDK release.

Thanks!

Navigation Menu

Repository navigation

Code completion shows getter type as nullable for type that overrides to non-nullable #4499

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Code completion shows getter type as nullable for type that overrides to non-nullable #4499

Description

Activity

DanTup commented on Apr 19, 2023

bwilkerson commented on Apr 19, 2023

DanTup commented on Apr 20, 2023

DanTup commented on Apr 20, 2023

bwilkerson commented on Apr 20, 2023

DanTup commented on Apr 21, 2023

incendial commented on Apr 21, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions