From cdc3a1c490c041b5daf9ab096a7c03d6927b004b Mon Sep 17 00:00:00 2001 From: vromero Date: Mon, 18 Feb 2013 14:33:25 +0000 Subject: [PATCH] 6563143: javac should issue a warning for overriding equals without hashCode Reviewed-by: jjg, mcimadamore --- .../com/sun/tools/javac/code/Flags.java | 7 +++- .../com/sun/tools/javac/code/Lint.java | 3 -- .../com/sun/tools/javac/comp/Attr.java | 1 + .../com/sun/tools/javac/comp/Check.java | 32 +++++++++++++++++++ .../com/sun/tools/javac/comp/Resolve.java | 1 + .../tools/javac/resources/compiler.properties | 5 +++ .../sun/tools/sjavac/comp/Dependencies.java | 5 +-- .../OverridesEqualsButNotHashCodeTest.java | 22 +++++++++++++ .../OverridesEqualsButNotHashCodeTest.out | 2 ++ test/tools/javac/diags/examples.not-yet.txt | 1 + 10 files changed, 71 insertions(+), 8 deletions(-) create mode 100644 test/tools/javac/6563143/OverridesEqualsButNotHashCodeTest.java create mode 100644 test/tools/javac/6563143/OverridesEqualsButNotHashCodeTest.out diff --git a/src/share/classes/com/sun/tools/javac/code/Flags.java b/src/share/classes/com/sun/tools/javac/code/Flags.java index e48d9387..761a2407 100644 --- a/src/share/classes/com/sun/tools/javac/code/Flags.java +++ b/src/share/classes/com/sun/tools/javac/code/Flags.java @@ -258,7 +258,7 @@ public class Flags { public static final long CLASH = 1L<<42; /** - * Flag that marks either a default method or an interface containing default methods + * Flag that marks either a default method or an interface containing default methods. */ public static final long DEFAULT = 1L<<43; @@ -268,6 +268,11 @@ public class Flags { */ public static final long AUXILIARY = 1L<<44; + /** + * Flag that indicates that an override error has been detected by Check. + */ + public static final long BAD_OVERRIDE = 1L<<45; + /** Modifier masks. */ public static final int diff --git a/src/share/classes/com/sun/tools/javac/code/Lint.java b/src/share/classes/com/sun/tools/javac/code/Lint.java index ac2ef8d4..a80d6548 100644 --- a/src/share/classes/com/sun/tools/javac/code/Lint.java +++ b/src/share/classes/com/sun/tools/javac/code/Lint.java @@ -26,10 +26,7 @@ package com.sun.tools.javac.code; import java.util.EnumSet; -import java.util.HashMap; import java.util.Map; -import java.util.Set; -import javax.lang.model.element.Modifier; import com.sun.tools.javac.code.Symbol.*; import com.sun.tools.javac.util.Context; import com.sun.tools.javac.util.List; diff --git a/src/share/classes/com/sun/tools/javac/comp/Attr.java b/src/share/classes/com/sun/tools/javac/comp/Attr.java index 51df8b79..8842706a 100644 --- a/src/share/classes/com/sun/tools/javac/comp/Attr.java +++ b/src/share/classes/com/sun/tools/javac/comp/Attr.java @@ -3991,6 +3991,7 @@ public class Attr extends JCTree.Visitor { attribClassBody(env, c); chk.checkDeprecatedAnnotation(env.tree.pos(), c); + chk.checkClassOverrideEqualsAndHash(c); } finally { env.info.returnResult = prevReturnRes; log.useSource(prev); diff --git a/src/share/classes/com/sun/tools/javac/comp/Check.java b/src/share/classes/com/sun/tools/javac/comp/Check.java index cd42d4a6..19b91174 100644 --- a/src/share/classes/com/sun/tools/javac/comp/Check.java +++ b/src/share/classes/com/sun/tools/javac/comp/Check.java @@ -1588,6 +1588,7 @@ public class Check { (other.flags() & STATIC) == 0) { log.error(TreeInfo.diagnosticPositionFor(m, tree), "override.static", cannotOverride(m, other)); + m.flags_field |= BAD_OVERRIDE; return; } @@ -1599,6 +1600,7 @@ public class Check { log.error(TreeInfo.diagnosticPositionFor(m, tree), "override.meth", cannotOverride(m, other), asFlagSet(other.flags() & (FINAL | STATIC))); + m.flags_field |= BAD_OVERRIDE; return; } @@ -1615,6 +1617,7 @@ public class Check { other.flags() == 0 ? Flag.PACKAGE : asFlagSet(other.flags() & AccessFlags)); + m.flags_field |= BAD_OVERRIDE; return; } @@ -1642,6 +1645,7 @@ public class Check { "override.incompatible.ret", cannotOverride(m, other), mtres, otres); + m.flags_field |= BAD_OVERRIDE; return; } } else if (overrideWarner.hasNonSilentLint(LintCategory.UNCHECKED)) { @@ -1661,6 +1665,7 @@ public class Check { "override.meth.doesnt.throw", cannotOverride(m, other), unhandledUnerased.head); + m.flags_field |= BAD_OVERRIDE; return; } else if (unhandledUnerased.nonEmpty()) { @@ -1956,6 +1961,33 @@ public class Check { } } + public void checkClassOverrideEqualsAndHash(ClassSymbol someClass) { + if (lint.isEnabled(LintCategory.OVERRIDES)) { + boolean hasEquals = false; + boolean hasHashCode = false; + + Scope.Entry equalsAtObject = syms.objectType.tsym.members().lookup(names.equals); + Scope.Entry hashCodeAtObject = syms.objectType.tsym.members().lookup(names.hashCode); + for (Symbol s: someClass.members().getElements(new Filter() { + public boolean accepts(Symbol s) { + return s.kind == Kinds.MTH && + (s.flags() & BAD_OVERRIDE) == 0; + } + })) { + MethodSymbol m = (MethodSymbol)s; + hasEquals |= m.name.equals(names.equals) && + m.overrides(equalsAtObject.sym, someClass, types, false); + + hasHashCode |= m.name.equals(names.hashCode) && + m.overrides(hashCodeAtObject.sym, someClass, types, false); + } + if (hasEquals && !hasHashCode) { + log.warning(LintCategory.OVERRIDES, (DiagnosticPosition) null, + "override.equals.but.not.hashcode", someClass.fullname); + } + } + } + private boolean checkNameClash(ClassSymbol origin, Symbol s1, Symbol s2) { ClashFilter cf = new ClashFilter(origin.type); return (cf.accepts(s1) && diff --git a/src/share/classes/com/sun/tools/javac/comp/Resolve.java b/src/share/classes/com/sun/tools/javac/comp/Resolve.java index 981f2083..9fa37be0 100644 --- a/src/share/classes/com/sun/tools/javac/comp/Resolve.java +++ b/src/share/classes/com/sun/tools/javac/comp/Resolve.java @@ -3606,6 +3606,7 @@ public class Resolve { * while inapplicable candidates contain further details about the * reason why the method has been considered inapplicable. */ + @SuppressWarnings("overrides") class Candidate { final MethodResolutionPhase step; diff --git a/src/share/classes/com/sun/tools/javac/resources/compiler.properties b/src/share/classes/com/sun/tools/javac/resources/compiler.properties index 09bc7a0a..e1bfa1c3 100644 --- a/src/share/classes/com/sun/tools/javac/resources/compiler.properties +++ b/src/share/classes/com/sun/tools/javac/resources/compiler.properties @@ -2065,6 +2065,11 @@ compiler.warn.override.unchecked.thrown=\ {0}\n\ overridden method does not throw {1} +# 0: class name +compiler.warn.override.equals.but.not.hashcode=\ + Class {0}\n\ + overrides method equals but does not overrides method hashCode from Object + ## The following are all possible strings for the first argument ({0}) of the ## above strings. # 0: symbol, 1: symbol, 2: symbol, 3: symbol diff --git a/src/share/classes/com/sun/tools/sjavac/comp/Dependencies.java b/src/share/classes/com/sun/tools/sjavac/comp/Dependencies.java index 8f1930c8..4c3dab29 100644 --- a/src/share/classes/com/sun/tools/sjavac/comp/Dependencies.java +++ b/src/share/classes/com/sun/tools/sjavac/comp/Dependencies.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 1999, 2011, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1999, 2013, 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 @@ -113,9 +113,6 @@ public class Dependencies { return a.toString().compareTo(b.toString()); } - public boolean equals(Object obj) { - return super.equals(obj); - } } /** diff --git a/test/tools/javac/6563143/OverridesEqualsButNotHashCodeTest.java b/test/tools/javac/6563143/OverridesEqualsButNotHashCodeTest.java new file mode 100644 index 00000000..c586ec48 --- /dev/null +++ b/test/tools/javac/6563143/OverridesEqualsButNotHashCodeTest.java @@ -0,0 +1,22 @@ +/* + * @test /nodynamiccopyright/ + * @bug 6563143 + * @summary javac should issue a warning for overriding equals without hashCode + * @compile/ref=OverridesEqualsButNotHashCodeTest.out -Xlint:overrides -XDrawDiagnostics OverridesEqualsButNotHashCodeTest.java + */ + +@SuppressWarnings("overrides") +public class OverridesEqualsButNotHashCodeTest { + @Override + public boolean equals(Object o) { + return o == this; + } +} + +class Other { + @Override + public boolean equals(Object o) { + return o == this; + } +} + diff --git a/test/tools/javac/6563143/OverridesEqualsButNotHashCodeTest.out b/test/tools/javac/6563143/OverridesEqualsButNotHashCodeTest.out new file mode 100644 index 00000000..ad47ad61 --- /dev/null +++ b/test/tools/javac/6563143/OverridesEqualsButNotHashCodeTest.out @@ -0,0 +1,2 @@ +- compiler.warn.override.equals.but.not.hashcode: Other +1 warning diff --git a/test/tools/javac/diags/examples.not-yet.txt b/test/tools/javac/diags/examples.not-yet.txt index 437d61d2..2e442f38 100644 --- a/test/tools/javac/diags/examples.not-yet.txt +++ b/test/tools/javac/diags/examples.not-yet.txt @@ -110,4 +110,5 @@ compiler.warn.unchecked.cast.to.type # DEAD, replaced by comp compiler.warn.unexpected.archive.file # Paths: zip file with unknown extn compiler.warn.unknown.enum.constant # in bad class file compiler.warn.unknown.enum.constant.reason # in bad class file +compiler.warn.override.equals.but.not.hashcode # when a class overrides equals but not hashCode method from Object -- GitLab