False positive when determining unrelated types

[osaxma]

This is issue was discovered in: https://dart-review.googlesource.com/c/sdk/+/351781

Take the following example:

// `dart:io` type mock
class FileEntitySystemIO {}

class FileIO implements FileEntitySystemIO {}

// `package:file` type mock
class FileEntitySystemFilePkg implements FileEntitySystemIO {}

class FileFilePkg implements FileIO, FileEntitySystemFilePkg {}

void main() {
  <FileEntitySystemFilePkg>[].contains(FileIO());
  //                                   ^^^^^^^^
  //      The argument type 'FileIO' isn't related to 'FileEntitySystemFilePkg'
}

In the example above, it's possible for <FileEntitySystemFilePkg>[] to contain FileIO (i.e. FileFilePkg) but the analyzer reports collection_methods_unrelated_type lint rule.

Code Sample as a Test Case

The following sample can be added to `test/rules/collection_methods_unrelated_type_test.dart` within `CollectionMethodsUnrelatedTypeIterableTest` : ```dart test_contains_related_by_supertype() async { // special case, see discussion at: // https://dart-review.googlesource.com/c/sdk/+/351781 await assertNoDiagnostics(''' // `dart:io` type mock class FileEntitySystemIO {} class FileIO implements FileEntitySystemIO {} // `package:file` type mock class FileEntitySystemFilePkg implements FileEntitySystemIO {} class FileFilePkg implements FileIO, FileEntitySystemFilePkg {} void main() { [].contains(FileIO()); } '''); } ```

Root Cause

Using the debugger for the case above, the two types are determined to be unrelated in here:

    } else {
      var sameSupertypes = leftElement.supertype == rightElement.supertype;

      // Unrelated Enums have the same supertype, but they are not the same element, so
      // they are unrelated.
      if (sameSupertypes && leftElement is EnumElement) {
        return true;
      }

      return (leftElement.supertype?.isDartCoreObject ?? false) ||
          !sameSupertypes;
    }

link to the snippet above: https://github.com/dart-lang/sdk/blob/f20cd26533aa9ab012fcb069a1cd8dfa17924098/pkg/linter/lib/src/util/dart_type_utilities.dart#L293-L304

The local variables in the above code for the given test code sample:

debugger:~$ leftElement
-> ClassElementImpl (class FileIO implements FileEntitySystemIO)

debugger:~$ rightElement
-> ClassElementImpl (class FileEntitySystemFilePkg implements FileEntitySystemIO)

debugger:~$ leftElement.supertype
-> InterfaceTypeImpl (Object)

debugger:~$ leftElement.allSupertypes
-> List (2 items)
  [0]: InterfaceTypeImpl (Object)
  [1]: InterfaceTypeImpl (FileEntitySystemIO)

debugger:~$ rightElement.supertype
-> InterfaceTypeImpl (Object)

debugger:~$ rightElement.allSupertypes
-> List (2 items)
  [0]: InterfaceTypeImpl (Object)
  [1]: InterfaceTypeImpl (FileEntitySystemIO)

I am not sure why leftElement.superType is returning Object instead of FileEntitySystemIO (and similarly for rightElement).

If that's intended, then I believe one possibility to fix this issue is by using allSuperTypes instead of superType to evaluate sameSupertypes, or using the leastUpperBound method (actually I am curious as of why it wasn't used in the first place -- more expensive to compute?).

[srawlins]

I am not sure why leftElement.superType is returning Object instead of FileEntitySystemIO (and similarly for rightElement).

The super type of FileIO and FileEntitySystemFilePkg, which each type implicitly extends, is implicitly Object for each. No bug.

The lint rule will always over report or under report, whether we consider two inequal types which share a common type in their super hierarchy.

If we consider FileIO and FileEntitySystemFilePkg as "related" because it's possible that there exists a type somewhere in the universe that subtypes each of them (such as FileFilePkg), then all types are related except a few odd cases for types with subtype restrictions like String and int. The rule don't help users catch unrelated type bugs if the rule is only concerned with a few types like this.

If we consider FileIO and FileEntitySystemFilePkg as "unrelated" because neither is a subtype of the other, then we do not allow for cases like FileFilePkg, and the user must write an // ignore: comment, or an as cast.

If that's intended, then I believe one possibility to fix this issue is by using allSuperTypes instead of superType to evaluate sameSupertypes, or using the leastUpperBound method (actually I am curious as of why it wasn't used in the first place -- more expensive to compute?).

I think we should go the other way and stop considering types which have the exact same super type to be equal. I think we might have just written that case to allow for protobuf message types and Map. But generally, if you have a List<A> and ask whether that contains a B, you may be looking at a bug. If it's intentional, write // ignore:. It's such a beautiful tool, // ignore:. I love it.

[osaxma]

The lint rule will always over report or under report, whether we consider two inequal types which share a common type in their super hierarchy.

Yeah I haven't given it a fair thought earlier as I was focused in solving the failed tests of 351781 and lost the big picture 😅.

I do agree with you, though. I think what I was suggesting would just replace few "false positives" with "false negatives" to cater to the given diamond hierarchy which may not be that prevalent anyway.