[CSRanking] Swift 4.1 compatibility hack for favouring properties on concrete types over protocols #18952
Conversation
…concrete types over protocols Changes in shadowing behaviour by swiftlang#15412 caused a property on a concrete type to no longer shadow a protocol property member, which created unintentional ambiguities in 4.2. This commit ensures we at least keep these cases unambiguous in Swift 5 under Swift 4 compatibility mode. This is intentionally narrow in order to best preserve source compatibility under Swift 4 mode by ensuring we don't introduce any new ambiguities. Resolves SR-7425, SR-7940 & SR-8343.
hamishknight
force-pushed
the
concrete-is-better-swift4
branch
from
fc74954 to
9268fcd
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
lib/Sema/CSRanking.cpp
Outdated
| auto *nominal2 = dc2->getSelfNominalTypeDecl(); | ||
| if (nominal1 && nominal2 && nominal1 != nominal2) { | ||
| isSwift4ConcreteOverProtocolVar1 = isa<ProtocolDecl>(nominal2); | ||
| isSwift4ConcreteOverProtocolVar2 = isa<ProtocolDecl>(nominal1); |
There was a problem hiding this comment.
It seems like these two lines would be a little clearer if they were actually looking at the decl associated with each variable rather than the other one, e.g.:
isSwift4ConcreteOverProtocolVar1 = !isa<ProtocolDecl>(nominal1);
isSwift4ConcreteOverProtocolVar2 = !isa<ProtocolDecl>(nominal2);
There was a problem hiding this comment.
And perhaps rename the LHS of each. I mention this because at first I thought I was looking at typos in the name on the RHS.
There was a problem hiding this comment.
@rudkx That's actually how I originally wrote it, but wasn't sure the extra visual noise was worth it – I'll swap it back :). Any suggestions for the naming of the LHS? I can't immediately think of a better alternative.
There was a problem hiding this comment.
@hamishknight Perhaps drop isVarAndNotProtocol{1,2}?
I don't think keeping Swift4 in the name adds a lot of meaning and it's pretty clear that it's only set under -swift-version 4 so I don't think there's going to be a lot of confusion over what it's for.
There was a problem hiding this comment.
@rudkx I agree that's better, thanks! I've gone ahead and made that change.
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
Source compat suite failed to clone again – shouldn't it automatically restart in cases like this? |
|
@swift-ci Please smoke test OS X platform |
|
@swift-ci Please test source compatibility |
|
@hamishknight I'm not sure how much sense it makes to automatically retry, even on what appear to be spurious infrastructure failures, since you may hit the same failure again immediately and you wouldn't want to just spin and retry if you have many jobs hitting the same issue. |
|
@rudkx Sure, you'd want to have a maximum number of retries (even if that was just 1). I only mention it because I know we already automatically restart jobs that grab the wrong commit SHA (though I'm not actually sure what causes those failures; perhaps restarting those cannot immediately lead to the same failure). |
|
@hamishknight I think those are typically caused by triggering another run either while one is in progress or after one has been triggered but not started. @shahmishal Mishal, any thoughts on automatically retrying jobs when we can determine that the cause was related to infrastructure/networking/etc. as opposed to something inherent in the changes we're attempting to build and test? |
|
Debug compat suite failure appears to be unrelated – the bot is consistently failing for: |
|
Yeah those are unrelated and are hitting with other PRs as well. |
|
@swift-ci Please smoke test and merge |
1 similar comment
|
@swift-ci Please smoke test and merge |
|
@rudkx Do you think this would be worth cherry picking to the 4.2 branch now that it's taking changes for a potential 4.2.1 release? IMO the risk should be minimal considering how narrow it is. |
|
Opened a 4.2 PR just in case: #19288 |
(This is the Swift 4 mode portion of #18927)
Changes in shadowing behaviour by #15412 caused a property on a concrete type to no longer shadow a protocol property member, which created unintentional ambiguities in 4.2. This PR ensures we at least keep these cases unambiguous in Swift 5 under Swift 4 compatibility mode. This is intentionally narrow in order to best preserve source compatibility under Swift 4 mode by ensuring we don't introduce any new ambiguities.
Resolves SR-7425.