Add constraints to FixedWidthInteger.Stride and .Magnitude #17716

stephentyrone · 2018-07-03T20:34:47Z

Add the constraint that these associatedtypes themselves conform to FixedWidthInteger and to SignedInteger and UnsignedInteger, respectively. These are effectively part of the semantic requirement of the protocol already, because the requirements on .min and .max effectively force FixedWidthInteger to be twos-complement, which requires Magnitude not be Self for signed types. Also, in practice it's generally necessary to have these two constraints in order to effectively use the FixedWidthInteger protocol anyway; witness that by adding them to the protocol, we eliminate them as constraints from a number of extensions.

Resolves SR-8156.

Add the constraint that these associatedtypes themselves conform to FixedWidthInteger and to SignedInteger and UnsignedInteger, respectively. These are effectively part of the semantic requirement of the protocol already, because the requirements on .min and .max effectively force FixedWidthInteger to be twos-complement, which requires Magnitude not be Self for signed types. Also, in practice it's generally necessary to have these two constraints in order to effectively use the FixedWidthInteger protocol anyway; witness that by adding them to the protocol, we eliminate them as constraints from a number of extensions. This also resolves https://bugs.swift.org/browse/SR-8156

stephentyrone · 2018-07-03T20:35:33Z

@swift-ci test compiler performance

stephentyrone · 2018-07-03T20:43:19Z

Also, @airspeedswift if there's no perf regression, are you ok with pulling this into 4.2?

stephentyrone · 2018-07-03T20:47:11Z

@swift-ci please test

moiseev · 2018-07-03T20:56:35Z

stdlib/public/core/Integers.swift.gyb

@@ -2271,7 +2271,8 @@ extension BinaryInteger {
 /// other arithmetic methods and operators.
 public protocol FixedWidthInteger :
  BinaryInteger, LosslessStringConvertible
-  where Magnitude : FixedWidthInteger
+  where Magnitude : FixedWidthInteger & UnsignedInteger,
+           Stride : FixedWidthInteger & SignedInteger


nitpick: we usually don't align the :s.

boring =) I'll fix before committing.

moiseev · 2018-07-03T21:12:18Z

swift-ci · 2018-07-03T22:09:22Z

Build failed
Swift Test Linux Platform
Git Sha - 044c181

airspeedswift · 2018-07-03T22:58:22Z

How plausible is it that this could break source? Are there possible ways that you could be relying on not fulfilling these constraints and still get working code?

airspeedswift · 2018-07-03T22:58:28Z

@swift-ci please test source compatibility

airspeedswift · 2018-07-03T22:59:48Z

Linux failure unrelated?

@swift-ci please test linux platform

stephentyrone · 2018-07-04T00:11:59Z

@airspeedswift Re: breaking source. It's relatively low-risk because practically any concrete type conforming to FixedWidthInteger will already have the conformances. You would have to have defined a concrete fixed-size integer type whose stride was not a signed fixed-size integer, which is really going out of your way, or whose magnitude was not UnsignedInteger--this second possibility is the only one that worries me a little bit, but it's still pretty niche. The main risk is that if someone implemented their own fixed-size bignum using a sign-magnitude representation, they might have defined Magnitude == Self. This is probably a violation of the semantics documented for FixedWidthInteger, but in a subtle way, and it will have "worked" until now.

As far as generic code goes, any function definition that worked previously still works, and we haven't narrowed the set of candidate stdlib types at all, because they still conform to the same protocols, we just moved two requirements to the "right" place in the hierarchy.

For anyone not defining their own integer types, it's a non-issue.

swift-ci · 2018-07-04T05:20:39Z

Build comment file:

Summary for master full

Unexpected test results, excluded stats for Core, xcproj, StencilSwiftKit, CoreStore, Turnstile, ObjectMapper

Regressions found (see below)

Debug-batch
- debug-batch brief
- debug-batch detailed
Release
- release brief
- release detailed

Debug-batch

debug-batch 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,137,323,640	1,137,325,054	1,414	0.0%
time.swift-driver.wall	2213.0s	2234.6s	21.6s	0.98%

debug-batch detailed

Regressed (1)

name	old	new	delta	delta_pct
AST.NumASTBytesAllocated	27,518,189,065	28,213,293,935	695,104,870	2.53% ⛔

Improved (2)

name	old	new	delta	delta_pct
AST.NumSourceLinesPerSecond	1,770,416	1,728,123	-42,293	-2.39% ✅
Sema.SuperclassTypeRequest	132,285	55,989	-76,296	-57.68% ✅

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

