Add setting to make fields created via code action final #2371

mfussenegger · 2022-12-09T14:31:12Z

Adds a new codeGeneration.newFieldsFinal boolean setting which changes
the behavior of the "Assign parameter to new field" code action to
declare the new field as final.

In many applications it's preferred to make classes immutable if
possible. In such cases making fields final by default saves some
tedious editing.

mfussenegger · 2022-12-09T14:56:08Z

...c/org/eclipse/jdt/ls/core/internal/corrections/proposals/AssignToVariableAssistProposal.java

+		Preferences prefs = PreferenceManager.getPrefs(compilationUnit.getResource());
+		if (isParamToField && prefs.getCodeGenerationNewFieldsFinal()) {
+			modifiers |= Modifier.FINAL;
+		}


Is this also used in other contexts where adding final wouldn't make sense?

Looking at Eclipse's final cleanup (which was recently added as a cleanup action in https://github.com/eclipse/eclipse.jdt.ls/pull/2350/files#diff-c1fc3fbd7efda0e6e347dfc818586804aa68547c02b7c9abd75794e7a7481fadR46 .

So you could apply it to local variables, and even parameters. In this file I do see doAddparam, doAddLocal, but not sure how easy it is to accomplish that.

Another place this might go is in https://github.com/eclipse/eclipse.jdt.ls/blob/3400ec33d2b05e6ee639d90e2d3e1ed509c4b495/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/corrections/proposals/NewVariableCorrectionProposal.java#L391-L393

rgrunber

@fbricon , thoughts on having certain quick fixes / assists that create fields/parameters/locals add final by default (if possible) ?

rgrunber · 2022-12-12T22:22:50Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/Preferences.java

