Show "change modifiers to final" in quick assists

[CsCherrYY]

Signed-off-by: Shi Chen chenshi@microsoft.com

modifiers.mp4

[rgrunber]

Works well for method parameters and local variable declarations but some issues with field declarations.

[rgrunber]

I noticed if I select multiple field declarations, I don't see the quick assist to change all modifiers.

public class Item {
    private String label;
    private float price;
}

It works just fine for local variable declarations. QuickAssistProcessor.getFullyCoveredNodes(..) finds the multiple field declarations, but VariableDeclarationFixCore.createChangeModifierToFinalFix(..) is what fails to generate the fix.

[CsCherrYY]

Actually, if a field has no initializer the source action is not available (since it would not be modified later), the source action in Source Action -> change modifiers to final where possible is not available too. if the sample code is

public class Item {
    private String label = "test";
    private float price = 1.0F;
}

the quick assist could be found.

[rgrunber]

Right. That makes sense. I think what confused me was that it's possible to add the modifier individually :

If a local variable (not a field) has no initializer I also noticed the quick assist shows up, so would that be a bug as well ?

[CsCherrYY]

Yeah, that should be a bug, I'll take a look.

[CsCherrYY]

I found if we directly pass a VariableDeclarationFragment to VariableDeclarationFixCore.createChangeModifierToFinalFix(), it will just ignore the initialization check, but for a FieldDeclaration, it will check its initializer as expected. So we should do this ourselves.

I have fix this via db41c99 and added some tests.

[CsCherrYY]

ah, I'm sorry that I didn't push a commit yesterday since I'm still finding a way to solve this :).

Besides, I also find an interesting thing, the language server will not report an error if a local final variable has not been initialized, but will report for a field. e.g.,

public class AddModifier {
	private final String label1; // jdtls will report The blank final field label1 may not have been initialized
	public void test() {
		final String label2; // jdtls only reports The value of the local variable label2 is not used
	}
}

So it might explain why the initializer check happens in fieldCanBeFinal()
but not canAddFinal(), you can also find related code here. https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/567c1546ee76fbbd4a50ee6b0de8e046a19356fe/org.eclipse.jdt.core.manipulation/core%20extension/org/eclipse/jdt/internal/corext/fix/VariableDeclarationFixCore.java#L187-L196

Coming back to the eclipse implementation, the change modifiers to final quick assist also appear for both label1(will cause an error) and label2(will not cause an error) in my example code. Anyway, showing the quick assist for label1 does not make sense to me since it will cause an error.

So our problems here are:

• if the compiler reports are correct, should we only show the quick assist for label2?
• if we can check the initializer ourselves, should we just do this check in jdtls, and create a new bug in JDT side, then remove this temporary check after the fix is available in upstream JDT?

[rgrunber]

To answer your question above, the final modifier on a field, requires that it be initialized as part of the declaration statement. That's why there is an error. When the final keyword is added to a local variable that same restriction does not apply. One can still initialize it anywhere else in the scope as long as it happens only once. So I would say the Add final modifier for ${var} should always be available on a local variable. I think the logic you added to check there's an initializer isn't needed. Sorry about the confusion.

For a field, the behaviour seems fine to me.

I think when that gets fixed, this should be ready to merge.

[CsCherrYY]

Get it. So I should remove the check of initializer here and open a ticket on JDT side?

[rgrunber]

For now let's just mimic the behaviour of VariableDeclarationFixCore as it may be doing the right thing. I think there's quite a few things that determine whether to show modifiers or not (initializer, local vs. field, visibility, etc.).

So if you make a selection that fully covers a node (eg local variable, or field) and the quick assist for "Change modifiers to final" shows up for that selection, then the "Add final modifier for ${var}" should also show up when you simply place the cursor on the node and trigger quick-assist that way.

If we can just be consistent in that sense, then I think that's good enough for now.

[CsCherrYY]

Sure. I have changed the behavior and added the related test cases. Now we will show quick assists for both variables and fields, regardless of initializer. @rgrunber Could you please check if there is any case I'm missing?

[rgrunber]

Just one thing to address and then feel free to squash and merge.

[rgrunber]

Use converingNode from above instead of context.getCoveringNode()