[ClangImporter] Don't crash when a bad override affects NSErrors. #7907
Conversation
|
@swift-ci Please test |
| // Check for a bad override. | ||
| if (resultTy->isVoid()) | ||
| return Type(); | ||
| return resultTy; |
There was a problem hiding this comment.
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.
- (nonnull id)foo:(NSError **_Nullable)error;
...
NSError *error;
id result = [something foo:&error];
if (error) {
assumeItFailed();
} else {
useResult(result);
} 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.
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.
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.
(Good question, to be sure!)
There was a problem hiding this comment.
It does indeed still check for a nil return, without looking at the error.
| // Check for a bad override. | ||
| if (resultTy->isVoid()) | ||
| return Type(); | ||
| return resultTy; |
There was a problem hiding this comment.
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.
lib/ClangImporter/ImportType.cpp
Outdated
| // represent that, but it shouldn't fall over either. | ||
| if (resultTy->isAnyClassReferenceType() | ||
| || resultTy->is<AnyFunctionType>() | ||
| || resultTy->getAnyPointerElementType()) |
There was a problem hiding this comment.
This is an odd set of conditions that excludes bridged types. adjustResultTypeForThrowingFunction 's caller allows bridging, so an NSString * _Nonnull result type would fail to import (because String doesn't meet any of those conditions) while an NSObject * _Nonnull result type would smooth over the issue in the ObjC headers.
If we're not going to rigorously check for consistency, then an isVoid check probably suffices here as in the other places.
Most of the time the name importer does a good job deciding whether to import a particular method as throwing or not. However, when a method is an override, it skips all that work and assumes the decisions made for the superclass method apply here as well---which makes sense, since you're going to get the subclass implementation if you call the superclass's entry point. This can really throw things off if the types /don't/ match up, though. Handle the one case where this is legal according to the rules of Objective-C, and make sure we don't import methods in the other cases. rdar://problem/30705461
jrose-apple
force-pushed
the
undeserved-assertions-for-erroneous-errors
branch
from
4ac3fd7 to
48db841
Compare
|
Latest version LGTM, thank you! |
| 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.
@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.
The original code is wrong. Maybe Clang should be diagnosing the bogus override (an int instead of an object pointer? seriously?), but I think our generated IR is reasonable.
|
@swift-ci Please smoke test |
…iftlang#7907) Most of the time the name importer does a good job deciding whether to import a particular method as throwing or not. However, when a method is an override, it skips all that work and assumes the decisions made for the superclass method apply here as well---which makes sense, since you're going to get the subclass implementation if you call the superclass's entry point. This can really throw things off if the types /don't/ match up, though. Handle the one case where this is legal according to the rules of Objective-C, and make sure we don't import methods in the other cases. rdar://problem/30705461
Most of the time the name importer does a good job deciding whether to import a particular method as throwing or not. However, when a method is an override, it skips all that work and assumes the decisions made for the superclass method apply here as well—which makes sense, since you're going to get the subclass implementation if you call the superclass's entry point. This can really throw things off if the types don't match up, though. Handle the one case where this is legal according to the rules of Objective-C, and make sure we don't import methods in the other cases.
rdar://problem/30705461