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-5996] Improve compiler error with a fix-it: Cannot assign to value, 'bar' is a 'let' constant #48553
Comments
The fix is something like "if we're going to emit this error, check to see if the thing that can't be assigned has the same name as a writable property on |
@belkadan Since today we are trying to diagnose based on the expressions, it might be tricky to figure out if destination type of the assignment expression is a property, but it's definitely doable, let's mark it as starter bug and if anything I'll help out to anybody who'd want to work on this. |
Comment by Mayur Dhaka (JIRA) @xedin I'd like to pick this up and start to working on it (I've been bitten by this multiple times). I have no experience contributing to Swift though. Any help is appreciated! |
mayurdhaka.swift (JIRA User) Absolutely! Let me know if you need any help, you can also open PR with the code you have and we'll discuss there. |
Comment by Mayur Dhaka (JIRA) @xedin Thank you! I have no clue which file I need to start looking at to fix this problem. Also, can I ask you about the minutiae via some other channel since I don't want to end up polluting this thread with no valuable information. |
mayurdhaka.swift (JIRA User) We can discuss in the PR or we can discuss here as well, either way is fine by me. |
Comment by Mayur Dhaka (JIRA) Alright, let me clone, the project and file a basic PR and we can continue there. Meanwhile, could you point me in the direction I should be looking at to fix this problem? Thanks again. |
You can take a look at how `FailureDiagnosis` works, it starts at `ConstraintSystem::diagnoseFailureForExpr`. In particular what happens in `visitAssignmentExpr`, it has to check if destination of the assignment is contextually a field of a enclosing nominal type using (DC field of the ConstraintSystem attached to FailureDiagnosis), and if so and it's mutable provide a fix it to add `self.`. |
Comment by Mayur Dhaka (JIRA) Thanks a bunch for your help. I've located the file and even the code that you were talking about. I didn't find `visitAssignmentExpr` anywhere in the file (CSDiag.cpp). Here are some further things I'd want to clarify:
Thank you once again! |
Sorry, it's actually called `visitAssignExpr` and located at https://github.com/apple/swift/blob/master/lib/Sema/CSDiag.cpp#L7049. Answers to the bullet points: 1).
It would be best if you add the test case from this issue to |
Comment by Mayur Dhaka (JIRA) I'm getting a little further into understanding how to navigate my way through the project. Here's a few more questions:
There's more I'd like to ask but I want to get the Swift project built first ( with the Swift.xcodeproj file at hand). To be clear, I should build with utils/build-script -dx right? Implying that I want the debug build (as you said) and have an Xcode project at hand? |
mayurdhaka.swift (JIRA User) > In `bool FailureDiagnosis::visitAssignExpr(AssignExpr *assignExpr)` why does the method return a `bool`? `bool`, in this case, identifies if diagnostic has been produced or not. If one the visit* methods returns true the diagnostic engine is stopped. > Line 7052 has this code `diagnose(assignExpr->getLoc(), diag::cannot_assign_to_literal);` Don't use "diag::" prefix when searching, > To be clear, I should build with utils/build-script -dx right? Implying that I want the debug build (as you said) and have an Xcode project at hand?
|
Comment by Mayur Dhaka (JIRA) @xedin thanks for the answers. I understand a bit more now where things are placed in the project (at least as they relate to the work I've to do). Coming back to the solution you provided: > it has to check if destination of the assignment is contextually a field of a enclosing nominal type using (DC field of the ConstraintSystem attached to FailureDiagnosis) What does 'a field of an enclosing nominal type' mean? And where how does 'contextually' matter (and come in the picture?) For now, I've added an error in `DiagnosticsSema.def` to the best of my ability. I've raised a PR here: #12364 |
> What does 'a field of an enclosing nominal type' mean? And where how does 'contextually' matter (and come in the picture?) Since we are looking at the expression type checker it works with "bar = bar" individually, the problem here is that "bar" used as a label which, in Swift, is transformed into "let" declaration in the body of the function e.g. `func foo(bar: Int) { ... }` is actually very simplified is like this `func foo(_ arg: (Int)) { let bar = arg.$0 }` that's why when type-checker sees "bar = bar" it thinks that this is an attempt to assign a value to immutable variable "bar". Solver itself doesn't account for enclosing scope where field "bar" is defined, but there is a way to reach it using "DC" field which represents declaration context of the expression, you can do |
Comment by Mayur Dhaka (JIRA) Thanks, that clears things up a bit for me. > func foo(bar: Int) { ... }` is actually very simplified is like this `func foo(_ arg: (Int)) { let bar = arg.$0 } So will the same check work for the case `func foo(bar: Int) { ... }` and `func foo (myBar bar: Int) { ... }` ? Since, as you say, Swift simplicity coverts `func foo(bar: Int) { ... }` to essentially `func foo(_ bar: (Int))` ? My check will run irrespective of whether the variable name and the label name are the same, as in `func foo(bar: Int) { ... }`, or if the label name and the variable name are different, as in `func foo (myBar bar: Int) { ... }` Also, I'm still not clear on how do I get the build target to run. I have my Xcode project with its massive list of targets at hand but when I build and run, there is no 'Swift 4.0(Mayur's version)' product (excuse the naive way of putting my point across) that it produces for me to run an execute to start playing around with and see which method does what (something as simple as viewing what `dumpContext()` has to offer). The documentation doesn't make any mention of how to run Swift in the project's README.md, and neither do the docs on Swift.org. Is there something I'm missing? |
> func foo (myBar bar: Int) { ... } "myBar" in this case is an API label, the actual label used in the body is going to be "bar" still, but regardless you'd only want to do the check if the left and right hand side declarations point to the same name. > Also, I'm still not clear on how do I get the build target to run. There is a ALL_BUILD target which builds whole project including stdlib, and there is "swift" target which only builds/runs compiler binary. You'd want to build everything with ALL_BUILD first and then use "swift" to build/run the compiler (you can give it .swift file as an argument, could be done via "Edit Scheme" menu item). |
Comment by Mayur Dhaka (JIRA) > There is a ALL_BUILD target [...] Yes this is what is mentioned in the README too. I found the `All_Build` scheme but I don't see any `swift` scheme. However, `swift` is a target (Apologies, I'm not able to inline the screenshots.) |
mayurdhaka.swift (JIRA User) `swift` target is what you want, that is going to build swift executable, you can edit target scheme to add parameters to `swift` executable and run from Xcode in debug mode. |
Comment by Mayur Dhaka (JIRA) I've found the `swift` target and I've been able to provide a `.swift` file as a launch argument too. Writing the code written in this bug report's description gets me the errors when I run the `swift` scheme in Xcode too–all good on those fronts.
Scratch that, I've got breakpoints to work via the regular Xcode way, using the UI. It's just that the breakpoint never actually comes to `visitAssignExpr` since (I'm assuming) the code is never called? The code I'm running in my `.swift` file is, simply: ` struct Foo {
}` |
mayurdhaka.swift (JIRA User) Apparently it doesn't go all the way to `FailureDiagnosis`, the problem is diagnosed by `ConstraintSystem::diagnoseAssignmentFailure` when constraints for assignment are generated you can put the breakpoint at the beginning of that method. |
mayurdhaka.swift (JIRA User) What helps to find a place to start with problems like this is to do a text search for a part of swift's error message that looks unchanging, like "cannot assign to value". That will lead you to a .def file that has the error definitions in it, in this case DiagnosticsSema.def. The first identifier in the ERROR() definition there is a symbol for the error "assignment_lhs_is_immutable_variable". Then you do a project text search for that, and it leads you to ConstraintSystem::diagnoseAssignmentFailure like Pavel suggested. |
Comment by Mayur Dhaka (JIRA) @gregomni Thank you so much for your help. I've been trying something similar to navigate my way around the code and I've picked up on a few things. It didn't strike me to go about finding where the error was being called. @xedin Yes I've found it thanks, I'm investigating further. |
Comment by Mayur Dhaka (JIRA) @xedin I've been working through this and here's where I'm stuck: To simulate the problem I'm working with a simple example: struct Foo { init(bar: Int) { Now, I know that the way I might want to solve this, as you mentioned previously, is to get the enclosing type (`Foo` in our case) and see if the destination (rhs `bar` in the initialiser) belongs to the enclosing type. However, in `diagnoseAssignmentFailure` (line 4345), I'm working with three parameters: `Expr *dest`, `Type destTy`, and `SourceLoc equalLoc`. None of these seem to provide any way to reach the enclosing type so I could check for `bar` being defined there. Also, doesn't it make more sense to write the fix within `computeAssignDestType`, not `diagnoseAssignmentFailure` since `diagnoseAssignmentFailure`'s job seems to start after the destination is already determined to be invalid? |
Comment by Mayur Dhaka (JIRA) @xedin Any update on the previous issue? I'd appreciate it, thanks! |
mayurdhaka.swift (JIRA User) Sorry, I'm out this week but I'll try to take a look a bit later! |
mayurdhaka.swift (JIRA User) As I was mentioning before - I think what you can do is get declaration context of the constraint system (CS->DC) and use it to acquire type of "Foo" in this case and use `lookupMember` function to lookup member with the name of "bar", if such field exists you can diagnose it. |
Comment by Mayur Dhaka (JIRA) @xedin, sorry for the intrusion, and the late reply. I agree that the solution you propose could get me closer to solve the problem but here's where I am currently: My judgment says that I should attempt to place my fix in `TypeCheckConstraints.cpp`'s `computeAssignDestType` because `diagnoseAssignmentFailure`, as the name suggests implies that there IS an error. (We want an error with a FIXIT). That means my solution should be fit into `computeAssignDestType` but I can't access `CS->DC` from that function, and simply accessing `DC->...` doesn't let me see a `lookupMember` function. I could try to create a new method in `CSDiag.cpp` and call it, the same way `diagnoseAssignmentFailure` is called but that seems like too huge a change to the codebase to me from where I stand so I wanted to swing that solution by you first so I know what you think of that. Also, an aside: the documentation for `computeAssignDestType` reads: This doesn't add up for me. If I have a line of code `foo = bar`, `foo` is the destination, and `bar` is the rvalue, right? The documentation says 'value type of the given expression, which is the destination of an assignment'? |
`lookupMember` is a method on `ConstraintSystem` itself, it accepts two arguments Type of base and DeclName of the method. `DC` is only required to figure what "base" Type is in this case, It doesn't matter where to you it really, but I'd prefer all diagnostic logic be in diagnose* methods.
What this is trying to say that `computeAssignDestType` trying to compute the type which is going to be wrapped into @lvalue by top level code. So e.g. if you have `var foo = 42;` translated to types it's `@lvalue T = LiteralInt`, so `computeAssignDestType` is responsible for trying to figure out the concrete type of `T` is or use type variable with specific constraints. |
Resetting assignee on all Starter Bugs last modified in 2018 or earlier. |
This was addressed in 8d70f98 by Greg. There's a much much better diagnostic here
Closing this out. |
Attachment: Download
Additional Detail from JIRA
md5: 2a31856bcb9731013bc2d572207dcb8b
Issue Description:
New developers are puzzled by this. Obviously, they should have written this instead:
I think it would be a better experience if this was transformed into a fix-it, where we would offer the chance to append "self." to the LHS.
The text was updated successfully, but these errors were encountered: