8174840: Elements.overrides does not check the return type of the methods #22920

nizarbenalla · 2025-01-05T22:16:45Z

Please review this PR to clarify the documentation of Elements.overrides through an @apiNote, this PR is essentially a doc-only change. This PR supersedes #22244.

TIA.

Progress

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

Issues

JDK-8174840: Elements.overrides does not check the return type of the methods (Bug - P4)
JDK-8356164: Elements.overrides does not check the return type of the methods (CSR)

Reviewers

Chen Liang (@liach - Reviewer)
Joe Darcy (@jddarcy - Reviewer)

Contributors

Pavel Rappo <prappo@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22920

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22920.diff

Using Webrev

Link to Webrev Comment

… PR.

…irement on the implementation.

bridgekeeper · 2025-01-05T22:17:37Z

👋 Welcome back nbenalla! 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.

openjdk · 2025-01-05T22:18:29Z

@nizarbenalla 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:

8174840: Elements.overrides does not check the return type of the methods

Co-authored-by: Pavel Rappo <prappo@openjdk.org>
Reviewed-by: liach, darcy

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 730 new commits pushed to the master branch:

620f816: 8327495: Print more warning with -Xshare:auto when CDS fails to use archive
6bcbc3b: 8353489: Increase timeout and improve Windows compatibility in test/jdk/java/lang/ProcessBuilder/Basic.java
65f188f: 8354334: Remove @valuebased from ProcessHandle
... and 727 more: https://git.openjdk.org/jdk/compare/5709f79cf86d532b38133a7fc11e6388af573723...master

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 · 2025-01-05T22:18:55Z

@nizarbenalla The following label will be automatically applied to this pull request:

compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

liach · 2025-01-06T03:42:41Z

src/java.compiler/share/classes/javax/lang/model/util/Elements.java

+     * on thrown types, return types and those constraints on method modifiers not directly
+     * bound to the overriding relation as such.


Suggested change

* on thrown types, return types and those constraints on method modifiers not directly

* bound to the overriding relation as such.

* on exception types, return types, and method modifiers do not affect

* the overriding relation.

The original wording has grammatical mistake and makes no sense.

I don't agree that it makes no sense but I find this wording simpler, so I'll use it instead.

I would strongly recommend to wait for @jddarcy to review that.

The basic situation is that the "overrides" relation a develop has in mind may not be the same as the more restricted "overrides" relation defined in JLS. However, we don't necessarily want to require the extra checks to be omitted and don't necessarily wan to forbid the extra checks from being done.

The current wording doesn't accomplish this.

mlbridge · 2025-01-06T12:39:56Z

Webrevs

nizarbenalla · 2025-01-06T12:40:45Z

/contributor add prappo

openjdk · 2025-01-06T12:40:57Z

@nizarbenalla
Contributor Pavel Rappo <prappo@openjdk.org> successfully added.

liach · 2025-01-06T19:12:28Z

test/langtools/tools/javac/processing/model/util/elements/overrides/S.java

@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights reserved.


Is the beginning copyright year 2023 correct?

Yes, it is. The patch attached to the JBS issue is dated 2023. That patch is used in this PR.

Thanks for the verification.

liach · 2025-01-06T19:18:06Z

test/langtools/tools/javac/processing/model/util/elements/overrides/TestOverrides.java

+                var tm = mIn(t);
+                if (!elements.overrides(tm, sm, t))
+                    messager.printError(String.format(
+                            "%s does not override %s from %s", tm, sm, t.getQualifiedName()));


This message implies sm is from t, which is not the case. Consider improving the failure message.

This uses the wording from JLS 8.4.8.1 (emphasis mine):

An instance method mC declared in or inherited by class C, overrides from C another method mA declared in class A, iff all of the following are true:

Then we should use
String.format("%s does not override from %s %s", tm, t.getQualifiedName(), sm). sm is not from t, the override is instead.

If you go with that, consider adding the word "method" in between the last two placeholders to clearly separate them:

from %s method %s"

jddarcy · 2025-01-07T06:13:40Z

test/langtools/tools/javac/processing/model/util/elements/overrides/S.java