name	old	new	delta	delta_pct
AST.NumDecls	82,964	82,964	0	0.0%
AST.NumDependencies	158,496	158,518	22	0.01%
AST.NumImportedExternalDefinitions	1,394,849	1,394,849	0	0.0%
AST.NumInfixOperators	26,388	26,388	0	0.0%
AST.NumLinkLibraries	0	0	0	0.0%
AST.NumLoadedModules	207,331	207,331	0	0.0%
AST.NumLocalTypeDecls	8	8	0	0.0%
AST.NumObjCMethods	22,546	22,546	0	0.0%
AST.NumPostfixOperators	14	14	0	0.0%
AST.NumPrecedenceGroups	16,201	16,201	0	0.0%
AST.NumPrefixOperators	127	127	0	0.0%
AST.NumReferencedDynamicNames	180	180	0	0.0%
AST.NumReferencedMemberNames	3,795,617	3,795,628	11	0.0%
AST.NumReferencedTopLevelNames	272,867	272,867	0	0.0%
AST.NumSourceBuffers	362,793	362,793	0	0.0%
AST.NumSourceLines	2,351,367	2,351,367	0	0.0%
AST.NumTotalClangImportedEntities	4,586,913	4,586,913	0	0.0%
AST.NumUsedConformances	196,467	196,457	-10	-0.01%
Driver.ChildrenMaxRSS	70,709,667,840	71,250,812,928	541,145,088	0.77%
Driver.DriverDepCascadingDynamic	0	0	0	0.0%
Driver.DriverDepCascadingExternal	0	0	0	0.0%
Driver.DriverDepCascadingMember	0	0	0	0.0%
Driver.DriverDepCascadingNominal	0	0	0	0.0%
Driver.DriverDepCascadingTopLevel	0	0	0	0.0%
Driver.DriverDepDynamic	0	0	0	0.0%
Driver.DriverDepExternal	0	0	0	0.0%
Driver.DriverDepMember	0	0	0	0.0%
Driver.DriverDepNominal	0	0	0	0.0%
Driver.DriverDepTopLevel	0	0	0	0.0%
Driver.NumDriverJobsRun	16,691	16,691	0	0.0%
Driver.NumDriverJobsSkipped	0	0	0	0.0%
Driver.NumDriverPipePolls	266,966	268,822	1,856	0.7%
Driver.NumDriverPipeReads	292,115	291,907	-208	-0.07%
Driver.NumProcessFailures	0	0	0	0.0%
Frontend.NumProcessFailures	0	0	0	0.0%
IRModule.NumIRAliases	13,236	13,236	0	0.0%
IRModule.NumIRBasicBlocks	3,757,962	3,757,962	0	0.0%
IRModule.NumIRComdatSymbols	0	0	0	0.0%
IRModule.NumIRFunctions	2,146,382	2,146,382	0	0.0%
IRModule.NumIRGlobals	2,274,223	2,274,218	-5	-0.0%
IRModule.NumIRIFuncs	0	0	0	0.0%
IRModule.NumIRInsts	42,169,073	42,169,073	0	0.0%
IRModule.NumIRNamedMetaData	81,820	81,820	0	0.0%
IRModule.NumIRValueSymbols	3,914,175	3,914,170	-5	-0.0%
LLVM.NumLLVMBytesOutput	1,137,323,640	1,137,325,054	1,414	0.0%
Parse.NumFunctionsParsed	149,311	149,311	0	0.0%
SILModule.NumSILGenDefaultWitnessTables	0	0	0	0.0%
SILModule.NumSILGenFunctions	1,529,462	1,529,462	0	0.0%
SILModule.NumSILGenGlobalVariables	24,821	24,821	0	0.0%
SILModule.NumSILGenVtables	16,289	16,289	0	0.0%
SILModule.NumSILGenWitnessTables	42,913	42,913	0	0.0%
SILModule.NumSILOptDefaultWitnessTables	0	0	0	0.0%
SILModule.NumSILOptFunctions	1,385,772	1,385,772	0	0.0%
SILModule.NumSILOptGlobalVariables	25,588	25,588	0	0.0%
SILModule.NumSILOptVtables	25,013	25,013	0	0.0%
SILModule.NumSILOptWitnessTables	74,612	74,612	0	0.0%
Sema.EnumRawTypeRequest	14,950	14,950	0	0.0%
Sema.InheritedTypeRequest	96,048	96,048	0	0.0%
Sema.NamedLazyMemberLoadFailureCount	23,483	23,483	0	0.0%
Sema.NamedLazyMemberLoadSuccessCount	3,941,281	3,940,856	-425	-0.01%
Sema.NominalTypeLookupDirectCount	28,663,848	28,661,864	-1,984	-0.01%
Sema.NumConformancesDeserialized	4,815,717	4,824,766	9,049	0.19%
Sema.NumConstraintScopes	10,288,732	10,288,790	58	0.0%
Sema.NumConstraintsConsideredForEdgeContraction	30,213,326	30,213,153	-173	-0.0%
Sema.NumDeclsDeserialized	33,901,345	33,969,559	68,214	0.2%
Sema.NumDeclsValidated	2,891,528	2,891,649	121	0.0%
Sema.NumFunctionsTypechecked	925,101	925,101	0	0.0%
Sema.NumGenericSignatureBuilders	1,624,362	1,617,735	-6,627	-0.41%
Sema.NumLazyGenericEnvironments	6,542,063	6,572,436	30,373	0.46%
Sema.NumLazyGenericEnvironmentsLoaded	745,060	745,061	1	0.0%
Sema.NumLazyIterableDeclContexts	5,633,443	5,663,771	30,328	0.54%
Sema.NumTypesDeserialized	35,639,107	35,901,938	262,831	0.74%
Sema.NumTypesValidated	3,039,718	3,039,780	62	0.0%
Sema.NumUnloadedLazyIterableDeclContexts	3,802,062	3,832,395	30,333	0.8%

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	988,672,576	988,672,244	-332	-0.0%
time.swift-driver.wall	3624.5s	3636.9s	12.4s	0.34%

