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-8170] IUOs should trap with a distinct error message #50702

Closed
beccadax opened this issue Jul 3, 2018 · 6 comments
Closed

[SR-8170] IUOs should trap with a distinct error message #50702

beccadax opened this issue Jul 3, 2018 · 6 comments
Assignees
Labels
affects ABI Flag: Affects ABI compiler The Swift compiler in itself improvement standard library Area: Standard library umbrella

Comments

@beccadax
Copy link
Contributor

beccadax commented Jul 3, 2018

Previous ID SR-8170
Radar None
Original Reporter @beccadax
Type Improvement
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Compiler, Standard Library
Labels Improvement, AffectsABI
Assignee @beccadax
Priority Medium

md5: 2dd2dc20cece34d956473f7aaa2869ac

Issue Description:

When we trap due to a failed force-unwrap, there are two possible causes: either the user wrote a force-unwrap explicitly, or the compiler inserted a force-unwrap for an implicitly unwrapped optional. Explicit force-unwraps are common and easy to notice; IUOs are more rare but much harder to debug (since the places you might unwrap are invisible).

We should trap with a different error messages for explicit and implicit unwraps. This would help users decide whether they should examine the visible `!` characters in their code or audit it for IUOs they didn't account for.

This may affect ABI if we implement it by adding a parameter to the _diagnoseUnexpectedNilOptional function.

@beccadax
Copy link
Contributor Author

beccadax commented Jul 3, 2018

Quick notes in five minutes of looking at the compiler:

  • ForceValueExpr already includes an isForceOfImplicitlyUnwrappedOptional() getter, but some sites in CSApply.cpp don't seem to set it when they should ❓.

  • Swift._diagnoseUnexpectedNilOptional calls are emitted by SILGenFunction::emitPreconditionOptionalHasValue().

  • The AST node is in the SILLocation, but looking there feels like a layering violation to me, so I assume that's a bad way to implement it.

  • There are two paths to that function, one for lvalues and one for rvalues, and the "is this an IUO?" bit would have to be preserved along both of them.

@belkadan
Copy link
Contributor

belkadan commented Jul 3, 2018

cc @jckarter

@jckarter
Copy link
Member

jckarter commented Jul 3, 2018

Adding an argument to _diagnoseUnexpectedNilOptional and having emitPreconditionOptionalHasValue pass the appropriate value when it forms the call sounds reasonable to me.

@beccadax
Copy link
Contributor Author

beccadax commented Jul 4, 2018

@jckarter: Is figuring out whether a given use is an implicit force unwrap by looking at the node in the SILLocation a decent implementation or an ugly hack? I could add it as a parameter to SILGenFunction::emitCheckedGetOptionalValueFrom(), but some of its callers look like very general conversion code that would have a hard time knowing what to pass. (Although maybe a force-unwrap would never pass through those parts of SILGen...hmm...)

@jckarter
Copy link
Member

jckarter commented Jul 5, 2018

Adding it as an argument to `emitCheckedGetOptionalValueFrom` makes more sense to me. Everywhere we call it in SILGen, we ought to be able to walk back to an AST formation that's either working on an explicit or implicit unwrapping. If not, it's probably reasonable to default to the "explicit" case.

@beccadax
Copy link
Contributor Author

beccadax commented Mar 9, 2019

Fixed in #17826

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects ABI Flag: Affects ABI compiler The Swift compiler in itself improvement standard library Area: Standard library umbrella
Projects
None yet
Development

No branches or pull requests

3 participants