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

pavelrappo · 2024-11-19T16:08:18Z

Please review this essentially doc only change.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Error

⚠️ OCA signatory status must be verified

Issue

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22244

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-19T16:09:11Z

👋 Welcome back prappo! 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 · 2024-11-19T16:09:32Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-11-19T16:10:19Z

@pavelrappo 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.

mlbridge · 2024-11-19T16:13:39Z

Webrevs

jddarcy · 2024-11-20T05:37:41Z

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

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


If all these testing types could be put into the single test file, that would be preferable.

I condensed all the testing types in just one file. I couldn't put them further, in TestOverrides.java itself, because they would make it non-compilable. Additionally, I removed public from class S for symmetry: it shouldn't be special.

jddarcy · 2024-11-20T05:40:44Z

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

+     * @apiNote The notion of overriding, by itself, does not involve the
+     * method's return type, listed exceptions, or, to some extent, access
+     * modifiers. These are additional requirements checked by the compiler
+     * (see {@jls 8.4.8.3}).


Nit: use something like

(see JLS {@jls 8.4.8.3}).

as currently written, this would just render a section number pointing to a page.

jddarcy · 2024-11-20T05:58:35Z

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

+     * &mdash; such as when processing annotations &mdash; the additional
+     * requirements might not be checked, potentially causing this method
+     * to return a false positive.
+     *


The gist of this paragraph is good; some refinement is needed to not imply any particular requirements on the implementation, something like :"An implementation [of annotation processing] may choose to not check the additional requirements [under some limited conditions]".

Please correct me if I am wrong, but:

reading JLS 8.4.8.1 and JLS 8.4.8.3, 8.4.8.1 defines the "overrides" relation between two methods, and this relation does not include return type, throw exception, and to some degree modifiers. These are additional conditions, but these are conditions like "if m_C overrides m_A (when viewed from C)", and condition is met a compile-time error occurs. I.e. the fact that there e.g. an incorrect throws clause does not mean that m_C does not override m_A (when viewed from C), it only means a compile-time error occurs.

the Elements.overrides method strives to implement the relation, not the additional checks.

I agree it is reasonable to include a warning that only the relation is checked, not the additional constraints. But I find the wording "has not been sufficiently compiled" fairly confusing. I would try to point out this method only implements the "overrides" relation, not the additional constraints that are not part of the relation as such, as defined in JLS 8.4.8.1 and JLS 8.4.8.3. And hence, the two methods may satisfy the "overrides" relation, but still be part of erroneous code.

The gist of this paragraph is good; some refinement is needed to not imply any particular requirements on the implementation, something like :"An implementation [of annotation processing] may choose to not check the additional requirements [under some limited conditions]".

I tried to rephrase it the way you seem to have proposed; is that better?

My idea was something along these lines:

This method implements the overrides relation as specified in JLS 8.4.8.1. It does not implement the additional compile-time checks that Java compilers follow, specified in JLS 8.4.8.1 and 8.4.8.3, in particular the additional constraints on thrown types, return types and those constrains on method modifiers not directly bound to the overriding relation as such.

As I understand now, there may be implementations that also do the additional checks, so if we that should be permitted, the it could say "may not", instead of "does not".

Not sure if this makes sense.

I hear you: the Java Language Specification separates "overrides" from those additional checks.

So, if Elements.overrides models "overrides", it is under no obligation to perform those checks. Moreover, I think, it might not be always possible to perform them. Still, I believe we should NOT say that this method always skips them. So your "may not" is better.

Here's why. Whether any additional checks are performed seems to be implementation-dependant. For example, here's how ECJ (Eclipse) runs that same test (after some necessary modifications to decouple the test from our test infrastructure):

% java -jar ecj-4.33.jar --release 22 -classpath . -processor TestOverrides -proc:only S.java 1. ERROR: protected void m() does not override public void m() from T1 2. ERROR: void m() does not override public void m() from T2 3. ERROR: private void m() does not override public void m() from T3 3 problems (3 errors)

- Reduce the number of files - Rephrase apiNote

bridgekeeper · 2024-12-19T15:14:07Z

@pavelrappo 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!

pavelrappo · 2024-12-19T22:08:59Z

@jddarcy can we address this soon, or we should postpone this until after New Year?

pavelrappo · 2025-01-03T12:53:50Z

Keep alive.

nizarbenalla · 2025-01-06T12:31:29Z

This PR has been superseded by #22920. Please continue the discussion and review there.

Initial commit

61a00c0

openjdk bot added the rfr Pull request is ready for review label Nov 19, 2024

Fix copy-paste issues

a4f0b53

openjdk bot added the compiler compiler-dev@openjdk.org label Nov 19, 2024

jddarcy reviewed Nov 20, 2024

View reviewed changes

pavelrappo added 2 commits November 20, 2024 12:47

Respond to feedback

a9a4309

- Reduce the number of files - Rephrase apiNote

Improve characterisation testcases

add209c

bridgekeeper bot added the oca Needs verification of OCA signatory status label Jan 1, 2025

openjdk bot removed the rfr Pull request is ready for review label Jan 1, 2025

nizarbenalla mentioned this pull request Jan 6, 2025

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

Closed

4 tasks

pavelrappo closed this Jan 6, 2025

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

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

Uh oh!

Conversation

pavelrappo commented Nov 19, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Error

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Nov 19, 2024

Uh oh!

openjdk bot commented Nov 19, 2024

Uh oh!

openjdk bot commented Nov 19, 2024

Uh oh!

mlbridge bot commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jddarcy Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

pavelrappo Nov 20, 2024 • 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!

jddarcy Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

jddarcy Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

lahodaj Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

pavelrappo Nov 20, 2024 • 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!

lahodaj Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

pavelrappo Nov 21, 2024 • 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!

bridgekeeper bot commented Dec 19, 2024

Uh oh!

pavelrappo commented Dec 19, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

nizarbenalla commented Jan 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

pavelrappo commented Nov 19, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 19, 2024 •

edited

Loading

pavelrappo Nov 20, 2024 •

edited by bridgekeeper bot

Loading

pavelrappo Nov 20, 2024 •

edited by bridgekeeper bot

Loading

pavelrappo Nov 21, 2024 •

edited by bridgekeeper bot

Loading

pavelrappo commented Dec 19, 2024 •

edited by bridgekeeper bot

Loading

pavelrappo commented Jan 3, 2025 •

edited by bridgekeeper bot

Loading