Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sequence/Iterator conformance broken in 5.7 #60514

Closed
MaxDesiatov opened this issue Aug 11, 2022 · 9 comments · Fixed by #61091
Closed

Sequence/Iterator conformance broken in 5.7 #60514

MaxDesiatov opened this issue Aug 11, 2022 · 9 comments · Fixed by #61091
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself conformances Feature → protocol: protocol conformances regression type checker Area → compiler: Semantic analysis

Comments

@MaxDesiatov
Copy link
Member

MaxDesiatov commented Aug 11, 2022

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

Screenshot 2022-08-11 at 19 38 05

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 MaxDesiatov added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. type checker Area → compiler: Semantic analysis conformances Feature → protocol: protocol conformances labels Aug 11, 2022
@MaxDesiatov
Copy link
Member Author

MaxDesiatov commented Aug 11, 2022

@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
Copy link
Member

hborla commented Aug 11, 2022

@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: #59261

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

@MaxDesiatov
Copy link
Member Author

MaxDesiatov commented Aug 11, 2022

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
Copy link
Member

xedin commented Aug 11, 2022

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
Copy link
Member

hborla commented Aug 11, 2022

@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
Copy link
Member Author

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

@xedin xedin self-assigned this Aug 11, 2022
@xedin
Copy link
Member

xedin commented Aug 11, 2022

@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
Copy link
Member Author

@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
Copy link
Member

@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.

xedin added a commit to xedin/swift that referenced this issue Sep 13, 2022
…ntext

Prefer an overload of `makeIterator` that satisfies
`Sequence#makeIterator` requirement when `.makeIterator()`
is implicitly injected after sequence expression in `for-in`
loop context.

This helps to avoid issues related to conditional conformances
to `Sequence` that add `makeIterator` preferred by overloading
rules that could change behavior i.e.:

```swift
extension Array where Element == Double {
  func makeIterator() -> Array<Int>.Iterator {
    return [1, 2, 3].makeIterator()
  }
}

for i in [4.0, 5.0, 6.0] {
  print(i)
}
```

`makeIterator` declared in the `Array` extension is not a witness
to `Sequence#makeIterator` and shouldn't be used because that would
change runtime behavior.

Resolves: apple#60514
Resolves: apple#60958
xedin added a commit to xedin/swift that referenced this issue Sep 13, 2022
…ntext

Prefer an overload of `makeIterator` that satisfies
`Sequence#makeIterator` requirement when `.makeIterator()`
is implicitly injected after sequence expression in `for-in`
loop context.

This helps to avoid issues related to conditional conformances
to `Sequence` that add `makeIterator` preferred by overloading
rules that could change behavior i.e.:

```swift
extension Array where Element == Double {
  func makeIterator() -> Array<Int>.Iterator {
    return [1, 2, 3].makeIterator()
  }
}

for i in [4.0, 5.0, 6.0] {
  print(i)
}
```

`makeIterator` declared in the `Array` extension is not a witness
to `Sequence#makeIterator` and shouldn't be used because that would
change runtime behavior.

Resolves: apple#60514
Resolves: apple#60958
@AnthonyLatsis AnthonyLatsis added regression compiler The Swift compiler in itself labels Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself conformances Feature → protocol: protocol conformances regression type checker Area → compiler: Semantic analysis
Projects
None yet
5 participants