Handle unexpected raw values for @objc enums in derived conformances #17836
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please benchmark |
|
cc @allevato |
|
Build failed |
slavapestov
approved these changes
Jul 9, 2018
|
|
||
| public enum E: Int { | ||
| case a, b, c | ||
| } | ||
|
|
||
| // CHECK-DAG: sil [serialized] @$S22enum_raw_representable1EO0B5ValueACSgSi_tcfC | ||
| // CHECK-DAG: sil [serialized] @$S22enum_raw_representable1EO0B5ValueSivg | ||
| @objc public enum CLike: Int { |
There was a problem hiding this comment.
I would prefer this was in its own file, like objc_enum_raw_representable.swift
|
This should be a small code size win too! |
itaiferber
suggested changes
Jul 9, 2018
| @@ -90,6 +90,27 @@ static void deriveBodyRawRepresentable_raw(AbstractFunctionDecl *toRawDecl) { | |||
| } | |||
| #endif | |||
|
|
|||
| if (enumDecl->isObjC()) { | |||
There was a problem hiding this comment.
This is a good optimization but isn't safe to perform on types which override rawValue for some reason, right?
@objc enum Foo : Int {
case a
var rawValue: Int {
return 5
}
}
let a = Foo.a
print(a.rawValue) // 5
print(unsafeBitCast(a, to: Int.self)) // 0We should probably see if rawValue was synthesized, and if so, bypass calling it with this, yeah?
There was a problem hiding this comment.
This is the derived implementation of rawValue; it'll only be used if the user didn't write one.
(If you're talking about the hash(into:) change, I suppose you're right, but we're doomed either way in that case.)
There was a problem hiding this comment.
Sorry, yes! I was looking in the wrong place. Trying to do too many things at once 🙃
There was a problem hiding this comment.
hash(into:) could use the bitcast approach; isn't that what rawValue does?
There was a problem hiding this comment.
I suppose it could. I liked the semantic implications of this one, but bitcasting is a little more correct. Do you think it's important enough to be worth changing?
There was a problem hiding this comment.
Having hashing use the bitcast value feels more inherently correct to me, since an @objc enum can in fact be of any value of its raw type, and a hash implementation ought to feed all of the significant bits of the value to the hasher.
There was a problem hiding this comment.
I realized that hash(into:) should probably stay consistent with ==, which has a default implementation for RawRepresentable types, so I'm going to leave this as is.
jrose-apple
force-pushed
the
ready-for-anything
branch
from
ba964d7 to
0bcd22d
Compare
|
Reorganized tests only. @swift-ci Please test Linux |
|
@swift-ci Please smoke test macOS |
|
Build failed |
itaiferber
approved these changes
Jul 9, 2018
|
(source compat bot is having a problem, so I'm going to hold off for a bit) |
jrose-apple
force-pushed
the
ready-for-anything
branch
from
0bcd22d to
38e0a47
Compare
|
@swift-ci Please smoke test macOS |
|
@swift-ci Please test Linux |
|
Build failed |
|
Unrelated SwiftPM failure? @swift-ci Please test Linux |
Build comment file:Optimized (O)Regression (1)
Improvement (1)
No Changes (456)
Unoptimized (Onone)Regression (4)
Improvement (5)
No Changes (449)
Hardware Overview |
|
Build failed |
Because people put all sorts of nonsense into @objc enums (most reasonably, "private cases", which represent valid values that are not API), the Swift-synthesized implementation of 'hash(into:)' needs to not expect a switch statement to be exhaustive. And since Swift-defined @objc enums are supposed to behave enough like C-defined enums, they should at least handle simple method calls with an invalid raw value, which means that 'rawValue' likewise should not use a switch. This patch provides alternate implementations that look like this: extension ImportedEnum { public var rawValue: Int { return unsafeBitCast(self, to: Int.self) } public func hash(into hasher: inout Hasher) { hasher.combine(self.rawValue) } } rdar://problem/41913284
jrose-apple
force-pushed
the
ready-for-anything
branch
from
38e0a47 to
a1d8acb
Compare
|
@swift-ci Please test |
|
(still waiting for the source compat bots to come back online) |
|
Build failed |
|
Build failed |
|
swiftlang/swift-source-compat-suite#213 |
|
Remaining source compat failures match the failures on master. Merging. |
jrose-apple
added a commit
to jrose-apple/swift
that referenced
this pull request
Jul 11, 2018
…wiftlang#17836) Because people put all sorts of nonsense into @objc enums (most reasonably, "private cases", which represent valid values that are not API), the Swift-synthesized implementation of 'hash(into:)' needs to not expect a switch statement to be exhaustive. And since Swift-defined @objc enums are supposed to behave enough like C-defined enums, they should at least handle simple method calls with an invalid raw value, which means that 'rawValue' likewise should not use a switch. This patch provides alternate implementations that look like this: extension ImportedEnum { public var rawValue: Int { return unsafeBitCast(self, to: Int.self) } public func hash(into hasher: inout Hasher) { hasher.combine(self.rawValue) } } rdar://problem/41913284 (cherry picked from commit 357c7d6)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because people put all sorts of nonsense into
@objcenums (most reasonably, "private cases", which represent valid values that are not API), the Swift-synthesized implementation ofhash(into:)needs to not expect a switch statement to be exhaustive. And since Swift-defined@objcenums are supposed to behave enough like C-defined enums, they should at least handle simple method calls with an invalid raw value, which means thatrawValuelikewise should not use a switch.This patch provides alternate implementations that look like this:
rdar://problem/41913284