Skip to content

Organize Members hint: don't rearrange record components #8220

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
import org.netbeans.api.java.source.*;
import org.netbeans.api.java.source.JavaSource.Phase;
import org.netbeans.api.java.source.ModificationResult.Difference;
import org.netbeans.api.progress.ProgressUtils;
import org.netbeans.api.progress.BaseProgressUtils;
import org.netbeans.editor.BaseAction;
import org.netbeans.editor.BaseDocument;
import org.netbeans.editor.GuardedDocument;
Expand Down Expand Up @@ -73,7 +73,7 @@
@Hint(displayName = "#DN_org.netbeans.modules.java.hints.OrganizeMembers", description = "#DESC_org.netbeans.modules.java.hints.OrganizeMembers", category = "class_structure", enabled = false)
public class OrganizeMembers {

@TriggerTreeKind(Kind.CLASS)
@TriggerTreeKind({Kind.CLASS, Kind.RECORD, Kind.ENUM})
public static ErrorDescription checkMembers(final HintContext context) {
for (Diagnostic<?> d : context.getInfo().getDiagnostics()) {
if (Hacks.isSyntaxError(d)) {
Expand Down Expand Up @@ -122,49 +122,44 @@ private static void doOrganizeMembers(WorkingCopy copy, TreePath path) {
ClassTree clazz = (ClassTree) path.getLeaf();
clazz = gu.importComments(clazz, copy.getCompilationUnit());
TreeMaker maker = copy.getTreeMaker();

ClassTree nue = maker.Class(clazz.getModifiers(), clazz.getSimpleName(), clazz.getTypeParameters(), clazz.getExtendsClause(), clazz.getImplementsClause(), clazz.getPermitsClause(), Collections.<Tree>emptyList());
List<Tree> members = new ArrayList<>(clazz.getMembers().size());
Map<Tree, Tree> memberMap = new HashMap<>(clazz.getMembers().size());
List<Tree> members = new ArrayList<>();
Map<Tree, Tree> memberMap = new HashMap<>();

List<Tree> enumValues = new ArrayList<>();
List<Tree> fixedMembers = new ArrayList<>();
for (Tree tree : clazz.getMembers()) {
if (copy.getTreeUtilities().isSynthetic(new TreePath(path, tree))) {
// isSynthetic does not consider record components to be synthetic
if (copy.getTreeUtilities().isSynthetic(new TreePath(path, tree))
&& !(tree.getKind() == Kind.VARIABLE && copy.getTreeUtilities().isRecordComponent((VariableTree)tree))) {
continue;
}
Comment on lines 131 to 136
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought this is lower risk than updating isSynthetic() although it probably should be updated at some point.

inserting

            if (path.getLeaf().getKind() == Kind.VARIABLE &&
                path.getParentPath() != null &&
                path.getParentPath().getLeaf().getKind() == Kind.RECORD) {
                Set<Modifier> mods = ((VariableTree) path.getLeaf()).getModifiers().getFlags();
                if (!mods.contains(Modifier.STATIC)) {
                    return true; // all non static record fields are synthetic
                }
            }

at

would work

Tree member;
switch (tree.getKind()) {
case CLASS:
case INTERFACE:
case ENUM:
case ANNOTATION_TYPE:
member = maker.setLabel(tree, ((ClassTree)tree).getSimpleName());
break;
case VARIABLE:
member = maker.setLabel(tree, ((VariableTree)tree).getName());
if (copy.getTreeUtilities().isEnumConstant((VariableTree)tree)) {
enumValues.add(member);
Tree member = switch (tree.getKind()) {
case CLASS, INTERFACE, ENUM, RECORD, ANNOTATION_TYPE ->
maker.setLabel(tree, ((ClassTree)tree).getSimpleName());
case VARIABLE -> {
VariableTree vt = (VariableTree)tree;
Tree mem = maker.setLabel(tree, vt.getName());
if (copy.getTreeUtilities().isEnumConstant(vt) || copy.getTreeUtilities().isRecordComponent(vt)) {
fixedMembers.add(mem);
}
break;
case METHOD:
member = maker.setLabel(tree, ((MethodTree)tree).getName());
break;
case BLOCK:
member = maker.asReplacementOf(maker.Block(((BlockTree)tree).getStatements(), ((BlockTree)tree).isStatic()), tree, true);
break;
default:
member = tree;
}
yield mem;
}
case METHOD -> maker.setLabel(tree, ((MethodTree)tree).getName());
case BLOCK -> maker.asReplacementOf(maker.Block(((BlockTree)tree).getStatements(), ((BlockTree)tree).isStatic()), tree, true);
default -> tree;
};
members.add(member);
memberMap.put(member, tree);
}
// fool the generator utilities with cloned members, so it does not take positions into account
if (enumValues.isEmpty()) {
if (fixedMembers.isEmpty()) {
nue = GeneratorUtilities.get(copy).insertClassMembers(nue, members);
} else {
members.removeAll(enumValues);
members.removeAll(fixedMembers);
int max = nue.getMembers().size();
// insert the enum values in the original order
for (Tree t : enumValues) {
// insert the enum values or record components in the original order
for (Tree t : fixedMembers) {
nue = maker.insertClassMember(nue, max++, t);
}
nue = GeneratorUtilities.get(copy).insertClassMembers(nue, members);
Expand All @@ -181,8 +176,8 @@ private static void doOrganizeMembers(WorkingCopy copy, TreePath path) {
}

private static boolean checkGuarded(Document doc, List<? extends Difference> diffs) {
if (doc instanceof GuardedDocument) {
MarkBlockChain chain = ((GuardedDocument) doc).getGuardedBlockChain();
if (doc instanceof GuardedDocument guardedDocument) {
MarkBlockChain chain = guardedDocument.getGuardedBlockChain();
for (Difference diff : diffs) {
if ((chain.compareBlock(diff.getStartPosition().getOffset(), diff.getEndPosition().getOffset()) & MarkBlock.OVERLAP) != 0) {
return true;
Expand Down Expand Up @@ -226,7 +221,7 @@ public void actionPerformed(final ActionEvent evt, final JTextComponent componen
final Source source = Source.create(doc);
if (source != null) {
final AtomicBoolean cancel = new AtomicBoolean();
ProgressUtils.runOffEventDispatchThread(new Runnable() {
BaseProgressUtils.runOffEventDispatchThread(new Runnable() {
@Override
public void run() {
try {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.netbeans.modules.java.hints;

import java.util.prefs.Preferences;
import org.netbeans.api.editor.mimelookup.MimeLookup;
import org.netbeans.junit.NbTestCase;
import org.netbeans.modules.editor.java.JavaKit;
import org.netbeans.modules.java.hints.test.api.HintTest;
import org.netbeans.modules.java.ui.FmtOptions;
import org.openide.util.NbBundle;

/**
* @author mbien
*/
public class OrganizeMembersTest extends NbTestCase {

public OrganizeMembersTest(String name) {
super(name);
}

@Override
protected void setUp() throws Exception {
super.setUp();
HintTest.create(); // initializes lookup (stolen from bugs.TinyTest)
MimeLookup.getLookup(JavaKit.JAVA_MIME_TYPE)
.lookup(Preferences.class)
.putBoolean(FmtOptions.sortMembersInGroups, true);
}

@Override
protected boolean runInEQ() {
// without it, hint markers are sometimes not found
return true;
}
Comment on lines +47 to +51
Copy link
Member Author

@mbien mbien Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't know why this is needed. OrganizeMembers invokes runModificationTask which calls copy.rewrite() at some point.

result.getDifferences(source.getFileObject()) is sometimes null if tests don't run on EDT.


public void testSimpleMemberSort() throws Exception {
HintTest.create()
.sourceLevel(11)
.input(
"""
package test;
public class Test {
public static int b;
public static int a;
}
"""
)
.run(OrganizeMembers.class)
.findWarning("2:4-2:24:verifier:" + NbBundle.getMessage(OrganizeMembers.class, "MSG_OragnizeMembers"))
.applyFix()
.assertOutput(
"""
package test;
public class Test {
public static int a;
public static int b;
}
"""
);
}

public void testRecordMemberSort() throws Exception {
HintTest.create()
.sourceLevel(17)
.input(
"""
package test;
public record Test(int d, int c) {
public static int b;
public static int a;
}
"""
)
.run(OrganizeMembers.class)
.findWarning("2:4-2:24:verifier:" + NbBundle.getMessage(OrganizeMembers.class, "MSG_OragnizeMembers"))
.applyFix()
.assertOutput(
"""
package test;
public record Test(int d, int c) {
public static int a;
public static int b;
}
"""
);
}

public void testEnumMemberSort() throws Exception {
HintTest.create()
.sourceLevel(11)
.input(
"""
package test;
public enum Test {
B, A;
public static int b;
public static int a;
}
"""
)
.run(OrganizeMembers.class)
.findWarning("3:4-3:24:verifier:" + NbBundle.getMessage(OrganizeMembers.class, "MSG_OragnizeMembers"))
.applyFix()
.assertOutput(
"""
package test;
public enum Test {
B, A;
public static int a;
public static int b;
}
"""
);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,14 @@ public boolean isEnum(ClassTree tree) {
public boolean isEnumConstant(VariableTree tree) {
return (((JCTree.JCModifiers) tree.getModifiers()).flags & Flags.ENUM) != 0;
}


/**
* Checks whether given variable tree represents a record component.
*/
public boolean isRecordComponent(VariableTree tree) {
return (((JCTree.JCModifiers) tree.getModifiers()).flags & Flags.RECORD) != 0;
}

/**Checks whether the given tree represents an annotation.
* @deprecated since 0.67, <code>Tree.getKind() == Kind.ANNOTATION_TYPE</code> should be used instead.
*/
Expand Down
Loading