[ClangImporter] Fix NS_CLOSED_ENUM to validate raw values #85723
Conversation
an0
requested review from
Xazax-hun,
beccadax,
egorzhdan,
ian-twilightcoder,
j-hui and
susmonteiro
as code owners
j-hui
left a comment
j-hui
left a comment
There was a problem hiding this comment.
Thank you for your contribution! The compiler changes look good to me, aside from a question and a nit. I would like the test to be migrated to use StdlibUnittest though.
| // RUN: %empty-directory(%t) | ||
| // RUN: %target-build-swift %s -import-objc-header %S/Inputs/enum-closed-raw-value.h -o %t/a.out | ||
| // RUN: %target-codesign %t/a.out | ||
| // RUN: %target-run %t/a.out | %FileCheck %s |
There was a problem hiding this comment.
Can you please use StdlibUnittest and its expectEqual() functions, rather than pipe the output to FileCheck? See test/Interop/Cxx/enum/hashable-enums.swift for an example.
| @@ -0,0 +1,84 @@ | |||
| // RUN: %empty-directory(%t) | |||
| // RUN: %target-build-swift %s -import-objc-header %S/Inputs/enum-closed-raw-value.h -o %t/a.out | |||
There was a problem hiding this comment.
Can you please test this both with and without C++ interop?
|
|
||
| auto body = BraceStmt::create(ctx, SourceLoc(), {switchStmt}, SourceLoc(), | ||
| /*implicit*/ true); | ||
| return {body, /*isTypeChecked=*/false}; |
There was a problem hiding this comment.
Can you please elaborate on why isTypeChecked is returned as false here?
| auto body = BraceStmt::create(ctx, SourceLoc(), {switchStmt}, SourceLoc(), | ||
| /*implicit*/ true); | ||
| return {body, /*isTypeChecked=*/false}; | ||
| } else { |
There was a problem hiding this comment.
nit: no need for else branch, since the "then" branch returns at the end. This also minimizes the diff.
|
@swift-ci please test |
Add execution tests verifying that NS_CLOSED_ENUM (imported as @Frozen) properly validates raw values in init?(rawValue:), returning nil for invalid values instead of creating poison enum instances. These tests currently fail, demonstrating issue swiftlang#85701 where invalid raw values are accepted and later crash in switch statements.
Generate switch-based validation for @Frozen enum init?(rawValue:) instead of using unchecked reinterpretCast. This ensures only declared enum cases are accepted, returning nil for invalid raw values. For non-frozen enums (NS_ENUM), preserve existing reinterpretCast behavior for C compatibility. Fixes swiftlang#85701
|
@j-hui I've addressed all the review feedback:
|
| auto *caseBody = BraceStmt::create(ctx, SourceLoc(), {caseAssign}, | ||
| SourceLoc(), /*implicit*/ true); | ||
|
|
||
| auto *caseStmt = CaseStmt::createImplicit(ctx, CaseParentKind::Switch, |
There was a problem hiding this comment.
Great contribution! You could save a few bytes by generating one case statement with each known case as a series of labels, as their bodies are the same (reinterpret cast)
equivalent to the source:
switch rawValue {
case .first, .second, third:
reinterpretCast
default:
return nil
}
function merging may consolidate these blocks on the back end but better not to emit them in the first place
3 participants
Summary
Fixes #85701 -
NS_CLOSED_ENUMnow properly validates raw values ininit?(rawValue:), returningnilfor invalid values instead of creating invalid enum instances that crash later.Problem
When importing C enums marked with
NS_CLOSED_ENUM(which map to@frozenSwift enums), the synthesizedinit?(rawValue:)usedBuiltin.reinterpretCastwithout validation. This allowed creating enum instances with invalid raw values that would crash with "Fatal error: unexpected enum case" when used in switch statements.Solution
@frozenenums (NS_CLOSED_ENUM): Generate a switch statement that validates the raw value against all declared cases, returningnilfor invalid valuesNS_ENUM): Preserve existingreinterpretCastbehavior for C compatibilityImplementation
Modified
synthesizeEnumRawValueConstructorBody()inlib/ClangImporter/SwiftDeclSynthesizer.cppto detect frozen enums and generate switch-based validation, following the same pattern used for native Swift enums inDerivedConformanceRawRepresentable.cpp.Testing
Added comprehensive execution tests that verify:
nil(not crash)Performance
Matches native Swift enums.