Don't autocomplete symbols that have been renamed/removed

[jvican]

Describe the bug

Metals PC shows references to symbols that were renamed.

To Reproduce Steps to reproduce the behavior:

1. Go to a file Foo.scala defining Foo
2. Rename Foo to Foo2
3. Go to file UseSite.scala and try to complete Foo.
4. 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:

• Operating system: macOS/Windows/Linux
• Editor: Visual Studio Code/Atom/Vim/Sublime/Emacs
• Metals version: v0.7.5

Additional context

Search terms rename,symbol,pc,invalid

[olafurpg]

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:

• keep up-to-date symbol table in-memory that we feed to the presentation compiler, this could maybe be done with the new -Youtline compiler option in 2.12.10
• use "sourcepath" in the presentation compiler to let the compiler find the signatures itself. I'm not too familiar with how this could work, however.

I personally believe the first approach is better since it works nicely across different projects.

[martingd]

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.

[tgodzik]

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.

[tgodzik]

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.

[tgodzik]

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. Essentially D could be a lot of files that could use completions from B, C, but in this case it will not be able to do so. And the scenario might not be as obvious as this, so it's really impossible for Bloop to decide what to copy and what to not copy. And we cannot mix different compilations, as that will create an inconsistent of the workspace for the presentation compiler. So current behavior where we just copy last successful compilation is the best we can do.

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.

[olafurpg]

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

[martingd]

@tgodzik

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.

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.

Essentially D could be a lot of files that could use completions from B, C, but in this case it will not be able to do so. And the scenario might not be as obvious as this, so it's really impossible for Bloop to decide what to copy and what to not copy.

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?

And we cannot mix different compilations, as that will create an inconsistent of the workspace for the presentation compiler.

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.

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.

Sounds interesting.

[martingd]

@olafurpg

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.

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.

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

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.

[tgodzik]

@martingd

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.

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.

[dos65]

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. The problem is that whenever we do anything using PC we pass a source file and do a compile. Compiler enters symbols from source into symbolTable and keeps them until it will be restarted.

Example, with completions:

• first phase - enter all symbol and do complete

  package example
  
  object Main {
    def foo: Int = 42
  }
  
  object Second {
     val z = Mai@@ // returns `Main` that's correct
  }
  

• second - remove object Main and try to complete one more time

  package example
  
  object Second {
     val z = Mai@@ // returns `Main` that's incorrect
  }
  

Notice about PC restarts. To reproduce the thing above you have to keep file unsaved. Otherwise, if file will be saved after removing Main it will trigger compilation and presentation compiler will be restarted and there will be no example.Main

The only solution for this issue that I currently see is that we have to invalidate/remove symbols after every interaction with PC.

[olafurpg]

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 https://github.com/scalameta/metals/issues/917#issuecomment-729685029