-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8174840: Elements.overrides does not check the return type of the methods #22920
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
Changes from all commits
aa65705
36e5a47
fb11329
4f8877a
cc7cf7d
3493320
54d6890
dc7bc76
e21de7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /* | ||
| * Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights reserved. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the beginning copyright year 2023 correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it is. The patch attached to the JBS issue is dated 2023. That patch is used in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the verification. |
||
| * 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. | ||
| */ | ||
|
|
||
| /* | ||
| * This file models a few cases where Elements.overrides produces a false | ||
| * positive which warrants @apiNote. | ||
| */ | ||
|
|
||
| // S.java does not compile because it violates the JLS rules for overrides | ||
| class S { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| public void m() { } | ||
| } | ||
|
|
||
| // `protected` is a weaker modifier than `public` | ||
| class T1 extends S { | ||
|
|
||
| protected void m() { } | ||
| } | ||
|
|
||
| // `package-private` is a weaker modifier than `public` | ||
| class T2 extends S { | ||
|
|
||
| void m() { } | ||
| } | ||
|
|
||
| // `private` methods cannot override public method | ||
| class T3 extends S { | ||
|
|
||
| private void m() { } | ||
| } | ||
|
|
||
| // return type int is not compatible with void | ||
| class T4 extends S { | ||
|
|
||
| public int m() { return 0; } | ||
| } | ||
|
|
||
| // adding a checked exception violates the override rule | ||
| class T5 extends S { | ||
|
|
||
| public void m() throws Exception { } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| /* | ||
| * Copyright (c) 2023, 2025, 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; | ||
|
|
||
| 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 from %s method %s", tm, t.getQualifiedName(), sm)); | ||
| } | ||
| } | ||
| 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(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in e21de7d -- Sorry for the delay.