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-9851] Incorrect fixit for logical NOT operator on value of Bool? type  #52262

Closed
swift-ci opened this issue Feb 3, 2019 · 11 comments
Closed
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself diagnostics QoI Bug: Diagnostics Quality of Implementation good first issue Good for newcomers

Comments

@swift-ci
Copy link
Collaborator

swift-ci commented Feb 3, 2019

Previous ID SR-9851
Radar None
Original Reporter thompsonate (JIRA User)
Type Bug
Status Resolved
Resolution Done
Environment

Swift 4.2

Xcode 10.1

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

md5: 03fbd7492e84467054bb26de58f6b441

Issue Description:

The following code prompts an error that states "Value of optional type 'Bool?' must be unwrapped to a value of type 'Bool'"

let optionalBool: Bool? = false
if !optionalBool { }

The fixit that says "Coalesce using '??' to provide a default when the optional value contains 'nil'" generates this:

let optionalBool: Bool? = false
if !optionalBool ?? <#default value#> { }

This code prompts the same error. The appropriate correction would be:

let optionalBool: Bool? = false
if !(optionalBool ?? <#default value#>) { }
@belkadan
Copy link
Contributor

belkadan commented Feb 4, 2019

Looks like offerDefaultValueUnwrapFixit in doesn't check the context when deciding if the resulting expression needs parens; the fix would be to pass down an appropriate rootExpr to use with exprNeedsParensAfterAddingNilCoalescing. @xedin, where would a StarterBug contributor get that?

@xedin
Copy link
Member

xedin commented Feb 6, 2019

FailureDiagnostic has `getRawAnchor`, `getAnchor` (which pinpoints expression based on the locator path) and `getParentExpr` which returns the root expression, it seems like `diagnoseUnwrap` would need an additional parameter to pass that in.

@swift-ci
Copy link
Collaborator Author

Comment by Daniel Williams (JIRA)

@belkadan @xedin - Pardon my ignorance, but this is my first attempt at a bug on here. Are there already tests around `offerDefaultValueUnwrapFixit` that I could use as an example? I've not been able to find any tests related to `CSDiagnostics.cpp`, but I'm almost certainly looking in the wrong places 🙂.

@theblixguy
Copy link
Collaborator

xcadaverx (JIRA User) You can find them in `test/Constraints/fixes.swift`

@swift-ci
Copy link
Collaborator Author

Comment by Daniel Williams (JIRA)

Hoping someone can chime in here, as I'm having a bit of trouble. I'm compiling a file with the first block of code in the description of this ticket.

I have a test written up that is passing when I force `needsParensOutside` to `true` in the `offerDefaultValueUnwrapFixit` method. However, when passing a root expression acquired from FailureDiagnostic's `getParentExpr()`, the `exprNeedsParensAfterAddingNilCoalescing` method ends up returning false. If I dump the contents of the rootExpr in lldb, it looks like this:

(lldb) e rootExpr->dump() (prefix_unary_expr type='<null>' (declref_expr type='<null>' decl=Swift.(file).Bool extension.! function_ref=unapplied) (paren_expr implicit type='<null>' (declref_expr type='<null>' decl=hello.(file).optionalBool@./hello.swift:1:5 function_ref=unapplied)))

I see that this is an implicit `paren_expr`, which makes `exprNeedsParensOutsideFollowingOperator` return false because of this logic, since `isa<ParentExpr>(parent)` returns true:

 if (!parent || isa<TupleExpr>(parent) || isa<ParenExpr>(parent)) { return false; }

`e parent->dump()` shows:

(lldb) e parent->dump() (paren_expr implicit type='<null>' (declref_expr type='<null>' decl=hello.(file).optionalBool@./hello.swift:1:5 function_ref=unapplied))

Can someone give me a quick briefing on the implicit paren_expr? 🙂

@xedin
Copy link
Member

xedin commented Feb 20, 2019

xcadaverx (JIRA User) All of the operators form unified application representation in AST, where some function is applied to arguments e.g. binary operator `1 + 2` would be represented as `+(1, 2)` same for function calls and other operators like prefix, if there is a single argument it's going to be a pair of parens e.g. `(1)` if there are more than one argument it's going to be a tuple `(1, 2)`, added implicitly my compiler. The example from description AST looks like this `!(optionalBool)`, it seems like `exprNeedsParensOutsideFollowingOperator` might need to check is paren or tuple expressions are "implicit" before returning `false`.

@swift-ci
Copy link
Collaborator Author

Comment by Daniel Williams (JIRA)

@xedin - Totally makes sense now. I now see that `ParenExpr` is a subclass of `IdentityExpr`, which has isImplicit(). That does indeed work and make my test pass. Time to run the rest of the tests. Thanks so much!!

@xedin
Copy link
Member

xedin commented Feb 20, 2019

No problem!

@swift-ci
Copy link
Collaborator Author

Comment by Daniel Williams (JIRA)

@xedin @belkadan - I've opened a pull request for this ticket here: #22802

I read the `Contributing.md` since the PR said that I need to run the CI tests, but I couldn't find a way to run the CI tests without commit access. I did run the tests locally, though. I welcome any comments, as C++ is not my forte, and I'm really new to the swift compiler in general.

Cheers

@xedin
Copy link
Member

xedin commented Feb 22, 2019

xcadaverx (JIRA User) Only committers can trigger CI unfortunately, but I'll help you out with that after review.

@swift-ci
Copy link
Collaborator Author

Comment by Daniel Williams (JIRA)

PR merged: #22802

@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
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself diagnostics QoI Bug: Diagnostics Quality of Implementation good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants