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
Comments
Looks like |
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. |
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 🙂. |
xcadaverx (JIRA User) You can find them in `test/Constraints/fixes.swift` |
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? 🙂 |
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`. |
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!! |
No problem! |
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 |
xcadaverx (JIRA User) Only committers can trigger CI unfortunately, but I'll help you out with that after review. |
Comment by Daniel Williams (JIRA) PR merged: #22802 |
Environment
Swift 4.2
Xcode 10.1
Additional Detail from JIRA
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'"
The fixit that says "Coalesce using '??' to provide a default when the optional value contains 'nil'" generates this:
This code prompts the same error. The appropriate correction would be:
The text was updated successfully, but these errors were encountered: