-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[ClangImporter] Don't crash when a bad override affects NSErrors. #7907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,16 @@ | ||
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-silgen -parse-as-library -verify %s | ||
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-sil -O -parse-as-library -DEMIT_SIL %s | ||
|
||
// REQUIRES: objc_interop | ||
|
||
import Foundation | ||
import errors | ||
|
||
#if !EMIT_SIL | ||
func test0() { | ||
try ErrorProne.fail() // expected-error {{errors thrown from here are not handled}} | ||
} | ||
#endif | ||
|
||
// Test "AndReturnError" stripping. | ||
// rdar://20722195 | ||
|
@@ -71,10 +74,12 @@ func testBlockFinal() throws { | |
try ErrorProne.runSwiftly(5000, callback: {}) | ||
} | ||
|
||
#if !EMIT_SIL | ||
func testNonBlockFinal() throws { | ||
ErrorProne.runWithError(count: 0) // expected-error {{missing argument for parameter #1 in call}} | ||
ErrorProne.run(count: 0) // expected-error {{incorrect argument label in call (have 'count:', expected 'callback:')}} | ||
} | ||
#endif | ||
|
||
class VeryErrorProne : ErrorProne { | ||
override class func fail() throws {} | ||
|
@@ -108,3 +113,24 @@ func testNSErrorExhaustive() { | |
} | ||
} | ||
} | ||
|
||
func testBadOverrides(obj: FoolishErrorSub) throws { | ||
try obj.performRiskyOperation() | ||
let _: FoolishErrorSub = try obj.produceRiskyOutput() | ||
let _: String = try obj.produceRiskyString() | ||
|
||
let _: NSObject = try obj.badNullResult() | ||
let _: CInt = try obj.badNullResult2() // This is unfortunate but consistent. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DougGregor, note this change. I checked the generated IR and there's not even a zero check here, because we treat "null" and "zero" as different error conventions (probably because of bridged types). But we don't crash, and the original code is arguably wrong. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original code is wrong. Maybe Clang should be diagnosing the bogus override (an |
||
let _: CInt = try obj.badZeroResult() | ||
try obj.badNonzeroResult() as Void | ||
|
||
let base = obj as SensibleErrorBase | ||
try base.performRiskyOperation() | ||
let _: NSObject = try base.produceRiskyOutput() | ||
let _: String = try base.produceRiskyString() | ||
|
||
let _: NSObject = try base.badNullResult() | ||
let _: NSObject = try base.badNullResult2() | ||
let _: CInt = try base.badZeroResult() | ||
try base.badNonzeroResult() as Void | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean that if a non-nil return type is returned the error is checked first in the generated code for the NSError?
e.g.
If so that might cause unexpected failures of un-initialized memory cases when passing into existing failure methods.
Or is this just for the type checker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have an Objective-C attribute form for that (
swift_error(nonnull_error)
), but we never infer it in the Clang importer specifically because we were concerned about the uninitialized memory cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I'll double-check that the generated code is still going to look for a nil value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Good question, to be sure!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does indeed still check for a nil return, without looking at the error.