Skip to content
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

8313651: Add 'final' keyword to public property methods in controls #1213

Closed

Conversation

andy-goryachev-oracle
Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle commented Aug 17, 2023

In the Control hierarchy, all property accessor methods must be declared final.

Added a test to check for missing final keyword and added the said keyword where required.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8314581 to be approved
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

  • JDK-8313651: Add 'final' keyword to public property methods in controls (Bug - P3)
  • JDK-8314581: Add 'final' keyword to public property methods in controls (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1213/head:pull/1213
$ git checkout pull/1213

Update a local copy of the PR:
$ git checkout pull/1213
$ git pull https://git.openjdk.org/jfx.git pull/1213/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1213

View PR using the GUI difftool:
$ git pr show -t 1213

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1213.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 17, 2023

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@andy-goryachev-oracle
Copy link
Contributor Author

/csr

@openjdk openjdk bot added the csr Need approved CSR to integrate pull request label Aug 17, 2023
@openjdk
Copy link

openjdk bot commented Aug 17, 2023

@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@andy-goryachev-oracle please create a CSR request for issue JDK-8313651 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

@andy-goryachev-oracle andy-goryachev-oracle marked this pull request as ready for review August 18, 2023 21:40
@openjdk openjdk bot added the rfr Ready for review label Aug 18, 2023
@mlbridge
Copy link

mlbridge bot commented Aug 18, 2023

Comment on lines 208 to 216
sb.append(prefix);
for (int i = 0; i < name.length(); i++) {
char c = name.charAt(i);
if (i == 0) {
sb.append(Character.toUpperCase(c));
} else {
sb.append(c);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sb.append(prefix);
for (int i = 0; i < name.length(); i++) {
char c = name.charAt(i);
if (i == 0) {
sb.append(Character.toUpperCase(c));
} else {
sb.append(c);
}
}
sb.append(prefix);
sb.append(Character.toUpperCase(name.charAt(0)));
sb.append(name, 1, name.length);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, thanks!

@kevinrushforth
Copy link
Member

At first glance this looks good, but I'll need time to get back to it. And thanks for adding a test.

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Aug 18, 2023

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

@andy-goryachev-oracle
Copy link
Contributor Author

And thanks for adding a test.

I feel the test can be improved by collecting the classes automatically, so as to catch possible issues when new controls are added (it'll be a full classpath scan though). While the scan should be limited to Controls, I wonder if other classes can benefit from enforcing the 'final' constraint?

@kevinrushforth
Copy link
Member

Um, "classpath"? JavaFX is a collection of modules that are found on the module path. In any case, this seems better done as follow-up issue, if you want to file one.

Btw, having an automatic scan of "everything" seems like overkill, and isn't really desirable, so if you do want to go down this route, I'd keep it simple. Since any non-exported package (i.e., any pacakge that isn't under javafx.**) will already need to be explicitly listed in the test/addExports file, and since we so very rarely add a new package, you might consider having a hard coded List of packages that you enumerate looking for all classes in each package for ones with properties.

@andy-goryachev-oracle
Copy link
Contributor Author

Um, "classpath"?

The problem I was referring to is how to enumerate all descendants of Control in javafx.* hierarchy, and I don't have a good solution. So, no change in the unit test is being planned, and all newly developed controls will have to be added manually.

Comment on lines 83 to 84
import org.junit.Assert;
import org.junit.Test;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are JUnit 4, we should use 5 for new tests.

Comment on lines 97 to 98
public class ControlPropertiesTest {
private static final boolean FAIL_FAST = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use an empty line after the class declaration.

Comment on lines 99 to 103

// or perhaps collect all classes in a package as described here:
// https://stackoverflow.com/questions/28678026/how-can-i-get-all-class-files-in-a-specific-package-in-java
private Class[] allControlClasses() {
return new Class[] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was also thinking that these should be generated by the contents of the package. When I looked at javafx.scene.control I saw classes that weren't controls, but were used by them, so they would need to be removed. So it's a question of whitelist vs. blacklist.

In any case, I'm not sure why you're using an array here. Set.of seems more suitable and will also check for duplicate entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Respectfully disagree - these classes (in javafx.scene.control.cell) are public and in the Control hierarchy, so are subject of the 'final' limitation.

There are no duplicate classes.

The bigger problem is (my) inability to extract the list/set/array of classes automatically.

Copy link
Collaborator

Choose a reason for hiding this comment

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

these classes (in javafx.scene.control.cell) are public and in the Control hierarchy, so are subject of the 'final' limitation.

I'm confused. Which classes should appear in this list exactly (what are the rules to determine that)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which classes should appear in this list exactly

subclasses of javafx.scene.control.Control

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, this is not easy. I thought that we need the classes in the control package. At this point it might just be simpler to use the hardcoded class list you already made and "hope" that new classes are added. There won't be many of these I suspect.

Comment on lines 169 to 232
@Test
public void testMissingFinalMethods() {
for (Class c: allControlClasses()) {
check(c);
}
}

private void check(Class cls) {
Method[] methods = cls.getMethods();

HashMap<String, Method> h = new HashMap<>();
for (Method m: methods) {
h.put(m.getName(), m);
}

for (Method m: methods) {
String name = m.getName();
if (name.endsWith("Property")) {
int mod = m.getModifiers();
if (Modifier.isPublic(mod)) {
if (FAIL_FAST) {
Assert.assertTrue(err(m, "is not final"), Modifier.isFinal(mod));
} else {
if (!Modifier.isFinal(mod)) {
System.err.println(err(m, "is not final"));
}
}
}

String propName = name.substring(0, name.length() - "Property".length());
check(h, propName, "get", 0);
check(h, propName, "set", 1);
check(h, propName, "is", 0);
}
}
}

private void check(HashMap<String, Method> map, String name, String prefix, int numArgs) {
StringBuilder sb = new StringBuilder(64);
sb.append(prefix);
sb.append(Character.toUpperCase(name.charAt(0)));
sb.append(name, 1, name.length());

String s = sb.toString();
Method m = map.get(s);
if (m != null) {
if (m.getParameterCount() == numArgs) {
int mod = m.getModifiers();
if (Modifier.isPublic(mod)) {
if (FAIL_FAST) {
Assert.assertTrue(err(m, "is not final"), Modifier.isFinal(mod));
} else {
if (!Modifier.isFinal(mod)) {
System.err.println(err(m, "is not final"));
}
}
}
}
}
}

private String err(Method method, String message) {
return method + " " + message;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

(This comment didn't appear in the review for some reason.)

I think that the code could be a bit more clean. I took a stab at it:

    Set<Class<?>> CLASSES = Set.of(/*...*/);

    private void check(Class<?> cls) {
        Map<String, Method> methods = Stream.of(cls.getMethods()).collect(Collectors.toMap(Method::getName, m -> m)); // the loop is also fine
        
        for (var method : methods.values()) {
            String name = method.getName();
            if (name.endsWith("Property")) {
                checkModifiers(method);

                String propName = name.substring(0, name.length() - "Property".length());
                check(methods, propName, "get", 0);
                check(methods, propName, "set", 1);
                check(methods, propName, "is", 0);
            }
        }
    }

    private void check(Map<String, Method> methods, String name, String prefix, int numArgs) {
        String methodName = new StringJoiner("", prefix, name.substring(0, 1).toUpperCase() + name.substring(1)).toString();

        Method m = methods.get(methodName);
        if (m != null && m.getParameterCount() == numArgs) {
            checkModifiers(m);
        }
    }

    private void checkModifiers(Method m) {
        int mod = m.getModifiers();
        if (Modifier.isPublic(mod) && !Modifier.isFinal(mod)) {
            if (FAIL_FAST) {
                throw new AssertionError(errorMessage(m));
            } else {
                System.err.println(errorMessage(m));
            }
        }
    }

    private String errorMessage(Method method) {
        return method + "is not final";
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for suggestions!

Frankly, I don't see much value in rewriting of this method. A more interesting problem is how to enumerate all descendants of the Control class using the package name, in such a way that will reliably work in an IDE, from a JAR, or in any test framework (jtreg or otherwise).

edit: about the test itself - I see little difference between the two version, but will gladly use your version if you feel strongly about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

edit: about the test itself - I see little difference between the two version, but will gladly use your version if you feel strongly about it.

I don't, I just think it's more readable. Fine to leave as is. What I might strongly suggest is extracting the checkModifiers method that is used in 2 places in your version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, and also fixed a stupid mistake ... method names are not unique.
many thanks!


private static final boolean FAIL_FAST = !true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you intend to commit this? It was true before which seems good for tests, otherwise I'd really suggest using false here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, committed by accident. thanks for noticing!

Copy link
Collaborator

@nlisker nlisker left a comment

Choose a reason for hiding this comment

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

Looks good. Gave a couple of minor optional suggestions.

As to the topic of class finding, I think it's fine to use a manual list, at least for now. The complexity of automatically detecting the classes might not be worth the hassle. Leaving it up to you.

*/
@Test
public void testMissingFinalMethods() {
for (Class c: allControlClasses()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we use a space before :. Don't know if it's being enforced. If you change it there are 2 more places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding superfluous spaces increases our carbon footprint ;-)
Modified the formatter to increase the footprint.

Copy link
Member

Choose a reason for hiding this comment

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

The space before : is still missing here.

}

/**
* Tests for missing final keyword in Control properties and their accessors.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically, we're looking at mutators too, not just accessors.

import com.sun.javafx.scene.control.skin.FXVK;

/**
* Tests contract for properties and their accessors in the Control type hierarchy.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mutators too?

@kevinrushforth kevinrushforth self-requested a review August 29, 2023 15:59
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

The fix looks good. So does the test with a couple comments. I also noted a couple potential follow-up items.

* in the Control type hierarchy.
*
* Currently uses a list of classes, so any new Controls must be added manually.
* Perhaps the test should scan classpath and find all the Controls automagically.
Copy link
Member

Choose a reason for hiding this comment

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

Minor: "classpath" --> "modulepath"

More interesting question: Should we do this in other modules (e.g., javafx.graphics), too? If so, that would be for a follow-up issue.


// list all descendants of Control class.
// or perhaps collect all classes in a package as described here:
// https://stackoverflow.com/questions/28678026/how-can-i-get-all-class-files-in-a-specific-package-in-java
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the reference to stackoverflow.

*/
@Test
public void testMissingFinalMethods() {
for (Class c: allControlClasses()) {
Copy link
Member

Choose a reason for hiding this comment

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

The space before : is still missing here.


private void checkModifiers(Method m) {
int mod = m.getModifiers();
if (Modifier.isPublic(mod) && !Modifier.isFinal(mod)) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we also check protected methods? If so, that would be something for a follow-up issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

easy enough to add, and makes sense.

}

private void check(Class cls) {
Method[] methods = cls.getMethods();
Copy link
Member

Choose a reason for hiding this comment

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

This will only return public methods. Since you modified checkModifiers to check protected methods, you will need to use getDeclaredMethods instead. This will return protected (and package scope) methods as well as public methods. It won't return any method in a superclass, but since the set of classes you are checking includes intermediate classes (such as ButtonBase and Labeled), you will still get the coverage (and without redundantly checking the same method in those classes multiple times).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, what was I thinking??
And we also have Region.setWidth() which is protected and not final by design. Reverting the last change.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Looks good.

@kevinrushforth
Copy link
Member

@nlisker @hjohn Can you re-review?

Copy link
Collaborator

@nlisker nlisker left a comment

Choose a reason for hiding this comment

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

Looks good. Added some minor comments.

Comment on lines 102 to 103
private Set<Class> allControlClasses() {
return Set.of(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that it matters much since the method is called once, but why is there a need for a method that returns the same constant set? The set can just be a constant: private final Set<Class<?>> = Set.of(...);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will keep as a method, if we ever get to extracting classes at run time.

Comment on lines +214 to +216
} else {
System.err.println(msg);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for an else clause since throw will exit the scope of the method if it's reached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

personal style.

private static final boolean FAIL_FAST = true;

// list all current descendants of Control class.
private Set<Class> allControlClasses() {
Copy link
Collaborator

@nlisker nlisker Sep 8, 2023

Choose a reason for hiding this comment

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

Please use Class<?> instead of raw types. In 2 other places below as well.

@openjdk
Copy link

openjdk bot commented Sep 8, 2023

@andy-goryachev-oracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8313651: Add 'final' keyword to public property methods in controls

Reviewed-by: jhendrikx, nlisker, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

  • eb7de72: 8315317: Add test for JDK-8262518
  • ed92171: 8315870: icu fails to compile with Visual Studio 2022 17.6.5

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added ready Ready to be integrated and removed csr Need approved CSR to integrate pull request labels Sep 8, 2023
@andy-goryachev-oracle
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Sep 11, 2023

Going to push as commit 624fe86.
Since your change was applied there have been 3 commits pushed to the master branch:

  • 325be56: 8310666: gradle validateSourceSets task not run when TEST_ONLY=true
  • eb7de72: 8315317: Add test for JDK-8262518
  • ed92171: 8315870: icu fails to compile with Visual Studio 2022 17.6.5

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 11, 2023
@openjdk openjdk bot closed this Sep 11, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Sep 11, 2023
@openjdk
Copy link

openjdk bot commented Sep 11, 2023

@andy-goryachev-oracle Pushed as commit 624fe86.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@andy-goryachev-oracle andy-goryachev-oracle deleted the 8313651.final branch September 11, 2023 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants