Feedback requested: improving inneffective nil coalescing diagnostic feedback

Development Compiler
1

Motivated by this bug report and corresponding PR, I was thinking a bit about how to improve developer feedback for what's going on in this scenario:

func deadNilCoalesce<T>(_ value: T?, fallback: T) {
    let _ = (value ?? fallback).flatMap { $0 }
                   `- warning: left side of nil coalescing operator '??' has non-optional type 'T?', so the right side is never used
}

Obviously the wording "...has non-optional type 'T?'" is self-contradictory, hence the bug report. But, the diagnostic is still "right" that the RHS of the ?? operator is dead. The reason is that the LHS gets injected into an optional (as the .some case of a T??), so it can't be nil. Without getting into why the typechecker ends up with this particular solution, I was wondering how we might improve developer feedback in this situation.

The current diagnostic wording mentions only types and not values. This generally makes sense in the cases where the LHS type actually is not optional:

let x: Int
let _ = x ?? 42
// warning: left side of nil coalescing operator '??' has non-optional type 'Int', so the right side is never used

But, IIUC, even in this case the reality is that the LHS value is implicitly wrapped in an optional. Indeed, the left side of the ?? must be optional, since that's how the operator is defined, and its entire purpose is to provide a fallback value for a nil optional.

I can imagine rewording this diagnostic so that it no longer hides the fact that there's an implicit optional injection occurring, which would generalize to the case in the bug report:

let _ = (value ?? fallback).flatMap { $0 }
               `- warning: left side of nil coalescing operator '??' is implicitly wrapped in an optional, so the right side is never used

And if we wanted to elaborate more on the types involved or whatever, we could add a note:

let _ = (value ?? fallback).flatMap { $0 }
         |     `- warning: left side of nil coalescing operator '??' is implicitly wrapped in an optional, so the right side is never used
         ` note: value of type 'T?' is implicitly wrapped in an optional here, producing 'T??'

But I'm wondering if explicitly discussing the optional injection/wrapping is actually helpful or desirable since it's sort of an implementation detail.

So, overall, looking for feedback from people about how this diagnostic could be improved. What sort of wording would you expect to see here? Should the implicit optional wrapping mechanism be unconditionally mentioned or should we special-case the double-optional scenario with some different feedback? What is the mental model we want to foster with this diagnostic?

4 Likes
2

I think the current diagnostic is better when the type truly isn't optional. When it is, we can fall back to something more complicated…but I kind of think a missing piece is "but why did it do that when the types otherwise line up?" and the answer would be "some other part of the expression expects an optional". I don't know if it would be easy to pick that out, though (in this case it's not even a constraint, exactly; it's the use of a method that only exists on Optional).

2 Likes
3

Thanks for the feedback – I was inclined to think maybe unifying everything would be good, but if the multiply-optional case is a relative outlier then I can see the appeal of not bringing up how the compiler implicitly did something that doesn't show up in the source code.

Tangentially, I'm curious if the "why" here is clear to you (or anyone). I've looked at the -debug-constraints output a bit and did not find it particularly illuminating (not that I really know how to interpret it that well).

4

Wouldn’t the ?? be due to the use of flatMap, which causes the () expression to be promoted from ? to ?? since that’s what flatMap requires?

5

Agree: The mental model we want to foster is an explanation for why the code required an extra level of optionality in order to compile at all.

In this particular example it’s not just the ?? that isn’t doing what the user probably expects, it’s the flatMap { $0 } as well. Diagnosing only the first bit is always going to be a bit misleading.

6

flatMap shouldn’t exist. there, i said it.

i believe it to be useless in the general case, which implies that i also believe it to be useless here. the user’s code really should just be

func deadNilCoalesce<T>(_ value: T?, fallback: T) {
    let _ = value ?? fallback
}

that’s what the developer almost certainly wants, and any chain of educational notes that doesn’t hammer this point home is ultimately doing a disservice to the beginner.

now it might feel good and intellectually stimulating as experts to walk through why the type checker ended up in this situation. if i were really pressed to provide an explanation, i would say something like the following:

  1. the flatMap closure is typed T -> T?, so if you assume that $0 is T, then the thing in parentheses (value ?? fallback) must be T?.
  2. the type checker sees that value is already a T?, so it realizes that fallback will never be called.
  3. the only way that it knows how to describe this issue is by saying that T? isn’t optional, which makes perfect sense if we simply suppose that U = T? and that U does not need to be coalesced with anything to obtain U.

the idea that double optionals (T??, or U?) are involved is misleading - it’s the kind of explanation that sounds like a smart thing to say but is wrong and ultimately leads the developer even further astray. in fact, hallucination of double optionals being present when none actually are is probably what led the developer to wrongly add flatMap to the code in the first place.

but we really shouldn’t be cracking open the rule book to begin with, because none of this would have happened if flatMap hadn’t gotten involved and the user had just written value ?? fallback.

what we have is a Bad API living in the standard library, an API that is leading the user astray, and sending them into a maddening spiral of trying to parse through interactions between increasingly arcane type checking rules. Swift experts are tempted to exhaustively explain all these interactions to the beginner, and might justify it under the pretense of “facilitating understanding of the type checker”, but that’s just distracting from the root cause of the problem, which is that we have Bad APIs in the standard library, APIs that shouldn’t exist and really ought to be deprecated.

i’m quite surprised that flatMap has lived for as long as it has while being as useless (and as we have seen, actively harmful) as it is. its relative, Sequence.forEach, gets far more hate despite, in my opinion, being much more useful than flatMap.

now if i haven’t made my point clear yet, i don’t think there needs to be any changes to the compiler diagnostics at all. that’s just the wrong problem to solve here. what we have here are rotten standard library APIs and instead of figuring out how to take them off the shelves, we’re instead occupying ourselves with discussions on how to best treat food poisoning.

1 Like
7

Lot of opinions in here already, so why not one more?

(value ?? fallback) should only be able to typecheck as T. Automatic promotion of values to Optional is arguably a mistake in general, but it's certainly a mistake to go looking inside ?? inside () inside a method receiver and picking a random value to promote to Optional.

It makes me wonder, how much time in general does the compiler spend wondering if random values should actually be Optional, when it's struggling to typecheck something?

Should implicit promotion to optional apply to any situation other than the full right-hand-side of an assignment, or the full value of a method argument?

2 Likes
8

That is equivalent to saying that the line shouldn’t compile at all, since an arbitrary T could never have a flatMap method.

This is certainly a position one can take, but it doesn’t help inform the question posed: given that this code does compile with a warning, what should the warning say?

9

Yes, exactly.

10
With this test harness that mimics `??` and `flatMap` 
infix operator ??? : NilCoalescingPrecedence

// Edit: added this overload
@_transparent @_alwaysEmitIntoClient public func ??? <T: ~Copyable>(
    optional: consuming T?,
    defaultValue: @autoclosure () throws -> T // FIXME: typed throws
) rethrows -> T {
    switch consume optional {
        case .some(let value): value
        case .none: try defaultValue()
    }
}

@_transparent @_alwaysEmitIntoClient public func ??? <T: ~Copyable>(
    optional: consuming T?,
    defaultValue: @autoclosure () throws -> T?
) rethrows -> T? {
    switch consume optional {
        case .some(let value): value
        case .none: try defaultValue()
    }
}

extension Optional {
    @_alwaysEmitIntoClient public func myFlatMap<E: Error, U: ~Copyable>(
        _ transform: (Wrapped) throws(E) -> U?
    ) throws(E) -> U? {
        switch self {
            case .some(let y): try transform(y)
            case .none: .none
        }
    }
}

let optional: Int? = 1

let a = (optional ?? 10).flatMap { _ in 0 }    // 🔶
let b = (optional ?? 10).myFlatMap { _ in 0 }  // 🔶
let c = (optional ??? 10).flatMap { _ in 0 }   // âś…
let d = (optional ??? 10).myFlatMap { _ in 0 } // âś…

I am getting the warnings for those:

let optional: Int? = 1

let a = (optional ?? 10).flatMap { _ in 0 }    // 🔶
let b = (optional ?? 10).myFlatMap { _ in 0 }  // 🔶

but not for these:

let c = (optional ??? 10).flatMap { _ in 0 }   // âś…
let d = (optional ??? 10).myFlatMap { _ in 0 } // âś…

Which suggests there's some difference between my custom ??? operator and real ?? operator.. but what exactly is that difference and could ?? be modified to not have such behaviour?

11

One difference is that there are two overloads of func ?? in the stdlib:

  • the one taking a T? and an @autoclosure () -> T, returning T;
  • and the other taking a T? and an @autoclosure () -> T?, returning T?.

I think the latter one was added in the early days of Swift due to Optional promotion causing weird corner cases when the types at the call site didn't match exactly. (I also think the language would've been better with less Optional promotion, but that hardly going to change.)

(Edit: This wasn't the reason for the warning; see my other message below.)

3 Likes
12

The difference is that there are two overloads of func ?? in the stdlib

Thank you, but even with the added second overload (updated the test harness above) I still can not reproduce the warning with my ??? operator..

13

Ah, of course. We're talking about an auxiliary diagnostic (from Sema; see definition of use_of_qq_on_non_optional_value) that's specifically added for the standard library operator ??. That's why you won't see it for your own one.

2 Likes
14

It would be, but only half of the features necessary to make it not useless, but redundant, exist.

Those being:

  1. Optional chaining. :white_check_mark:
  2. A map function which operates on 1-tuples. :cross_mark:

For example, consider the body of this function:

extension Optional {
  /// Transform `.some` into `.none`, if a condition fails.
  /// - Parameters:
  ///   - isSome: The condition that will result in `nil`, when evaluated to `false`.
  func filter<Error>(
    _ isSome: (Wrapped) throws(Error) -> Bool
  ) throws(Error) -> Self 
}

Literally, nobody ever needs a mapping function, so it could be this:

if let self, try isSome(self) { self }
else { nil }

But going with the idea that any form map is useful in general, flatMap is required:

try flatMap { wrapped throws(Error) in try isSome(wrapped) ? wrapped : nil }

(which should be the following but typed throws are half-baked:)

try flatMap { try isSome($0) ? $0 : nil }

because this is missing:

try self?&.map { wrapped throws(Error) in try isSome(wrapped) ? wrapped : nil }
&.map 
public struct And<Value> {
  @usableFromInline let value: Value
  @inlinable init(_ value: Value) { self.value = value }
}

postfix operator &
@inlinable public postfix func &<Value>(value: Value) -> And<Value> {
  .init(value)
}

public extension And {
  @inlinable func map<Transformed, Error>(
    _ transform: (Value) throws(Error) -> Transformed
  ) throws(Error) -> Transformed {
    try transform(value)
  }
}

Maybe you were misremembering which is useless? I don't think map does anything that flatMap doesn't do, because of Optional promotion.

15

Good to know, thank you again.

Note that flatMap is not particular relevant to this issue as I am getting this same warning from:

let optional: Int? = 1
extension Optional { func foo() {} }
(optional ?? 10).foo()

or:

_ = (optional ?? 10) != .some(1)
1 Like
17

I wonder if this (a nested Optional) was really what happened? The following example has two print calls, one in flatMap closure, another at the end. If flapMap (see its code here) is called on .some(nil), the wrapped nil value would be passed to closure and got printed. However, my exeperiment showed that this didn't happen. The only output was generated by the print call at the end.

Test 1:

func deadNilCoalesce<T>(_ value: T?, fallback: T) -> T? {
    (value ?? fallback).flatMap {
        print($0)
        return $0 
    }
}

let value: Int? = nil
let result = deadNilCoalesce(value, fallback: 1)
print(result) 

// Output: 
// nil

Even if there is really Optional promotion invovled in the original example, I still find it's hard to understand why RHS of ?? is ignored. See the third line in the following code. IIUC it invovles Optional promotion. However, the RHS of ?? is executed, otherwise c would be .some(nil), not .some(.some(2)).

Test 2:

let a: Int? = nil
let b: Int? = a ?? 1 // b is .some(1)
let c: Int?? = a ?? 2  // c is .some(.some(2))
let d: Int?? = b ?? 3  // d is .some(.some(1))

// print(b)
// print(c)
// print(d)

I also found another weird behavior. If I replace ?? operator with @tera's implementation of ???, the output changes, which indicates the RHS side is executed! Given that @tera's code was copied from stdlib, I wonder what caused the difference?

Test 3:

  func deadNilCoalesce<T>(_ value: T?, fallback: T) -> T? {
-     (value ?? fallback).flatMap {
+     (value ??? fallback).flatMap {
          print($0)
          return $0 
      }
  }

// Output:
// 1
// Optional(1)

I have a different opinion on this. The original code makes sense to me. IMO the requirements were:

  • step 1: If input value is nil, use default value
  • step 2: Transform the result of step 2 using flatMap. The result might be a different Optional value.

Your implementation missed the transform part and was effectively just a wrapper of ?? operator.

In my understanding flatMap is equivalent to bind method of Monad. It's designed for a fixed pattern, so it's not as flexible as Sequence.forEach. But it has its own advantages (e.g. chaining).

1 Like
18

Houston we have a problem.. A bigger problem than just an incorrect warning. The following snippet:

extension Optional {
    func foo() { print("result is", self) }
}

func optional() -> Int? {
    print("lhs is used")
    return nil
}

func rhs() -> Int {
    print("rhs is used")
    return 1
}

(optional() ?? rhs()).foo()

Gives this result on Swift 6.2 (tested on intel target of godbolt):

lhs is used
rhs is used
result is Optional(1)

while on later Swift versions (e.g. Swift 6.3) it gives:

lhs is used
result is nil

Speaking of godbolt, its ARM targets do not feel quite alright at the moment, at least IRT that snippet of code.

FWIW, with a custom ??? operator above (which was indeed copied from some copy of standard library) the results are correct.

Edit: godbolt link

Edit 2: added a test for

(optional() ?? rhs()) != .some(1)

Same difference between old and new Swift versions.

In case the link before doesn't work, here's a new one.

2 Likes
19

(Edit: I was using godbolt wrong, see my next comment)

Hmm, I see the latter output on all Swift versions, including 6.2. (Well, I didn’t try all the versions on godbolt, but every version that I did try showed the same result.

Based on the diagnostic message:

warning: left side of nil coalescing operator '??' has non-optional type 'Int?', so the right side is never used

It seems that the compiler is optional-promoting the arguments of ??, so that the lhs becomes Int??.some(.none) and the rhs becomes Int?.some(1), rather than simply promoting the result of ?? to Int?.

I do find this behavior surprising, but since it seems to have been consistent through the history of Swift I don’t expect it to change.

• • •

Also, I tried searching the Swift repo for “left side of nil coalescing operator” and also for “has non-optional type”, but all the results I saw were tests that expected to see that warning. I was unable to locate the source which actually produces the warning.

20

FYI @pyrtsa shared the source that produces the diagnostics above.

1 Like
21

Oh wait now I see what you describe.

I think I hadn’t realized that the compiler version on godbolt can be chosen independently for the assembly and for the execution, and I was only changing the version for the assembly.

My mistake.

Yes, I think you’ve found a bug, did you open a bug report?