Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61a00c0
Initial commit
pavelrappo Nov 19, 2024
a4f0b53
Fix copy-paste issues
pavelrappo Nov 19, 2024
a9a4309
Respond to feedback
pavelrappo Nov 20, 2024
add209c
Improve characterisation testcases
pavelrappo Nov 21, 2024
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
6 changes: 6 additions & 0 deletions src/java.compiler/share/classes/javax/lang/model/util/Elements.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,12 @@ default TypeElement getOutermostTypeElement(Element e) {
* elements.getTypeElement("I"));
* }
*
* @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 {@jls 8.4.8.3} for details. An implementation may choose not
* to check the additional requirements under some conditions.
*
Copy link
Member

Choose a reason for hiding this comment

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

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]".

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@pavelrappo pavelrappo Nov 20, 2024 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@pavelrappo pavelrappo Nov 21, 2024 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

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

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)

* @param overrider the first method, possible overrider
* @param overridden the second method, possibly being overridden
* @param type the class or interface of which the first method is a member
Expand Down
52 changes: 52 additions & 0 deletions test/langtools/tools/javac/processing/model/util/elements/overrides/S.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

class S {

public void m() { }
}

class T1 extends S {

protected void m() { }
}

class T2 extends S {

void m() { }
}

class T3 extends S {

private void m() { }
}

class T4 extends S {

public int m() { return 0; }
}

class T5 extends S {

public void m() throws Exception { }
}
68 changes: 68 additions & 0 deletions test/langtools/tools/javac/processing/model/util/elements/overrides/TestOverrides.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/*
* @test
* @bug 8174840
* @library /tools/javac/lib
* @build JavacTestingAbstractProcessor TestOverrides
* @compile -processor TestOverrides -proc:only S.java
*/

import java.util.Set;
import javax.annotation.processing.RoundEnvironment;
import javax.lang.model.element.ExecutableElement;
import javax.lang.model.element.TypeElement;

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

/*
* This test models a few cases where Elements.overrides produces a false
* positive which warrants @apiNote.
*/
public class TestOverrides extends JavacTestingAbstractProcessor {

@Override
public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment round) {
if (!round.processingOver()) {
var sm = mIn(elements.getTypeElement("S"));
for (var subtypeName : new String[]{"T1", "T2", "T3", "T4", "T5"}) {
var t = elements.getTypeElement(subtypeName);
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()));
}
}
return true;
}

private ExecutableElement mIn(TypeElement t) {
return t.getEnclosedElements().stream()
.filter(e -> e.getKind() == METHOD)
.filter(e -> e.getSimpleName().toString().equals("m"))
.map(e -> (ExecutableElement) e)
.findAny()
.get();
}
}