Skip to content

Commit

Permalink
[stdlib] Remove magnitude-based overload of abs(_:).
Browse files Browse the repository at this point in the history
The standard library has two versions of the `abs(_:)` function:

```
func abs<T : SignedNumeric>(_ x: T) -> T where T.Magnitude == T
func abs<T : SignedNumeric & Comparable>(_ x: T) -> T
```

The first is more specialized than the second because `T.Magnitude` is
known to conform to `Comparable`. Indeed, it’s a more specialized
implementation that returns `magnitude`.

However, this overload behaves oddly: in the expression `abs(-8)`, the type
checker will pick the first overload because it is more specialized. That’s
a general guiding principle for overloading: pick the most specialized
overload that works.

However, to select that overload, it needs to pick a type for the literal
“8” for which that overload works, and it chooses `Double`. The “obvious”
answer, `Int`, doesn’t work because `Int.Magnitude == UInt`.

There is a conflict between the two rules, here: we prefer more-specialized
overloads (but we’ll fall back to less-specialized if those don’t work) and we prefer to use `Int` for integer literals (but we’ll fall back to `Double` if it doesn’t work). We have a few options from a type-checker
perspective:

1. Consider the more-specialized-function rule to be more important
2. Consider the integer-literals-prefer-`Int` rule to be more important
3. Call the result ambiguous and make the user annotate it

The type checker currently does #1, although at some point in the past it
did #2. Moving forward, #1 is a better choice because it prunes the number
of overloads that need to be considered: if the more-specialized overload
succeeds its type-check, the others need not be considered. It’s also
easier to reason about than the literal-scoring approach, because there can
be a direct definition for “more specialized than” that can be reasoned
about.

I think we should dodge the issue by removing the more-specialized version
of `abs(_:)`. Its use of `magnitude` seems unlikely to provide a
significant performance benefit, and the presence of overloading either
forces us to consider both overloads always (which is bad for type checker
performance) or accept the regression that `abs(-8)` is `Double`. Better
to eliminate the overloading and, if needed in the future, find a better
way to introduce the more-specialized implementation without it being a
separate signature.

Fixes rdar://problem/42345366.
  • Loading branch information
DougGregor committed Dec 5, 2018
1 parent 58e855e commit 85d488d
Show file tree
Hide file tree
Showing 5 changed files with 8 additions and 23 deletions.
11 changes: 0 additions & 11 deletions stdlib/public/core/Integers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -318,17 +318,6 @@ extension SignedNumeric {
}
}


/// Returns the absolute value of the given number.
///
/// - Parameter x: A signed number.
/// - Returns: The absolute value of `x`.
@inlinable
public func abs<T : SignedNumeric>(_ x: T) -> T
where T.Magnitude == T {
return x.magnitude
}

/// Returns the absolute value of the given number.
///
/// The absolute value of `x` must be representable in the same type. In
Expand Down
12 changes: 0 additions & 12 deletions test/SourceKit/CodeComplete/complete_moduleimportdepth.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,6 @@ func test() {
// CHECK: key.modulename: "Swift"
// CHECK-NEXT: },

// CHECK-LABEL: key.name: "abs(:)",
// CHECK-NEXT: key.sourcetext: "abs(<#T##x: Comparable & SignedNumeric##Comparable & SignedNumeric#>)",
// CHECK-NEXT: key.description: "abs(x: Comparable & SignedNumeric)",
// CHECK-NEXT: key.typename: "Comparable & SignedNumeric",
// CHECK-NEXT: key.doc.brief: "Returns the absolute value of the given number.",
// CHECK-NEXT: key.context: source.codecompletion.context.othermodule,
// CHECK-NEXT: key.moduleimportdepth: 1,
// CHECK-NEXT: key.num_bytes_to_erase: 0,
// CHECK-NOT: key.modulename
// CHECK: key.modulename: "Swift"
// CHECK-NEXT: },

// FooHelper.FooHelperExplicit == 1
// CHECK-LABEL: key.name: "fooHelperExplicitFrameworkFunc1(:)",
// CHECK-NEXT: key.sourcetext: "fooHelperExplicitFrameworkFunc1(<#T##a: Int32##Int32#>)",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Func _int64ToStringImpl(_:_:_:_:_:) has been removed
Func _uint64ToStringImpl(_:_:_:_:_:) has been removed
Func _typeByMangledName(_:substitutions:) has been removed
Func _withUninitializedString(_:) has been removed
Func abs(_:) has been removed

Func Optional.unwrappedOrError() has been removed
Struct _UnwrappingFailed has been removed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ Func UnsafeMutableRawPointer.copyBytes(from:count:) has been removed (deprecated
Func UnsafeMutableRawPointer.deallocate(bytes:alignedTo:) has been removed (deprecated)
Func UnsafeMutableRawPointer.initializeMemory(as:at:count:to:) has been removed (deprecated)
Func UnsafeMutableRawPointer.initializeMemory(as:from:) has been removed (deprecated)
Func abs(_:) has been removed
Protocol Collection has generic signature change from <Self : Sequence, Self.Index : Comparable, Self.Index == Self.Indices.Element, Self.Indices : Collection, Self.Indices == Self.Indices.SubSequence, Self.SubSequence : Collection, Self.Indices.Element == Self.Indices.Index, Self.Indices.Index == Self.SubSequence.Index, Self.SubSequence.Index == Self.Indices.Indices.Element, Self.Indices.Indices.Element == Self.Indices.Indices.Index, Self.Indices.Indices.Index == Self.SubSequence.Indices.Element, Self.SubSequence.Indices.Element == Self.SubSequence.Indices.Index, Self.SubSequence.Indices.Index == Self.SubSequence.Indices.Indices.Element, Self.SubSequence.Indices.Indices.Element == Self.SubSequence.Indices.Indices.Index> to <Self : Sequence, Self.Element == Self.SubSequence.Element, Self.Index : Comparable, Self.Index == Self.Indices.Element, Self.Indices : Collection, Self.Indices == Self.Indices.SubSequence, Self.SubSequence : Collection, Self.SubSequence == Self.SubSequence.SubSequence, Self.Indices.Element == Self.Indices.Index, Self.Indices.Index == Self.SubSequence.Index, Self.SubSequence.Index == Self.Indices.Indices.Element, Self.Indices.Indices.Element == Self.Indices.Indices.Index, Self.Indices.Indices.Index == Self.SubSequence.Indices.Element, Self.SubSequence.Indices.Element == Self.SubSequence.Indices.Index, Self.SubSequence.Indices.Index == Self.SubSequence.Indices.Indices.Element, Self.SubSequence.Indices.Indices.Element == Self.SubSequence.Indices.Indices.Index>
Protocol Sequence has generic signature change from <Self.Element == Self.Iterator.Element, Self.Iterator : IteratorProtocol, Self.SubSequence : Sequence, Self.SubSequence == Self.SubSequence.SubSequence, Self.Iterator.Element == Self.SubSequence.Element, Self.SubSequence.Element == Self.SubSequence.Iterator.Element> to <Self.Element == Self.Iterator.Element, Self.Iterator : IteratorProtocol>
Protocol _SequenceWrapper has been removed
Expand Down
6 changes: 6 additions & 0 deletions test/stdlib/IntegerCompatibility.swift
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,9 @@ func sr5176(description: String = "unambiguous Int32.init(bitPattern:)") {
func sr6634(x: UnsafeBufferPointer<UInt8>) -> Int {
return x.lazy.filter { $0 > 127 || $0 == 0 }.count // should be unambiguous
}

// abs of an integer literal
func returnIntAbs() -> Int {
let x = abs(-8)
return x
}

0 comments on commit 85d488d

Please sign in to comment.