Sequence/Iterator conformance broken in 5.7

[MaxDesiatov]

Describe the bug

Code utilizing Sequence and Iterator protocols that worked in previous versions of Swift no longer type-checks in 5.7 snapshots.

Steps To Reproduce

Try to build WasmTransformer with Xcode 14.0 beta 5.

Expected behavior

This code type-checks and builds as it did Swift 5.5/5.6

Screenshots

Environment (please fill out the following information)

• OS: macOS 12.5 and Ubuntu 20.04
• Xcode Version/Tag/Branch: 14.0 beta 5 (14A5294e), also reproducible on Linux with swiftlang/swift:nightly-5.7-focal image and 6fa8c6b08e85 hash.

Additional context

Here's the code snippet (not self-contained, but I hope it helps to pinpoint the issue) that causes these new errors:

public protocol VectorSectionReader: Sequence where Element == Result<Item, Error> {
    associatedtype Item
    var count: UInt32 { get }
    mutating func read() throws -> Item
}

public struct VectorSectionIterator<Reader: VectorSectionReader>: IteratorProtocol {
    private(set) var reader: Reader
    private(set) var left: UInt32
    init(reader: Reader, count: UInt32) {
        self.reader = reader
        self.left = count
    }
    private var end: Bool = false
    public mutating func next() -> Reader.Element? {
        guard !end else { return nil }
        guard left != 0 else { return nil }
        let result = Result(catching: { try reader.read() })
        left -= 1
        switch result {
        case .success: return result
        case .failure:
            end = true
            return result
        }
    }
}

extension VectorSectionReader {
    __consuming public func makeIterator() -> VectorSectionIterator<Self> {
        VectorSectionIterator(reader: self, count: count)
    }

    public func collect() throws -> [Item] {
        var items: [Item] = []
        items.reserveCapacity(Int(count))
        for result in self {
            try items.append(result.get())
        }
        return items
    }
}

[MaxDesiatov]

@hborla I have a feeling this can be related to primary associated type changes that landed in 5.7. Since you worked on those, would you have a moment to give some guidance on what caused the issue? Thanks!

[hborla]

@MaxDesiatov the type checking of for-in loops was re-worked in Swift 5.7 - the method call / variable synthesis is now done in the constraint system instead of SILGen to allow iterating over existential Sequence types: https://github.com/apple/swift/pull/59261

@xedin I remember there was another ambiguity related to that change. Any ideas here?

[MaxDesiatov]

I've also tried adding a constraint on Iterator to VectorSectionReader hoping that would resolve it:

public protocol VectorSectionReader: Sequence
where Element == Result<Item, Error>, Iterator == VectorSectionIterator<Self> {
  // ...
}

That makes it complain about circular references between VectorSectionReader and VectorSectionIterator, ultimately leading to a crash with this output:

