[SR-6690] ObjectMapper project in source compatibility suite broken by fix for SR-6685

[rudkx]

Previous ID	SR-6690
Radar	rdar://problem/40863157
Original Reporter	@rudkx
Type	Bug
Status	Resolved
Resolution	Done

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug
Assignee	@clackary
Priority	Medium

md5: 856cbd26e1ce6ec8571a8765cc6aa095

Issue Description:

The fix for https://bugs.swift.org/browse/SR-6685 exposes a problem in the ObjectMapper project in the source compatibility suite.

The project has functions overloaded by both 'inout T?' and 'inout T!', which should never have been allowed by the compiler for any version of Swift.

For example:

ObjectMapper/Sources/TransformOperators.swift:58:13: error: invalid redeclaration of '<-'
public func <- <Transform: TransformType>(left: inout Transform.Object!, right: (Map, Transform)) 

ObjectMapper/Sources/TransformOperators.swift:36:13: note: '<-' previously declared here
public func <- <Transform: TransformType>(left: inout Transform.Object?, right: (Map, Transform)) 

ObjectMapper/Sources/TransformOperators.swift:115:13: error: invalid redeclaration of '<-'
public func <- <Transform: TransformType>(left: inout [Transform.Object]!, right: (Map, Transform))
...etc...

[rudkx]

I think we're going to need to ask the project to update here.

The changes I'm preparing for eliminating IUOs from the type system will expose the same problem and will necessarily mean that we cannot put this under some form of version check.

[belkadan]

cc @lplarson

I'd be happy to say that this is downgraded to a warning in old modes, but that the function is then marked unavailable or something so that it never gets picked in overload resolution. That's better than just making it a hard error all the time.

[rudkx]

@belkadan I don't see a way to do anything like that.

The problem is that we literally replace IUO with Optional in generating the OverloadSignature InterfaceType, but were missing the case where it was an inout IUO, so we've let this slip through since the beginning.

Removing {{IUO}} from the type system makes it impossible to distinguish the two cases.

[belkadan]

I don't see an easy way to do it, but in practice we could hardcode something like "does this function have inout IUO as an argument type, if so mark it unavailable instead of emitting the warning".

[rudkx]

I'll look into whether there is a better option here. My concern is that having two OverloadSignature objects that are identical actually exist (rather than have the redeclaration be rejected) will lead to other problems.

I can certainly look at hacking the diagnostic to emit a warning in these cases, but I'll point out that changing the behavior os that the IUO overload is unavailable is in itself another source compatibility break (today we call the T arguments). That break will also come with removing IUO from the type system of course.

@DougGregor any thoughts on this?

[rudkx]

I looked into this a little further. Once IUOs are removed from the type system we'll mangle the two names the same way which obviously results in lots of badness. I don't think there's a good answer here. As I said with IUOs in the type system we can certainly conditionalize this, but once they're removed we're back to a breaking change here.

[DougGregor]

The best we could probably do here would be to emit a warning in Swift 4.1 (but otherwise not change the behavior), which will become a hard error in Swift 5 when the two declarations would collide. Beyond "it's nice to provide a warning before breaking things", this has a second advantage: with the elimination of IUOs from the type system, something like this will work:

func f(_: inout Int?) { }
var x: Int! = nil
f(&x) // currently an error because IUO types != Optional types.

So the correct fix in Swift 5 will be "delete the IUO overload"; one won't have to change any IUO callers.

[rudkx]

I updated #13680 to emit the warning rather than error on master, and will cherry-pick this to swift-4.1-branch.

Leaving this open to track the fact that the issue will still need to be addressed once the IUO changes go in.

[rudkx]

In #14244 I added the ability pass as inout Optional<T> in place of ImplicitlyUnwrappedOptional<T> (and vice-versa) in the Swift 4.1 release (also merged into master and the swift-5.0-branch).

This makes the previous warning something you can take action on - remove the second overload and code should continue to work.

My PR for removing IUO from the type system is now here: #14299

I expect to merge very soon (possibly today).

The PR to XFAIL ObjectMapper is here: swiftlang/swift-source-compat-suite#127

SwiftLint is using an older version of SourceKitten that does not include @jpsim PR jpsim/SourceKitten#445 so that will need to be XFAILed as well until the dependencies are updated.

[swift-ci]

Comment by Nicole Jacque (JIRA)

@rudkx, is this getting fixed on 4.1 and 5.0 branches? That PR was only on master.

[rudkx]

najacque (JIRA User) The ObjectMapper code needs to be changed to remove the inout T and vice-versa.

[swift-ci]

Comment by Nicole Jacque (JIRA)

Ok, so it sounds like we need to ask ObjectMapper's maintainers to make a change. @lplarson can you please reach out to the maintainer?

[lplarson]

Reached out to project maintainer requesting a project hash update with a fix.

[swift-ci]

Comment by Nicole Jacque (JIRA)

@swift-ci create

[clackary]

I've sent another email requesting an updated hash.

[clackary]

Looks like they've updated the project to build with 4.2, and it passes a project_precommit_check. I've opened a pull request to update a new hash in the suite.

swiftlang/swift-source-compat-suite#227

[clackary]

Testing passed, #227 merged. Issue resolved.