Community
Participate
Working Groups
I20080613-2000 There should be a configurable warning which tells me when a class overrides a synchronized method without synchronizing it. package A; public class A { public synchronized void foo() { } } package A; public class B extends A { @Override public void foo() { // Warning should be flagged here super.foo(); } } Such a case usually indicates thread-unsafe code and it would be great if the compiler could help me in this case.
Good idea.
Created attachment 106233 [details] initial patch My first JDT/Core patch :) It would be great if someone could give feedback on the patch in order to improve it.
Will have a look at it.
The patch is impressive. Minor things to improve: 1- The method in the message should be decomposed in 3 pieces: {declaringClass}{selector}({arguments}) 417 = The method {0}.{1}({2}) is overriding a synchronized method without being synchronized 2- ProblemReport#missingSynchronizedOnInheritedMethod(...) the arguments should be specificed in readable form for first arguments, and shortReadable form for second array of arguments. This is how we achieve storing more information (with qualified names) in markers, but only short forms in the message, for improved readability. e.g. new String[] { new String(method.declaringClass.readableName()), new String(method.selector), typesAsString(method.isVarargs(), method.parameters, false), }, new String[] { new String(method.declaringClass.shortReadableName()), new String(method.selector), typesAsString(method.isVarargs(), method.parameters, true), }, 3- I would like to see more tests on MethodVerifyTest. Like what occurs with local & anonymous types, enum fields ? like for enum public enum X { FOO { void foo() { super.foo(); } }; synchronized void foo() {} } What also occurs in presence of return type covariance ? 4- The warning token in batch compiler "missingSynchronizedOnInheritedMethod" is just too long to remember. A shorter name would be better "syncOverride" ? But overall, the patch is very high quality, and is very respectful of the existing coding practice and style. If you address these issues, I believe we could commit it for 3.5M1. BTW - give what I saw, would you be willing to contribute more fixes, and grow into a committer (maybe?) ?
Created attachment 106812 [details] updated patch Philippe, thanks for the feedback! Here is an updated version of the patch. I changed the following items: * added contributor reference * improved message as suggested and added 4th placeholder * added the warning token to the help message of the batch compiler * added more testcase -> fix for enums ;) One thing I don't like at the moment is the textual presentation of problem for enums: "The method new X(){}.foo() is overriding a synchronized method without being synchronized" I looked at other places and it seems other problems have the same issue. But I think this could be tackled by another bug. See the following snippet for the same behavior: public enum E { FOO {@Override public void FOO() { }};} > BTW - give what I saw, would you be willing to contribute more fixes, and grow > into a committer (maybe?) ? Contributing more fixes - definitely! I really love to work on JDT (core and ui) so you'll see more patches from my side. Currently I'm doing a project as part of Google Summer of Code ( see http://wiki.eclipse.org/Concurrency-related_refactorings_for_JDT ) which is really tightly coupled to JDT. The compile warnings shown there are currently done by a compilationParticipant+AST reconciling. If the JDT/Core team is interested in one ore more of them to be I would be proud to rewrite them and provide patches. Regarding this bug: Any other objections we need to fix?
>If the JDT/Core team is interested in one ore more of them to be I would be >proud to rewrite them and provide patches. Sure. "Compile warnings for non-final locks" looks generally useful. I am less convinced about volatile arrays, but could be proven wrong.
Will open a new bug for non-final locks. To the volatile array warning: The idea is that declaring an array as volatile does not ensure the visibility of the values of the array. See for example the following urls: http://gee.cs.oswego.edu/dl/cpj/jmm.html "Similarly, declaring an array field as volatile does not ensure visibility of its elements. Volatility cannot be manually propagated for arrays because array elements themselves cannot be declared as volatile. " http://www.ibm.com/developerworks/java/library/j-jtp06197.html "...as when an array reference is declared volatile, only the reference, not the elements themselves, have volatile semantics..."
Philippe, anything I can do to push this forward?
Sorry for long silence, I was away on vacation. Re: additional warnings, please enter specific problem reports for these, and assign them to you. I will check your latest patch again, and commit it for 3.5M2 if all is good (as I think it will be). Thanks again.
Renamed new MethodVerifyTest tests into test170-173
Recording patch as external contribution from Benjamin Muskalla for automated IP log creation. Released for 3.5M2
Entry in build notes: Added a new compiler warning to signal absence of synchronized modifier when overriding a synchronized method. This diagnosis is controlled by option: JavaCore.COMPILER_PB_MISSING_SYNCHRONIZED_ON_INHERITED_METHOD and produces a problem marker which ID is IProblem.MissingSynchronizedModifierInInheritedMethod problem ID. Compiler option ID: Reporting Missing Synchronized Modifier On Inherited Method. When enabled, the compiler will issue an error or a warning if a method overrides a synchronized method without having a synchronized modifier. Option id: "org.eclipse.jdt.core.compiler.problem.missingSynchronizedOnInheritedMethod" Possible values: { "error", "warning", "ignore" } Default: "warning"
Verified for 3.5M2 using I20080914-2000
