Clean up _BridgedNSError and _BridgedStoredNSError #14682

jrose-apple · 2018-02-16T22:16:09Z

Despite their similar names and uses, these protocols no longer share much functionality - the former is used to take @objc enums defined in Swift that conform to Error and expose them as NSErrors, and the latter handles NS_ERROR_ENUM C enums, which get imported into Swift as a wrapper around NSError. We can actually simplify them quite a bit.

Eliminate base protocol __BridgedNSError, which no longer provides any implementation for _BridgedStoredNSError.
Eliminate default implementations that match what the compiler would synthesize.
Adopt recursive constraints and where-clauses on associated types (and update the Clang importer to handle this).
Collapse signed and unsigned default implementations when reasonable.
Fold _BridgedStoredNSError's _nsErrorDomain into the existing public requirement CustomNSError.errorDomain.

rdar://problem/35230080

@objc

Despite their similar names and uses, these protocols no longer share much functionality - the former is used to take @objc enums defined in Swift that conform to Error and expose them as NSErrors, and the latter handles NS_ERROR_ENUM C enums, which get imported into Swift as a wrapper around NSError. We can actually simplify them quite a bit. - Eliminate base protocol __BridgedNSError, which no longer provides any implementation for _BridgedStoredNSError. - Eliminate default implementations that match what the compiler would synthesize. - Adopt recursive constraints and where-clauses on associated types (and update the Clang importer to handle this). - Collapse signed and unsigned default implementations when reasonable. - Fold _BridgedStoredNSError's _nsErrorDomain into the existing public requirement CustomNSError.errorDomain. rdar://problem/35230080

jrose-apple · 2018-02-16T22:16:33Z

lib/ClangImporter/ImportDecl.cpp