+ * questions.
+ */
+
+class S {


Following the usual conventions for annotation processing tests, it would be possible (and IMO desirable) for these declarations to be included in the same file as the test scaffolding, assuming S compiles.

If S does not compile, that should be documented.

jddarcy · 2025-01-07T06:15:32Z

test/langtools/tools/javac/processing/model/util/elements/overrides/TestOverrides.java

+
+import static javax.lang.model.element.ElementKind.METHOD;
+
+/*


This comment would be helpful immediately before S and it would also be helpful for the various conditions being tested to be documented via comments.

nizarbenalla · 2025-01-23T13:55:45Z

Thanks for the reviews, Updated accordingly.

bridgekeeper · 2025-02-20T15:59:25Z

@nizarbenalla This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

nizarbenalla · 2025-02-20T16:09:20Z

/open

@jddarcy, could you re-review this PR?

openjdk · 2025-02-20T16:10:08Z

@nizarbenalla This pull request is already open

jddarcy · 2025-03-04T03:49:41Z

src/java.compiler/share/classes/javax/lang/model/util/Elements.java

+     * @apiNote It may not implement the additional compile-time checks on exception types,
+     * return types, and method modifiers specified in JLS {@jls 8.4.8.1} and {@jls 8.4.8.3},
+     * although implementations of this method are allowed to implement these additional checks.
+     *


I don't think the current wording conveys enough context for the reader. I suggest a structure like:

apiNote
This method must examines X, Y, and Z in determining whether one method overrides another. In addition, an implementation may have a stricter checking including at properties A, B, and C as described in JLS ..."

HTH

Updated in e21de7d -- Sorry for the delay.

bridgekeeper · 2025-04-25T06:35:10Z

@nizarbenalla This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

nizarbenalla · 2025-04-25T09:10:06Z

Keep alive.

liach

These update notes look good.

jddarcy · 2025-04-29T01:47:14Z

/csr needed

jddarcy

Current version of the javadoc update looks fine.

openjdk · 2025-04-29T01:48:31Z

@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request.

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

nizarbenalla · 2025-05-05T21:51:38Z

CSR request has been approved. Thank you all for the reviews!
/integrate

openjdk · 2025-05-05T21:52:29Z

Going to push as commit 606f201.
Since your change was applied there have been 732 commits pushed to the master branch:

2a4f37c: 8356119: Typo in bytecode behavior for Lookup.findGetter
ca7e4c4: 8352612: No way to add back lint categories after "none"
620f816: 8327495: Print more warning with -Xshare:auto when CDS fails to use archive
... and 729 more: https://git.openjdk.org/jdk/compare/5709f79cf86d532b38133a7fc11e6388af573723...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-05T21:52:35Z

@nizarbenalla Pushed as commit 606f201.

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

pavelrappo and others added 3 commits January 5, 2025 22:34

Initial commit obtained from a cherry pick/squash of a now superseded…

aa65705

… PR.

(C) 2025

36e5a47

Respond to feedback. Using Jan's comment to not imply particular requ…

fb11329

…irement on the implementation.

openjdk bot added the compiler compiler-dev@openjdk.org label Jan 5, 2025

liach reviewed Jan 6, 2025

View reviewed changes

nizarbenalla mentioned this pull request Jan 6, 2025

8174840: Elements.overrides does not check the return type of the methods #22244

Closed

3 tasks

Respond to feedback around the wording.

4f8877a

nizarbenalla marked this pull request as ready for review January 6, 2025 12:35

openjdk bot added the rfr Pull request is ready for review label Jan 6, 2025

liach reviewed Jan 6, 2025

View reviewed changes

jddarcy reviewed Jan 7, 2025

View reviewed changes

nizarbenalla added 3 commits January 23, 2025 13:55

Merge remote-tracking branch 'upstream/master' into elements-api-note

cc7cf7d

Update according to review comments.

3493320

whitespace

54d6890

jddarcy reviewed Mar 4, 2025

View reviewed changes

nizarbenalla added 2 commits March 27, 2025 01:21

Merge branch 'master' into elements-api-note

dc7bc76

feedback: improve api note to convey more context

e21de7d

liach approved these changes Apr 25, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 25, 2025

jddarcy approved these changes Apr 29, 2025

View reviewed changes

openjdk bot added the csr Pull request needs approved CSR before integration label Apr 29, 2025

openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated csr Pull request needs approved CSR before integration labels Apr 29, 2025

openjdk bot added the integrated Pull request has been integrated label May 5, 2025

openjdk bot closed this May 5, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 5, 2025

nizarbenalla deleted the elements-api-note branch May 5, 2025 21:52

		* on thrown types, return types and those constraints on method modifiers not directly
		* bound to the overriding relation as such.

		@@ -0,0 +1,52 @@
		/*
		* Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights reserved.


		import static javax.lang.model.element.ElementKind.METHOD;

		/*

8174840: Elements.overrides does not check the return type of the methods #22920

8174840: Elements.overrides does not check the return type of the methods #22920

Uh oh!

Conversation

nizarbenalla commented Jan 5, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Jan 5, 2025

Uh oh!

openjdk bot commented Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jan 5, 2025

Uh oh!

liach Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

nizarbenalla commented Jan 6, 2025

Uh oh!

openjdk bot commented Jan 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavelrappo Jan 6, 2025 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jddarcy Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nizarbenalla commented Jan 23, 2025

Uh oh!

bridgekeeper bot commented Feb 20, 2025

Uh oh!

nizarbenalla commented Feb 20, 2025

Uh oh!

openjdk bot commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Apr 25, 2025

Uh oh!

nizarbenalla commented Apr 25, 2025

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

jddarcy commented Apr 29, 2025

Uh oh!

nizarbenalla commented Jan 5, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 5, 2025 •

edited

Loading

liach Jan 6, 2025 •

edited

Loading

mlbridge bot commented Jan 6, 2025 •

edited

Loading

pavelrappo Jan 6, 2025 •

edited by bridgekeeper bot

Loading

jddarcy Jan 7, 2025 •

edited

Loading