release 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	309,933	309,933	0	0.0%
AST.NumLoadedModules	19,326	19,326	0	0.0%
AST.NumTotalClangImportedEntities	1,012,211	1,012,211	0	0.0%
AST.NumUsedConformances	201,980	201,970	-10	-0.0%
IRModule.NumIRBasicBlocks	3,133,390	3,133,390	0	0.0%
IRModule.NumIRFunctions	1,737,620	1,737,620	0	0.0%
IRModule.NumIRGlobals	1,890,833	1,890,828	-5	-0.0%
IRModule.NumIRInsts	29,850,598	29,850,598	0	0.0%
IRModule.NumIRValueSymbols	3,326,865	3,326,860	-5	-0.0%
LLVM.NumLLVMBytesOutput	988,672,576	988,672,244	-332	-0.0%
SILModule.NumSILGenFunctions	739,648	739,648	0	0.0%
SILModule.NumSILOptFunctions	999,915	999,915	0	0.0%
Sema.NumConformancesDeserialized	2,315,353	2,323,890	8,537	0.37%
Sema.NumConstraintScopes	10,170,429	10,170,502	73	0.0%
Sema.NumDeclsDeserialized	7,217,770	7,221,094	3,324	0.05%
Sema.NumDeclsValidated	2,086,914	2,086,922	8	0.0%
Sema.NumFunctionsTypechecked	418,120	418,120	0	0.0%
Sema.NumGenericSignatureBuilders	356,035	356,029	-6	-0.0%
Sema.NumLazyGenericEnvironments	1,249,483	1,250,908	1,425	0.11%
Sema.NumLazyGenericEnvironmentsLoaded	152,831	152,831	0	0.0%
Sema.NumLazyIterableDeclContexts	850,001	851,424	1,423	0.17%
Sema.NumTypesDeserialized	8,734,567	8,755,244	20,677	0.24%
Sema.NumTypesValidated	1,366,152	1,366,194	42	0.0%

xwu · 2018-07-04T13:16:13Z

@stephentyrone Would it help to make explicit in the documentation that FixedWidthInteger types are two's complement? I think a bunch of generic code makes that assumption already, and it would make the .Magnitude constraint a no-brainer.

stephentyrone · 2018-07-05T12:40:47Z

@xwu Yeah, I'll give some thought to what the right way to document that is. We don't strictly require that the actual internal representation be twos-complement, rather that the API surface behaves as though it were.

stephentyrone · 2018-07-05T15:57:45Z

@swift-ci smoke test

stephentyrone · 2018-07-05T16:18:51Z

@xwu further improvement to documentation of FixedWidthInteger: https://bugs.swift.org/browse/SR-8189

dabrahams · 2018-07-05T18:52:50Z

LGTM basically. The one question I have is whether it's right to constrain Stride to being signed?

stephentyrone · 2018-07-05T18:56:48Z

@dabrahams All strides are signed: associatedtype Stride : Comparable, SignedNumeric. The new requirement is that the stride of a fixed-width integer is a fixed-width integer.