+  if (auto *depBase = origBase->getAs<DependentMemberType>()) {
+    Type substBase = recursivelySubstituteBaseType(conformance, depBase);
+    ModuleDecl *module = conformance->getDeclContext()->getParentModule();
+    return depMemTy->substBaseType(module, substBase);


This definitely needs a check from someone who understands the ramifications better than I do.

This feels like it should be a Type::subst(), not its own subst-like recursive algorithm.

Oops, I forgot this change was in here, or I wouldn't have merged until you got a chance to review. IIRC we can't use Type::subst because we're still in the process of building the conformance, but I could be wrong.

jrose-apple · 2018-02-16T22:18:32Z

stdlib/public/SDK/Foundation/NSError.swift

-  /// The error code for the given error.
-  var code: Code { get }
+  associatedtype Code: _ErrorCodeProtocol, RawRepresentable
+  where Code.RawValue: FixedWidthInteger


This was the last thing I couldn't update. These constraints ought to be on _ErrorCodeProtocol itself, because there'll never be any _ErrorCodeProtocol types that don't match, but the compiler crashes if I try to do that when we actually go to use the conformance. I'll try to come up with a reduced case that doesn't depend on the importer.

Okay, no luck with a reduction, but that's because it's the ClangImporter code I mentioned above that's at fault. Clearly it needs more actual attention and not a quick fix.

jrose-apple · 2018-02-16T22:18:47Z

@swift-ci Please test

jrose-apple · 2018-02-16T22:18:53Z

@swift-ci Please test source compatibility

jrose-apple · 2018-02-16T22:18:58Z

@swift-ci Please test compiler performance

swift-ci · 2018-02-17T17:18:58Z

Build comment file:

Summary for master full

Unexpected test results, stats may be off for FAIL_Kronos-Kronos.xcodeproj_3.0_BuildXcodeProjectTarget_Kronos_generic-platform-tvOS.log, FAIL_RxDataSources-Pods-Pods.xcodeproj_3.0_BuildXcodeProjectTarget_Pods-RxDataSources_generic-platform-iOS.log, FAIL_Kronos-Kronos.xcodeproj_3.0_BuildXcodeProjectTarget_Kronos_generic-platform-iOS.log, FAIL_JSQDataSourcesKit-JSQDataSourcesKit.xcodeproj_4.0_BuildXcodeProjectTarget_JSQDataSourcesKit-iOS_generic-platform-iOS.log, FAIL_Dollar-Dollar.xcodeproj_3.0_BuildXcodeProjectScheme_Dollar_generic-platform-macOS.log, 3, FAIL_Kronos-Kronos.xcodeproj_3.0_BuildXcodeProjectTarget_Kronos_generic-platform-macOS.log, FAIL_RxDataSources-Pods-Pods.xcodeproj_3.0_BuildXcodeProjectTarget_Pods-Example_generic-platform-iOS.log

Regressions found (see below)

Debug
- debug brief
- debug detailed
Debug-opt
- debug-opt brief
- debug-opt detailed
Wmo-onone
- wmo-onone brief
- wmo-onone detailed
Release
- release brief
- release detailed

Debug

debug brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	1,066,190,277	1,066,718,638	528,361	0.05%
time.swift-driver.wall	2017.0s	2016.1s	-858.8ms	-0.04%

debug detailed

Regressed (2)

name	old	new	delta	delta_pct
Sema.NumLazyGenericEnvironments	9,509,446	9,635,503	126,057	1.33% ⛔
Sema.NumLazyIterableDeclContexts	7,611,167	7,754,764	143,597	1.89% ⛔

Improved (1)

name	old	new	delta	delta_pct
Sema.NumLazyGenericEnvironmentsLoaded	906,188	897,078	-9,110	-1.01% ✅

Unchanged (delta < 1.0% or delta < 100.0ms) (20)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	1,667,326	1,666,346	-980	-0.06%
AST.NumLoadedModules	333,643	333,701	58	0.02%
AST.NumTotalClangImportedEntities	5,187,865	5,187,206	-659	-0.01%
AST.NumUsedConformances	151,698	151,708	10	0.01%
IRModule.NumIRBasicBlocks	3,294,874	3,297,291	2,417	0.07%
IRModule.NumIRFunctions	1,669,789	1,671,098	1,309	0.08%
IRModule.NumIRGlobals	1,573,417	1,574,458	1,041	0.07%
IRModule.NumIRInsts	34,708,269	34,730,159	21,890	0.06%
IRModule.NumIRValueSymbols	2,884,729	2,886,811	2,082	0.07%
LLVM.NumLLVMBytesOutput	1,066,190,277	1,066,718,638	528,361	0.05%
SILModule.NumSILGenFunctions	1,019,044	1,019,468	424	0.04%
SILModule.NumSILOptFunctions	1,491,964	1,494,525	2,561	0.17%
Sema.NumConformancesDeserialized	6,056,662	6,074,019	17,357	0.29%
Sema.NumConstraintScopes	10,999,422	10,952,310	-47,112	-0.43%
Sema.NumDeclsDeserialized	48,652,057	48,989,180	337,123	0.69%
Sema.NumDeclsValidated	2,105,452	2,099,132	-6,320	-0.3%
Sema.NumFunctionsTypechecked	1,013,124	1,012,012	-1,112	-0.11%
Sema.NumGenericSignatureBuilders	1,668,176	1,657,153	-11,023	-0.66%
Sema.NumTypesDeserialized	50,833,079	51,103,542	270,463	0.53%
Sema.NumTypesValidated	5,266,213	5,257,100	-9,113	-0.17%

Debug-opt

debug-opt brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (1)

name	old	new	delta	delta_pct
time.swift-driver.wall	3569.6s	3432.0s	-137.6s	-3.85% ✅

Unchanged (delta < 1.0% or delta < 100.0ms) (1)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	1,041,679,752	1,041,729,552	49,800	0.0%

debug-opt detailed

Regressed (2)

name	old	new	delta	delta_pct
Sema.NumLazyGenericEnvironments	10,616,737	10,734,021	117,284	1.1% ⛔
Sema.NumLazyIterableDeclContexts	7,991,664	8,131,710	140,046	1.75% ⛔

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (21)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	1,661,576	1,661,931	355	0.02%
AST.NumLoadedModules	320,321	320,365	44	0.01%
AST.NumTotalClangImportedEntities	5,355,149	5,356,182	1,033	0.02%
AST.NumUsedConformances	151,326	151,351	25	0.02%
IRModule.NumIRBasicBlocks	3,588,816	3,589,107	291	0.01%
IRModule.NumIRFunctions	1,331,480	1,331,557	77	0.01%
IRModule.NumIRGlobals	1,325,666	1,325,703	37	0.0%
IRModule.NumIRInsts	28,105,597	28,106,535	938	0.0%
IRModule.NumIRValueSymbols	2,425,341	2,425,450	109	0.0%
LLVM.NumLLVMBytesOutput	1,041,679,752	1,041,729,552	49,800	0.0%
SILModule.NumSILGenFunctions	1,018,187	1,018,279	92	0.01%
SILModule.NumSILOptFunctions	2,062,857	2,063,241	384	0.02%
Sema.NumConformancesDeserialized	12,811,569	12,821,059	9,490	0.07%
Sema.NumConstraintScopes	10,982,228	10,939,687	-42,541	-0.39%
Sema.NumDeclsDeserialized	55,065,981	55,365,476	299,495	0.54%
Sema.NumDeclsValidated	2,089,428	2,089,521	93	0.0%
Sema.NumFunctionsTypechecked	1,008,746	1,008,939	193	0.02%
Sema.NumGenericSignatureBuilders	1,716,914	1,706,915	-9,999	-0.58%
Sema.NumLazyGenericEnvironmentsLoaded	926,942	917,634	-9,308	-1.0%
Sema.NumTypesDeserialized	59,474,517	59,712,308	237,791	0.4%
Sema.NumTypesValidated	5,242,366	5,242,918	552	0.01%

Wmo-onone

wmo-onone brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	964,764,054	964,849,560	85,506	0.01%
time.swift-driver.wall	1567.1s	1563.5s	-3.7s	-0.24%

wmo-onone detailed

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (23)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	200,425	200,518	93	0.05%
AST.NumLoadedModules	11,685	11,695	10	0.09%
AST.NumTotalClangImportedEntities	633,170	633,552	382	0.06%
AST.NumUsedConformances	155,292	155,350	58	0.04%
IRModule.NumIRBasicBlocks	2,655,775	2,656,543	768	0.03%
IRModule.NumIRFunctions	1,412,679	1,413,065	386	0.03%
IRModule.NumIRGlobals	1,295,738	1,295,993	255	0.02%
IRModule.NumIRInsts	30,829,106	30,835,408	6,302	0.02%
IRModule.NumIRValueSymbols	2,430,807	2,431,366	559	0.02%
LLVM.NumLLVMBytesOutput	964,764,054	964,849,560	85,506	0.01%
SILModule.NumSILGenFunctions	556,550	556,668	118	0.02%
SILModule.NumSILOptFunctions	616,645	616,832	187	0.03%
Sema.NumConformancesDeserialized	1,418,138	1,418,770	632	0.04%
Sema.NumConstraintScopes	10,322,308	10,279,802	-42,506	-0.41%
Sema.NumDeclsDeserialized	4,863,768	4,863,315	-453	-0.01%
Sema.NumDeclsValidated	986,734	986,849	115	0.01%
Sema.NumFunctionsTypechecked	312,895	312,958	63	0.02%
Sema.NumGenericSignatureBuilders	177,642	176,871	-771	-0.43%
Sema.NumLazyGenericEnvironments	834,974	833,656	-1,318	-0.16%
Sema.NumLazyGenericEnvironmentsLoaded	102,383	101,682	-701	-0.68%
Sema.NumLazyIterableDeclContexts	527,223	526,677	-546	-0.1%
Sema.NumTypesDeserialized	4,998,595	4,997,413	-1,182	-0.02%
Sema.NumTypesValidated	1,303,556	1,303,619	63	0.0%

Release

release brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	1,060,785,333	1,061,094,786	309,453	0.03%
time.swift-driver.wall	3412.1s	3390.1s	-22.1s	-0.65%

release detailed

Regressed (0)

name	old	new	delta	delta_pct

Improved (1)

name	old	new	delta	delta_pct
Sema.NumLazyGenericEnvironmentsLoaded	251,907	249,201	-2,706	-1.07% ✅

Unchanged (delta < 1.0% or delta < 100.0ms) (22)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	489,978	490,256	278	0.06%
AST.NumLoadedModules	59,188	59,217	29	0.05%
AST.NumTotalClangImportedEntities	1,564,041	1,565,188	1,147	0.07%
AST.NumUsedConformances	158,731	158,916	185	0.12%
IRModule.NumIRBasicBlocks	3,096,999	3,101,215	4,216	0.14%
IRModule.NumIRFunctions	1,304,967	1,305,950	983	0.08%
IRModule.NumIRGlobals	1,367,122	1,368,009	887	0.06%
IRModule.NumIRInsts	26,494,002	26,521,266	27,264	0.1%
IRModule.NumIRValueSymbols	2,430,171	2,431,797	1,626	0.07%
LLVM.NumLLVMBytesOutput	1,060,785,333	1,061,094,786	309,453	0.03%
SILModule.NumSILGenFunctions	594,065	594,426	361	0.06%
SILModule.NumSILOptFunctions	1,066,503	1,067,890	1,387	0.13%
Sema.NumConformancesDeserialized	4,541,083	4,550,349	9,266	0.2%
Sema.NumConstraintScopes	10,624,687	10,586,281	-38,406	-0.36%
Sema.NumDeclsDeserialized	14,137,377	14,167,931	30,554	0.22%
Sema.NumDeclsValidated	1,207,426	1,207,772	346	0.03%
Sema.NumFunctionsTypechecked	448,847	449,038	191	0.04%
Sema.NumGenericSignatureBuilders	496,665	493,688	-2,977	-0.6%
Sema.NumLazyGenericEnvironments	2,637,781	2,645,368	7,587	0.29%
Sema.NumLazyIterableDeclContexts	1,908,849	1,920,484	11,635	0.61%
Sema.NumTypesDeserialized	15,749,561	15,776,745	27,184	0.17%
Sema.NumTypesValidated	2,291,135	2,291,324	189	0.01%

jrose-apple · 2018-02-19T17:00:46Z

I'm counting that as a success. The numbers barely changed. (Still waiting on review. This isn't urgent.)

jrose-apple · 2018-03-07T18:13:14Z

Review ping

phausler

So you are now making errorDomain API? instead of underscored public?

Has this been passed to Cocoa API review?

jrose-apple · 2018-03-07T19:08:58Z

Ah, it's already API via the CustomNSError protocol. Sorry for not making that clear.

phausler

since it is an existing API the changes look non threatening to me, however truthfully I am not certain of exactly what the difference is here.

jrose-apple · 2018-03-07T21:14:05Z

It does mean that external clients may end up referencing the entry points directly, which implies that we wouldn't ever be able to remove them, but we wouldn't ever be able to remove them anyway because they're doing something useful. So maintaining that difference in the ABI doesn't seem useful to me.

jrose-apple requested review from phausler and DougGregor February 16, 2018 22:16

jrose-apple commented Feb 16, 2018

View reviewed changes

phausler reviewed Mar 7, 2018

View reviewed changes

phausler approved these changes Mar 7, 2018

View reviewed changes

jrose-apple merged commit e202e90 into swiftlang:master Mar 7, 2018

jrose-apple deleted the bridge-over-troubled-errors branch March 7, 2018 21:14

jrose-apple mentioned this pull request Mar 20, 2018

[ClangImporter] Add a comment to 'recursivelySubstituteBaseType' #15381

Merged

Clean up _BridgedNSError and _BridgedStoredNSError #14682

Clean up _BridgedNSError and _BridgedStoredNSError #14682

Uh oh!

Conversation

jrose-apple commented Feb 16, 2018

Uh oh!

jrose-apple Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

DougGregor Mar 9, 2018

Choose a reason for hiding this comment

Uh oh!

jrose-apple Mar 9, 2018

Choose a reason for hiding this comment

Uh oh!

jrose-apple Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

jrose-apple Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Feb 16, 2018

Uh oh!

jrose-apple commented Feb 16, 2018

Uh oh!

jrose-apple commented Feb 16, 2018

Uh oh!

swift-ci commented Feb 17, 2018

Build comment file:

Summary for master full

Debug

debug brief

debug detailed

Debug-opt

debug-opt brief

debug-opt detailed

Wmo-onone

wmo-onone brief

wmo-onone detailed

Release

release brief

release detailed

Uh oh!

jrose-apple commented Feb 19, 2018

Uh oh!

jrose-apple commented Mar 7, 2018

Uh oh!

phausler left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Mar 7, 2018

Uh oh!

phausler left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Mar 7, 2018

Uh oh!

Uh oh!