/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:1:17: error: circular reference
public protocol VectorSectionReader: Sequence where Element == Result<Item, Error>, Iterator == VectorSectionIterator<Self> {
                ^
/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:7:15: note: through reference here
public struct VectorSectionIterator<Reader: VectorSectionReader>: IteratorProtocol {
              ^
/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:8:22: note: through reference here
    public typealias Element = Reader.Element
                     ^
/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:8:22: note: through reference here
    public typealias Element = Reader.Element
                     ^
/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:7:15: note: through reference here
public struct VectorSectionIterator<Reader: VectorSectionReader>: IteratorProtocol {
              ^
/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:1:17: note: through reference here
public protocol VectorSectionReader: Sequence where Element == Result<Item, Error>, Iterator == VectorSectionIterator<Self> {
                ^
/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:8:39: error: 'Element' is not a member type of type 'Reader'
    public typealias Element = Reader.Element
                               ~~~~~~ ^
/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:17:43: error: 'Element' is not a member type of type 'Reader'
    public mutating func next() -> Reader.Element? {
                                   ~~~~~~ ^
/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:7:15: error: type 'VectorSectionIterator<Reader>' does not conform to protocol 'IteratorProtocol'
public struct VectorSectionIterator<Reader: VectorSectionReader>: IteratorProtocol {
              ^
Swift.IteratorProtocol:2:20: note: protocol requires nested type 'Element'; do you want to add it?
    associatedtype Element
                   ^
swift-frontend: /home/build-user/swift/lib/AST/RequirementMachine/InterfaceType.cpp:232: swift::AssociatedTypeDecl *swift::rewriting::PropertyBag::getAssociatedType(swift::Identifier): Assertion `assocType != nullptr && "Need to look harder"' failed.
Please submit a bug report (https://swift.org/contributing/#reporting-bugs) and include the project and the crash backtrace.
Stack dump:
0.  Program arguments: /usr/bin/swift-frontend -frontend -c /WasmTransformer/Sources/WasmTransformer/BinaryFormat.swift /WasmTransformer/Sources/WasmTransformer/ByteEncodable.swift /WasmTransformer/Sources/WasmTransformer/InputByteStream.swift /WasmTransformer/Sources/WasmTransformer/LEB128.swift /WasmTransformer/Sources/WasmTransformer/OutputWriter.swift /WasmTransformer/Sources/WasmTransformer/Readers/CodeSectionReader.swift /WasmTransformer/Sources/WasmTransformer/Readers/ElementSectionReader.swift /WasmTransformer/Sources/WasmTransformer/Readers/FunctionSectionReader.swift /WasmTransformer/Sources/WasmTransformer/Readers/ImportSectionReader.swift /WasmTransformer/Sources/WasmTransformer/Readers/ModuleReader.swift /WasmTransformer/Sources/WasmTransformer/Readers/TypeSectionReader.swift -primary-file /WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift /WasmTransformer/Sources/WasmTransformer/Sections.swift /WasmTransformer/Sources/WasmTransformer/Trampoline.swift /WasmTransformer/Sources/WasmTransformer/Transformers/CustomSectionStripper.swift /WasmTransformer/Sources/WasmTransformer/Transformers/I64ImportTransformer.swift /WasmTransformer/Sources/WasmTransformer/Transformers/SizeProfiler.swift /WasmTransformer/Sources/WasmTransformer/Transformers/StackOverflowSanitizer+Fixtures.swift /WasmTransformer/Sources/WasmTransformer/Transformers/StackOverflowSanitizer.swift /WasmTransformer/Sources/WasmTransformer/WasmTransformer.swift -emit-dependencies-path /WasmTransformer/.build/aarch64-unknown-linux-gnu/debug/WasmTransformer.build/Readers/VectorSectionReader.d -emit-reference-dependencies-path /WasmTransformer/.build/aarch64-unknown-linux-gnu/debug/WasmTransformer.build/Readers/VectorSectionReader.swiftdeps -target aarch64-unknown-linux-gnu -Xllvm -aarch64-use-tbi -disable-objc-interop -I /WasmTransformer/.build/aarch64-unknown-linux-gnu/debug -color-diagnostics -enable-testing -g -module-cache-path /WasmTransformer/.build/aarch64-unknown-linux-gnu/debug/ModuleCache -swift-version 5 -Onone -D SWIFT_PACKAGE -D DEBUG -new-driver-path /usr/bin/swift-driver -empty-abi-descriptor -resource-dir /usr/lib/swift -enable-anonymous-context-mangled-names -module-name WasmTransformer -parse-as-library -o /WasmTransformer/.build/aarch64-unknown-linux-gnu/debug/WasmTransformer.build/Readers/VectorSectionReader.swift.o -index-store-path /WasmTransformer/.build/aarch64-unknown-linux-gnu/debug/index/store -index-system-modules
1.  Swift version 5.7-dev (LLVM d5f117e38620783, Swift 260a80fcce5a2ab)
2.  Compiling with the current language version
3.  While evaluating request TypeCheckSourceFileRequest(source_file "/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift")
4.  While evaluating request TypeCheckFunctionBodyRequest(WasmTransformer.(file).VectorSectionReader extension.collect()@/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:36:17)
5.  While type-checking statement at [/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:36:44 - line:43:5] RangeText="{
        var items: [Item] = []
        items.reserveCapacity(Int(count))
        for result in self {
            try items.append(result.get())
        }
        return items
    "
6.  While type-checking statement at [/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:39:9 - line:41:9] RangeText="for result in self {
            try items.append(result.get())
        "
7.  While type-checking-for-each statement at [/WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:39:9 - line:41:9] RangeText="for result in self {
            try items.append(result.get())
        "
8.  While type-checking-target starting at /WasmTransformer/Sources/WasmTransformer/Readers/VectorSectionReader.swift:39:9

[xedin]

The issue here is that makeIterator() on Sequence is preferred over the one declared in extension of VectorSectionReader but at the same time next() on VectorSectionIterator is preferred over next() on IteratorProtocol... I don't actually understand why ranking would prefer makeIterator() on Sequence, need to look at the code there but I suspect it's due to this Self generic parameter.

[hborla]

@MaxDesiatov Adding that same-type requirement on the extension fixes it:

extension VectorSectionReader where Iterator == VectorSectionIterator<Self> {
  __consuming public func makeIterator() -> VectorSectionIterator<Self> {
    VectorSectionIterator(reader: self, count: count)
  }

  public func collect() throws -> [Item] {
    var items: [Item] = []
    items.reserveCapacity(Int(count))
    for result in self {
      try items.append(result.get())
    }
    return items
  }
}

[MaxDesiatov]

Perfect, thank you! I guess I can keep this open as a regression?

[xedin]

@MaxDesiatov Does it make sense for VectorSectionReader to have a default makeIterator() that returns VectorSectionIterator<Self> or should it only happen when the underlying iterator is VectorSectionIterator as per @hborla's suggestion? I'm asking because some call sites could start behaving differently after the change.

@hborla makeIterator declared as a Sequence requirement is considered better because of this very old bug in ranking that considers protocol requirements as if they are declared in a concrete type - https://github.com/apple/swift/blob/main/lib/Sema/CSRanking.cpp#L416-L418. I have attempted fix this behavior multiple times but all the attempts resulted in source breaks. That said, we could re-evaluate for Swift 6...

[MaxDesiatov]

@MaxDesiatov Does it make sense for VectorSectionReader to have a default makeIterator() that returns VectorSectionIterator<Self> or should it only happen when the underlying iterator is VectorSectionIterator as per @hborla's suggestion? I'm asking because some call sites could start behaving differently after the change.

In this case default makeIterator() is preferred, but we don't have any other iterators declared for conforming types, and we don't expect them to be.

I'm not the original author of the code, but I'll let @kateinoigakukun correct me if I'm wrong: the whole purpose of VectorSectionReader is to provide that extension for a group of types with a predefined iterator and collect() function based on it. I guess it means that it won't behave differently with Holly's solution in our codebase.

[kateinoigakukun]

@MaxDesiatov as far as I remember, Max understands my intention for the code correctly. So it seems ok with Holly's suggestion in our case.