@@ -435,6 +435,9 @@ public class Preferences {
 	// A named preference that defines the location to insert the code generated by source actions.
 	public static final String JAVA_CODEGENERATION_INSERTIONLOCATION = "java.codeGeneration.insertionLocation";

+	// A named preference that defines that fields created via code generation should be declared final
+	public static final String JAVA_CODEGENERATION_NEW_FIELDS_FINAL = "java.codeGeneration.newFieldsFinal";


It almost seems to me like we should just make adding final (if possible) the default, and not bother with a setting, since it's just too small of a case. If we did it in more places, then maybe we could have a general setting for "Add final to declarations whenever possible`.

rgrunber · 2022-12-12T22:36:48Z

...c/org/eclipse/jdt/ls/core/internal/corrections/proposals/AssignToVariableAssistProposal.java

+		Preferences prefs = PreferenceManager.getPrefs(compilationUnit.getResource());
+		if (isParamToField && prefs.getCodeGenerationNewFieldsFinal()) {
+			modifiers |= Modifier.FINAL;
+		}


Looking at Eclipse's final cleanup (which was recently added as a cleanup action in https://github.com/eclipse/eclipse.jdt.ls/pull/2350/files#diff-c1fc3fbd7efda0e6e347dfc818586804aa68547c02b7c9abd75794e7a7481fadR46 .

So you could apply it to local variables, and even parameters. In this file I do see doAddparam, doAddLocal, but not sure how easy it is to accomplish that.

Another place this might go is in https://github.com/eclipse/eclipse.jdt.ls/blob/3400ec33d2b05e6ee639d90e2d3e1ed509c4b495/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/corrections/proposals/NewVariableCorrectionProposal.java#L391-L393

mfussenegger · 2022-12-14T16:53:07Z

thoughts on having certain quick fixes / assists that create fields/parameters/locals add final by default

Making it final by default sounds good to me, but I'd keep it limited to fields. Marking local variables as final is much more controversial and the benefit of doing that is more questionable.

mfussenegger · 2023-01-15T10:53:02Z

Any other inputs on this? Should I change it to make fields (but not local variables) final by default?

rgrunber · 2024-03-20T17:35:00Z

Just as an update, this change now requires the work be done upstream because we de-duplicated a lot of the classes. If we can't get the work accepted so that Eclipse itself makes the modifier final, we can always change the API so it allows us to make customizations on our side.

These should handle Assign statement to new (local variable|field) & Create (local variable|field) ...

https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/c7ecdcae923fcbd2b7333e2de0c8c7cf7bc28eae/org.eclipse.jdt.core.manipulation/proposals/org/eclipse/jdt/internal/ui/text/correction/proposals/AssignToVariableAssistProposalCore.java#L227

https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/c7ecdcae923fcbd2b7333e2de0c8c7cf7bc28eae/org.eclipse.jdt.core.manipulation/proposals/org/eclipse/jdt/internal/ui/text/correction/proposals/AssignToVariableAssistProposalCore.java#L232

(for the above 2, we need to add varValue.modifiers().addAll(ast.newModifiers(Modifier.FINAL));

https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/c7ecdcae923fcbd2b7333e2de0c8c7cf7bc28eae/org.eclipse.jdt.core.manipulation/proposals/org/eclipse/jdt/internal/ui/text/correction/proposals/NewVariableCorrectionProposalCore.java#L259

(in each of the if blocks, we need to make sure the new declaration has a line for : newDecl.modifiers().addAll(ast.newModifiers(Modifier.FINAL))

https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/c7ecdcae923fcbd2b7333e2de0c8c7cf7bc28eae/org.eclipse.jdt.core.manipulation/proposals/org/eclipse/jdt/internal/ui/text/correction/proposals/AssignToVariableAssistProposalCore.java#L391

https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/c7ecdcae923fcbd2b7333e2de0c8c7cf7bc28eae/org.eclipse.jdt.core.manipulation/proposals/org/eclipse/jdt/internal/ui/text/correction/proposals/NewVariableCorrectionProposalCore.java#L549

Update: I have a basic PR for jdt.core.manipulation that will introduce API that will allow us to set this on by default. I should have it merged soon, then this PR will become simpler.

rgrunber · 2024-03-26T16:02:32Z

Based on discussion from #3111 (comment) , I guess we should have a setting.

rgrunber · 2024-04-04T20:57:43Z

@mfussenegger , let me know if this works for your needs. It should cover quite a few quick assists. Setting is java.codeGeneration.addFinalForNewDeclaration, with values none, variables, fields, all .

I might add a few more testcases.

mfussenegger · 2024-04-05T06:55:24Z

@mfussenegger , let me know if this works for your needs. It should cover quite a few quick assists. Setting is java.codeGeneration.addFinalForNewDeclaration, with values none, variables, fields, all .

I might add a few more testcases.

Did a quick test and it seems to work as expected. Thanks for picking this up

rgrunber · 2024-04-10T17:25:04Z

Just noticed that the "Create (local variable|field)" doesn't actually work because we have no way to override the false value that the underlying API sets when calling the constructor to create the proposal (I was trying to avoid having addFinal be the default upstream). The proposal gets passed to JDT-LS so we can modify it, if the API just had some simple NewVariableCorrectionProposalCore.setAddFinal(boolean).

- Adds `java.codeGeneration.addFinalForNewDeclaration` setting which changes the behavior of the "Assign statement to new (local variable|field)", "Create (local variable|field)", "Assign parameter to new field", "Assign all parameters to new fields". - Possible values are "none", "variables", "fields", "all" - Fix up some test cases for ease of understanding - Update target platform to 4.32-I-builds/I20240417-1800 Signed-off-by: Roland Grunberg <[email protected]>

rgrunber · 2024-04-18T17:10:33Z

Thanks for proposing this. I think I could have probably made some constants for "all", "variables", "fields" but let's see if this needs to evolve / include more cases.

mfussenegger commented Dec 9, 2022

View reviewed changes

mfussenegger marked this pull request as ready for review December 9, 2022 16:49

rgrunber reviewed Dec 12, 2022

View reviewed changes

This was referenced Mar 25, 2024

Add fAddFinal to insert 'final' modifier to some proposals. eclipse-jdt/eclipse.jdt.ui#1283

Merged

Provide support for Java 22 #3111

Merged

rgrunber force-pushed the new-fields-final branch from a5ad8a5 to fbdd01c Compare April 4, 2024 20:54

rgrunber added this to the End April 2024 milestone Apr 6, 2024

rgrunber added enhancement code-actions labels Apr 6, 2024

rgrunber force-pushed the new-fields-final branch 2 times, most recently from c9e4853 to 60b34e7 Compare April 18, 2024 14:23

rgrunber force-pushed the new-fields-final branch from 60b34e7 to f972bde Compare April 18, 2024 15:44

rgrunber self-assigned this Apr 18, 2024

rgrunber merged commit b090203 into eclipse-jdtls:master Apr 18, 2024
7 checks passed

rgrunber mentioned this pull request Apr 22, 2024

Add setting java.codeGeneration.addFinalForNewDeclaration. redhat-developer/vscode-java#3586

Merged

rgrunber mentioned this pull request Jun 18, 2024

Provide option to extract to local *final* variable redhat-developer/vscode-java#3308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setting to make fields created via code action final #2371

Add setting to make fields created via code action final #2371

mfussenegger commented Dec 9, 2022

mfussenegger Dec 9, 2022

rgrunber Dec 12, 2022

rgrunber left a comment

rgrunber Dec 12, 2022

rgrunber Dec 12, 2022

mfussenegger commented Dec 14, 2022

mfussenegger commented Jan 15, 2023

rgrunber commented Mar 20, 2024 •

edited

Loading

rgrunber commented Mar 26, 2024

rgrunber commented Apr 4, 2024

mfussenegger commented Apr 5, 2024

rgrunber commented Apr 10, 2024

rgrunber commented Apr 18, 2024

Add setting to make fields created via code action final #2371

Add setting to make fields created via code action final #2371

Conversation

mfussenegger commented Dec 9, 2022

mfussenegger Dec 9, 2022

Choose a reason for hiding this comment

rgrunber Dec 12, 2022

Choose a reason for hiding this comment

rgrunber left a comment

Choose a reason for hiding this comment

rgrunber Dec 12, 2022

Choose a reason for hiding this comment

rgrunber Dec 12, 2022

Choose a reason for hiding this comment

mfussenegger commented Dec 14, 2022

mfussenegger commented Jan 15, 2023

rgrunber commented Mar 20, 2024 • edited Loading

rgrunber commented Mar 26, 2024

rgrunber commented Apr 4, 2024

mfussenegger commented Apr 5, 2024

rgrunber commented Apr 10, 2024

rgrunber commented Apr 18, 2024

rgrunber commented Mar 20, 2024 •

edited

Loading