Rename refactoring in linked editing mode produces unexpected results

[travkin79]

I'm facing some issues when using the new in-line rename feature (in linked editing mode) from PR #1440. I appreciate the new feature very much, but there are cases where it does not work as expected.

Unexpected behavior that I observed so far (still testing):

1. Undo operations are in some cases split into very small steps (one letter at a time and each occurrence separately handled)
   • Take a method (e.g. in C/C++ editor from CDT LSP) that is used multiple times in the same file and press Alt + Shift + R to start the rename operation
   • Type a few letters, pause for 1 or 2 seconds, repeat twice
   • Try undoing the rename operation
   • Instead of undoing the three edits in three steps, the changes are undone letter by letter, while undoing the rename operation for each method name occurrence separately instead of changing all occurrences at once.
2. Selection doesn't fit to what was typed
   • In some cases, when renaming a method in multiple steps (similar to the case above), the caret jumps a few characters back and the rename selection changes to a substring of the new method name that I just entered. I did not yet find a way to reliable reproduce that.
3. Selection doesn't fit and undo makes wrong text edits
   • In some cases, renaming a method and then undoing the changes results in wrong text edits. At least, it seems, after undoing all changes, we get the original text version. Only intermediate results are corrupt.
4. Renaming a method and undoing it sends plenty textDocument/DocumentLink requests to the LS. That looks suspicious to me.

Video for case 1:

Undo-letter-by-letter.mp4

Video for case 3:

Undo-not-working-correctly.mp4

Video for case 4:

numerous-requests.mp4

It looks to me as if there is some concurrency issue in org.eclipse.lsp4e.operations.rename.LSPInlineRenameLinkedMode and related classes.

I've seen there is a preference for switching inline renaming off (what could be quick work-around to avoid the issues). Is there a reason why this switch is not presented in the UI (or did I just miss it)?

@sebthom, could you please take a look at this?

Environment

• LSP4E 0.19.7 (from main branch)
• LSP4J 0.24.0
• CDT LSP 3.1.100
• RHEL 9

C++ code used for testing:

namespace calculation {
    
    static double mult(double a, double b)
    {
        return a * b;
    }
    
    class AbstractCalculator
    {
        public:
        
        static double multiply(double a, double b);
        static double divide(double a, double b);
        double squareArea(double a, double b);
        double percentage(double a, double b);
    };
    
    double AbstractCalculator::multiply(double a, double b) { return mult(a, b); }
    
    double AbstractCalculator::divide(double a, double b) { return a / b; }
    
    class CalculatorImpl : public AbstractCalculator
    {
        public:
			double squareArea(double a, double b)
            {
                return AbstractCalculator::multiply(a, b);
            }
            
            double percentage(double a, double b)
            {
                return AbstractCalculator::multiply(divide(a, b), 100);
            }
	};
	
}

[travkin79]

Hello @ghentschke and @jonahgraham,

Do you experience similar renaming issues in your CDT LSP environment (see description above)?

[jonahgraham]

I do a little bit of weirdness, but mostly correct. I can't quite reproduce your case.

The most signficant issue I see is sometimes if I do:

1. Alt + Shift + R
2. Type new name
3. Type enter
4. Alt + Shift + R again

Then it corrupts the editor. The undo buffer is still correct.

This video shows step 1-4 above, followed by a bunch of Ctrl-Z:

rename-again-corrupts.webm

[travkin79]

Hi @jonahgraham,

Your example look somehow similar to one of my cases. Unfortunately, it's quite difficult to reliably reproduce the behavior, although it happens quite often (at least on my machine).

Hi @sebthom,

In the meanwhile I tried temporarily disabling in-line renaming for my team mates using a plugin_customization.ini and the preference ID org.eclipse.lsp4e.operations.rename.LSPInlineRenameLinkedMode#INLINE_RENAME_PREFERENCE, but it doesn't work. It seems, the reason is that LSPInlineRenameLinkedMode is reading the preference value directly from the instance scope instead of using the plug-in's preference store which also handles plugin_customization.ini and other ways of handling / setting default values.

@sebthom, is there a reason why you are reading the preference from the instance scope here? I expected something like the following instead or instead reading the value from the default scope first and use that value as fallback when reading from instance scope (which works as expected with plugin_customization.ini):

final var preferenceStore = LanguageServerPlugin.getDefault().getPreferenceStore();
return preferenceStore.getBoolean(INLINE_RENAME_PREFERENCE);