dabrahams · 2018-07-05T18:58:58Z

stdlib/public/core/Integers.swift.gyb

@@ -2271,7 +2271,8 @@ extension BinaryInteger {
 /// other arithmetic methods and operators.
 public protocol FixedWidthInteger :
  BinaryInteger, LosslessStringConvertible
-  where Magnitude : FixedWidthInteger
+  where Magnitude : FixedWidthInteger & UnsignedInteger,
+        Stride : FixedWidthInteger & SignedInteger


@stephentyrone I'm referring to the line above. Either "& SignedInteger" is redundant—in which case you should drop it—or you're constraining Stride to be signed here. Or, I'm missing something ;-)

What we get from Strideable is SignedNumeric. SignedInteger is a very tiny refinement of BinaryInteger & SignedNumeric, but I think it's the correct requirement semantically.

…#17716) * Add constraints to FixedWidthInteger.Stride and .Magnitude Add the constraint that these associatedtypes themselves conform to FixedWidthInteger and to SignedInteger and UnsignedInteger, respectively. These are effectively part of the semantic requirement of the protocol already, because the requirements on .min and .max effectively force FixedWidthInteger to be twos-complement, which requires Magnitude not be Self for signed types. Also, in practice it's generally necessary to have these two constraints in order to effectively use the FixedWidthInteger protocol anyway; witness that by adding them to the protocol, we eliminate them as constraints from a number of extensions. This also resolves https://bugs.swift.org/browse/SR-8156

…17766) * Add constraints to FixedWidthInteger.Stride and .Magnitude Add the constraint that these associatedtypes themselves conform to FixedWidthInteger and to SignedInteger and UnsignedInteger, respectively. These are effectively part of the semantic requirement of the protocol already, because the requirements on .min and .max effectively force FixedWidthInteger to be twos-complement, which requires Magnitude not be Self for signed types. Also, in practice it's generally necessary to have these two constraints in order to effectively use the FixedWidthInteger protocol anyway; witness that by adding them to the protocol, we eliminate them as constraints from a number of extensions. This also resolves https://bugs.swift.org/browse/SR-8156

stephentyrone requested review from moiseev and airspeedswift July 3, 2018 20:35

moiseev reviewed Jul 3, 2018

View reviewed changes

Don't align colons.

b806538

dabrahams reviewed Jul 5, 2018

View reviewed changes

stephentyrone merged commit 86ba697 into swiftlang:master Jul 5, 2018

stephentyrone deleted the constrain-fixed-width-integer branch July 5, 2018 19:13

stephentyrone mentioned this pull request Jul 5, 2018

[4.2] Add constraints to FixedWidthInteger.Stride and .Magnitude (#17716) #17766

Merged

Azoy mentioned this pull request Jul 17, 2018

[SE-0202] Rename Random to SystemRandomNumberGenerator swiftlang/swift-evolution#879

Merged

stephentyrone mentioned this pull request Jul 3, 2018

[SR-8156] Difficult to use random integers in generic contexts #50688

Closed

Add constraints to FixedWidthInteger.Stride and .Magnitude #17716

Add constraints to FixedWidthInteger.Stride and .Magnitude #17716

Uh oh!

Conversation

stephentyrone commented Jul 3, 2018

Uh oh!

stephentyrone commented Jul 3, 2018

Uh oh!

stephentyrone commented Jul 3, 2018

Uh oh!

stephentyrone commented Jul 3, 2018

Uh oh!

moiseev Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

stephentyrone Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

moiseev commented Jul 3, 2018

Uh oh!

swift-ci commented Jul 3, 2018

Uh oh!

airspeedswift commented Jul 3, 2018

Uh oh!

airspeedswift commented Jul 3, 2018

Uh oh!

airspeedswift commented Jul 3, 2018

Uh oh!

stephentyrone commented Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Jul 4, 2018

Build comment file:

Summary for master full

Debug-batch

debug-batch brief

debug-batch detailed

Release

release brief

release detailed

Uh oh!

xwu commented Jul 4, 2018

Uh oh!

stephentyrone commented Jul 5, 2018

Uh oh!

stephentyrone commented Jul 5, 2018

Uh oh!

stephentyrone commented Jul 5, 2018

Uh oh!

dabrahams commented Jul 5, 2018

Uh oh!

stephentyrone commented Jul 5, 2018

Uh oh!

dabrahams Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

stephentyrone Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabrahams Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stephentyrone commented Jul 4, 2018 •

edited

Loading

stephentyrone Jul 5, 2018 •

edited

Loading