[swift] Fix edge case for disambiguating messages with same name #3443
Conversation
| val nameToModules: Map<String, List<String>> = existingTypeModuleName | ||
| .entries | ||
| .groupBy({ it.key.simpleName }, { it.value }) | ||
| .mapValues { it.value.distinct() } |
There was a problem hiding this comment.
This change is irrelevant to the fix but it is correct so I sneaked it in.
| extension DateTime : Proto2Codable { | ||
|
|
||
| public init(from protoReader: ProtoReader) throws { | ||
| var value: module_one.DateTime? = nil |
There was a problem hiding this comment.
This is the line that is fixed with the SwiftPoet bump. Without it it emits var value: DateTime? = nil causing an ambiguous type error.
It now properly disambiguates it.
| dependencies: ["Wire"], | ||
| path: "wire-tests-swift/no-manifest/src/main/swift" | ||
| ), | ||
| .target( |
There was a problem hiding this comment.
These changes make so our manifest test harness is built in Xcode to verify the compiler errors or successful builds.
There was a problem hiding this comment.
Would these work as testTargets instead so they aren't exported to everyone using Wire via SPM?
There was a problem hiding this comment.
oooh good question. Yeah I can do that!
There was a problem hiding this comment.
@jszumski I dont think its possible actually. I cant have them depend on each other if they are testTargets according to SPM. Not sure if there is an SPM way to do this...
Seems like we are already doing this for the other target:
.target(
name: "WireTests",
dependencies: ["Wire"],
path: "wire-tests-swift/no-manifest/src/main/swift"
),
There was a problem hiding this comment.
Does:
products: [
.library(
name: "Wire",
targets: ["Wire"]
),
],
only mean it exposes this target?
There was a problem hiding this comment.
Yeah actually I think that prevents my original concern 👍
cb6c872 to
c73bdb7
Compare
SwiftPoet would not emit fully qualified type name if it shared the same simple name of a different module. This would cause ambiguous compiler errors.
The actual fix is in SwiftPoet outfoxx/swiftpoet#123 so this PR adds a test case and bumps SwiftPoet.
Here's a screenshot of it failing here before the bump: