Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SR-14729] Inaccurate diagnostic "Left side of nil coalescing operator '??' has non-optional type 'T?', so the right side is never used" #57079

Open
swift-ci opened this issue Jun 5, 2021 · 12 comments

Comments

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Jun 5, 2021

Previous ID SR-14729
Radar None
Original Reporter jackmarch (JIRA User)
Type Bug
Environment

macOS 11.4, Xcode 12.5, Swift 5.4. Haven't tested on older versions.

Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug, StarterBug, TypeChecker
Assignee jackmarch (JIRA)
Priority Medium

md5: 23f5a2f7514c2acf9995c86de2d0fbba

Issue Description:

Diagnostic claims that T? is a non optional type, even though it is optional. Contrived example:

func foo<T>(
    optionalT: T?,
    secondOptionalT: T?
) -> T?? {
    optionalT ?? secondOptionalT // Warning: Left side of nil coalescing operator '??' has non-optional type 'T?', so the right side is never used
}

My colleague ran into this issue when using a dictionary, as the subscript return is optional and they were using an optional Value. While the fix is to simply use a required Value (i.e. change below from [String:T?] to [String:T]) given writes/reads are optional anyway, the diagnostic is pretty confusing:

func practicalExample<T>(
    dict: inout [String:T?],
    optionalT: T?,
    secondOptionalT: T?
) {
    dict["foo"] = optionalT ?? secondOptionalT // Warning: Left side of nil coalescing operator '??' has non-optional type 'T?', so the right side is never used
}

Potential fix is change diagnostic to:

Warning: Left side of nil coalescing operator '??' has type 'T?', the non-optional variant of 'T??', so the right side is never used

There's probably a better wording than that though?

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Jun 7, 2021

I don't think "non-optional variant" is correct terminology. How about something like:

warning: left side of nil coalescing operator '??' is implicitly converted from type 'T?' to 'T??', so the right side is never used

You can change the diagnostic in DiagnosticsSema.def (tip: diagnostic names show up if you pass -Xfrontend -debug-diagnostic-names to the compiler) and fix any related test cases.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Jun 11, 2021

Comment by Jack March (JIRA)

thanks theindigamer (JIRA User) for the tips and the suggestion! One thing I would note though is how changing the diagnostic to that would behave for this function:

func foo<T>(
    t: T,
    t2: T
) -> T? {
    t ?? t2
}

Current diagnostic:

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

New diagnostic:

warning: left side of nil coalescing operator '??' is implicitly converted from type 'T' to 'T?', so the right side is never used

I think the former is easier to understand because it explicitly points out we're using the nil-coalescing operator for a non-optional type.

As an alternative, does this have correct terminology?

func foo<T>(
    optionalT: T?,
    secondOptionalT: T?
) -> T?? {
    optionalT ?? secondOptionalT // Warning: Left side of nil coalescing operator '??' has type 'T?' (non-optional T??), so the right side is never used
}

func foo<T>(
    t: T,
    t2: T
) -> T? {
    t ?? t2 // Warning: Left side of nil coalescing operator '??' has type 'T' (non-optional 'T?'), so the right side is never used
}

@LucianoPAlmeida
Copy link
Collaborator

@LucianoPAlmeida LucianoPAlmeida commented Jun 13, 2021

I wonder if this is the expected behavior to implicit wrap it in an implicit inject_into_optional in this case? This is just a question of curiosity, because to me for example:

func practicalExample<T>(dict: inout [String:T?],
                         optionalT: T?,
                         secondOptionalT: T?
) {
  // 1.
  dict["foo"] = optionalT ?? secondOptionalT // Warning: Left side of nil coalescing operator '??' has non-optional type 'T?', so the right side is never used
  let _: T?? = optionalT ?? secondOptionalT // Warning: Left side of nil coalescing operator '??' has non-optional type 'T?', so the right side is never used
  
  // 2.
  let a = optionalT ?? secondOptionalT 
  dict["bar"] = a
  let _: T?? = a 
}

1 and 2 are in behavior(intention of user) expected to be the same, but because of the implicit injection on optional to optional the behavior is ends up being a bit unexpected and surprising from user perspective... I'm not familiar with the details and why's of it but I wonder if the implicit injection is necessary in this case?

cc @xedin CodaFi (JIRA User)

@xedin
Copy link
Member

@xedin xedin commented Jun 14, 2021

Unfortunately it's just a consequence of argument application, since `??` has signature `<T>(_: T?, _: @autoclosure () throws -> T) -> T` solver ends up matching `T?` (as an argument) to `$T_param?` which creates this implicit injection for the first argument.

@LucianoPAlmeida
Copy link
Collaborator

@LucianoPAlmeida LucianoPAlmeida commented Jun 15, 2021

Hum, yeah ... this makes sense, but given both first and second arguments are `T?` I wonder if would make sense to pick the optional overload

public func ?? <T>(optional: T?, defaultValue: @autoclosure () throws -> T?)
of `??` which has signature `<U>(_: U?, _: @autoclosure () throws -> U?) -> U?`? If I understand correctly, If it pick this optional one the implicit conversion wouldn't be necessary right @xedin?

@xedin
Copy link
Member

@xedin xedin commented Jun 16, 2021

That would make sense to me, I wonder what would it take to change optional promotion behavior when it comes to type variables, if that's easy to do we should try to run source compat suite and see if it breaks anything. With that adjustment constraints like `$T? conv Int?` would result in `$T` bound to `Int` without any additional promotions.

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Aug 25, 2021

@xedin @LucianoPAlmeida how about we improve the diagnostic here, and split the work of investigating the type-checking change in a separate JIRA? It seems to me that the diagnostic change has negligible risk and is useful, so it makes sense to land that first. The type-checking change is potentially riskier (even if the source compat suite passes, some other code could fail), so splitting out that into a separate JIRA unblocks work on this.

@LucianoPAlmeida
Copy link
Collaborator

@LucianoPAlmeida LucianoPAlmeida commented Aug 26, 2021

I think this would make sense theindigamer (JIRA User), and also just improve the diagnostic wording in those special cases seems like a good starter task.
What do you think @xedin?

@xedin
Copy link
Member

@xedin xedin commented Aug 26, 2021

Sounds good to me if that would be easy to detect.

@LucianoPAlmeida
Copy link
Collaborator

@LucianoPAlmeida LucianoPAlmeida commented Aug 26, 2021

As far as I understand by a quick look to just improve the wording on diagnostic the change would be here

Ctx.Diags.diagnose(DRE->getLoc(), diag::use_of_qq_on_non_optional_value,
detect based on optionality of expression types and in this case provide a better message. Does it make sense?

@xedin
Copy link
Member

@xedin xedin commented Aug 27, 2021

Yeah, that does make sense to me, it definitely makes it easier to check on the type-checked AST.

@LucianoPAlmeida
Copy link
Collaborator

@LucianoPAlmeida LucianoPAlmeida commented Aug 28, 2021

Right, so let's make it a starter bug, thanks for the input @xedin

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants