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
Don't autocomplete symbols that have been renamed/removed #917
Comments
Thank you for reporting! This is a known limitation since we currently rely on the classfiles from disk to provide the state of public signatures in the workspace. I see two possible approaches that could work to address this issue:
I personally believe the first approach is better since it works nicely across different projects. |
Hi @olafurpg Is it correct to assume that the issue is caused by Bloop not updating the class files until everything compiles? If that's the case, is there any way to have Bloop update class files for source files that compile without errors so the class files are in sync with the sources (i.e., all sources that compile without errors)? If yes, I assume Metals will start suggesting the correct names. This bug is particularly annoying when doing a refactoring where a function needs to change signature (e.g., a new parameter) and you want to check the all the calls to that function to make sure you insert the correct value for the new parameter. So you change the function definition and then let the compiler flag all errors and fix them one-by-one. In that case Metals will suggest the old signature until all errors are fixed. |
Bloop might be copying the compiled artifacts only after compiling the whole build target. It should be possible to be changed, but not exactly sure about the consequences. This might go even deeper to Zinc, which is used underneath. I think that going the way explained by @olafurpg might be better. |
There is a related PR here: https://github.com/scalacenter/bloop/pull/1348/files where the old artifacts are copied for any failed compilation, so we can always have something working when the workspace is started again. |
After thinking about it for a bit let's say we have files A, B, C, D that depend on each other: A <- B, C <- D. If we change something in A that causes B and C not to compile we could create classfile for A, but we will not have one for B, C and D. I think we should instead try to fix it in the presentation compiler itself. I plan to experiment on it a bit while working on a better support for Scala 3. |
This is a hard problem that has always been a known limitation (in my mind) from the start, but it might not be well documented since it’s quite complicated to explain without going into implementation details. I agree the current behavior is annoying when refactoring. I think the best solution would be to extend Metals to build outlines with the presentation compiler for files that are off-sync with the classfiles on disk. It should be possible to layer the classpath so the presenter compiler doesn’t hit the stale signatures from the build. The tricky bit is to know what files to outline and (optionally) come up with an efficient and robust way to cache the outlines so that restarting the Metals server in the middle of a refactoring session doesn’t break any functionality |
Yes, we could have the previous versions of the class files for B, C and D. Since B and C has not, changed the signatures from the old class files generated by B and C in the previous compilation must be correct.
If possible, Bloop could copy the class files for source files that compile without error and not overwrite class files originating from source files that no longer compile without error. In this example A was changed and recompiled without error, so copy the classes generated from the compilation of A. Because B and C depends on A, they were also recompiled but failed compilation, so keep the classes they generated in the previous compilation. I do not know whether there is a map telling which classes were generated by which source files – but I guess so because how can Bloop / Zinc otherwise tell which source files to recompile when something changes?
Why is that a problem? The inconsistent set of class files (where inconsistent means coming from different compilations) could be considered consistent with the source files.
Sounds interesting. |
A simple workaround is to add the new signature / new function without removing the old and save the file. Then everything still compiles and we have a consistent set of class files. Now, remove the old signature and Metals will complete the new signature when fixing errors in the dependent source file.
You know that much better than me it just occurred to me that it would be simpler to do what I suggested in the previous comment but there are most probably some cases I have overlooked and/or not covered by the simple example. |
There is a lot of mention of state in Bloop, which might not go well with the approach you mentioned and I am really not comfortable changing that in Bloop right now as I don't have enough of an experience with the project. I feel like fixing it in Metals itself is a much more attainable goal. |
Just hit on this problem one more time and did some investigations. It seems that one part of this issue comes from PC and compiler side. Example, with completions:
Notice about PC restarts. The only solution for this issue that I currently see is that we have to invalidate/remove symbols after every interaction with PC. |
This might result in a significant performance degradation for a pretty modest UX improvement. I believe a better solution is to use -Youtline to build an up-to-date classpath even for erroneous code as mentioned in #917 (comment) |
Describe the bug
Metals PC shows references to symbols that were renamed.
To Reproduce
Steps to reproduce the behavior:
Foo.scala
definingFoo
Foo
toFoo2
UseSite.scala
and try to completeFoo
.Foo
will be completed while it shouldn't.Expected behavior
Metals PC should detect when symbols have been renamed and remove them from completions. A way to do this would be to modify the PC to check that the class file associated with a symbol does indeed exist. If it doesn't exist, it means it was removed and should be filtered out.
Screenshots
Installation:
Additional context
Search terms
rename,symbol,pc,invalid
The text was updated successfully, but these errors were encountered: