Community
Participate
Working Groups
BETA_JAVA8: In the following program, the compiler hints the user that X.foo could be declared as static. This is wrong and will result in the code not continuing to compile: // --- interface I { void foo(int i); } public class X { public static void main(String[] args) { X x = null; I i = x::foo; try { i.foo(10); } catch (NullPointerException npe) { System.out.println(npe.getMessage()); } } int foo(int x) { return x; } }
Shankha, please follow up, TIA.
Created attachment 230585 [details] WIP: Patch for the change including the test case I needed help on addition of the test case. I have not been able to figure out the correct way to add the test case. I am not able to specify the compliance level. Thanks
(In reply to comment #2) > Created attachment 230585 [details] > WIP: Patch for the change including the test case > > I needed help on addition of the test case. I have not been able to figure > out the correct way to add the test case. I am not able to specify the > compliance level. You have posted the patch in the wrong bug, it should have been attached to bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=406619.
Created attachment 230852 [details] Patch RunAllJDTCore tests and RunAllJava8Tests are fine. I have a question regarding if (this.haveReceiver && someMethod != null && !someMethod.isStatic() && isMethodReference) { AbstractMethodDeclaration srcMethod = someMethod.sourceMethod(); if(srcMethod != null && srcMethod.isMethod() && (srcMethod.bits & ASTNode.CanBeStatic) != 0) srcMethod.bits &= ~ASTNode.CanBeStatic; } This is the core change in ReferenceExpression.java. Is the check of isMethodReference necessary. I could not think of a case where it would be on along with this.haveReceiver. Thanks.
Review comments: (1) We still emit an incorrect warning for: / --- interface I { void doit (X x); } public class X { void foo() { return; } public static void main(String[] args) { I i = X::foo; } } (2) Our coding practice calls for a space between if and '(' (3) In reply to comment #4) > Is the check of > isMethodReference necessary. I could not think of a case where it would be > on along with this.haveReceiver. I don't think it is needed. (4) In if(srcMethod != null && srcMethod.isMethod() && (srcMethod.bits & ASTNode.CanBeStatic) != 0) srcMethod.bits &= ~ASTNode.CanBeStatic; the check (srcMethod.bits & ASTNode.CanBeStatic) != 0) is wasteful. please get rid of it. (5) This problem may be unsolvable in he general case when separate compilation units are involved. For example: // --- public class Y { void foo() { return; } } // -- interface I { void doit (); } public class X { void foo() { return; } public static void main(String[] args) { I i = new Y()::foo; } } This set of files actually trigger an NPE. In sourceMethod() the statement: AbstractMethodDeclaration[] methods = sourceType.scope.referenceContext.methods; triggers an NPE because scope is nulled out by org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.cleanUp() So the above statement should conservatively be changed to: AbstractMethodDeclaration[] methods = sourceType.scope == null ? null : sourceType.scope.referenceContext.methods; Please make this change and also add a test for this scenario.
Created attachment 231130 [details] Patch About the example: // --- public class Y { void foo() { return; } } // -- interface I { void doit (); } public class X { void foo() { return; } public static void main(String[] args) { I i = new Y()::foo; } } As you said this involves two different compilation units. The compiler provides a hint that the function "foo" belonging to class Y can be marked as static. If we mark it as static we get a error: The method foo() from the type Y should be accessed in a static . So somehow the information from one compilation unit is made available for the other compilation unit. Request to review the patch.
(In reply to comment #6) > Created attachment 231130 [details] > Patch > > About the example: > // --- > > public class Y { > void foo() { > return; > } > } > > // -- > interface I { > void doit (); > } > > public class X { > void foo() { > return; > } > > public static void main(String[] args) { > I i = new Y()::foo; > } > } > > As you said this involves two different compilation units. The compiler > provides a hint that the function "foo" belonging to class Y can be marked > as static. If we mark it as static we get a error: > > The method foo() from the type Y should be accessed in a static . > > So somehow the information from one compilation unit is made available for > the other compilation unit. > > > Request to review the patch. RunAllJava8Tests were fine.
Created attachment 231202 [details] Potentially simpler patch Shankha, please take a look at this patch: (1) It simplifies and merges two blocks of code into one. (2) Removes incorrect comment about haveReceiver - true means there is a captured object and false means pure type and not vice versa as the comment in the patch indicates. Please also add a test for comment#5 point#5 to prove that the code change in MethodBinding solves the NPE issue.
Created attachment 231214 [details] New patch to add the test case which lead to NPE. Replacing: "Potentially simpler patch" attachment I have added the test case. RunAllJava8 tests is clean. Do I need test406859d and test406859c both or I should remove test406859c ? The error condition (described below) is being hit by test384750z, therefore not adding any other test case. ReferenceExpression.java if (this.binding.isStatic()) { if (this.binding.declaringClass != this.receiverType) scope.problemReporter().indirectAccessToStaticMethod(this, this.binding); } Thanks.
Please ignore my earlier patch attachment 231214 [details]. Only the test case got posted. I will repost the patch. Thanks
Fix and test released here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=4d3e86023b52d983561e88d8a94124c1505fe619 Thanks Shankha
Comment on attachment 231214 [details] New patch to add the test case which lead to NPE. Replacing: "Potentially simpler patch" attachment >diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NegativeLambdaExpressionsTest.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NegativeLambdaExpressionsTest.java >index d00f07a..01709df 100644 >--- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NegativeLambdaExpressionsTest.java >+++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NegativeLambdaExpressionsTest.java >@@ -6535,4 +6535,131 @@ > ); > } >+//https://bugs.eclipse.org/bugs/show_bug.cgi?id=406859, [1.8][compiler] Bad hint that method could be declared static >+public void test406859a() { >+ Map compilerOptions = getCompilerOptions(); >+ compilerOptions.put(CompilerOptions.OPTION_ReportMethodCanBeStatic, CompilerOptions.ERROR); >+ compilerOptions.put(CompilerOptions.OPTION_ReportMethodCanBePotentiallyStatic, CompilerOptions.ERROR); >+ this.runNegativeTest( >+ new String[] { >+ "X.java", >+ "interface I {\n" + >+ " int foo(int i);\n" + >+ "}\n" + >+ "public class X {\n" + >+ " public static void main(String[] args) {\n" + >+ " X x = new X();\n" + >+ " I i = x::foo;\n" + >+ " i.foo(3);\n" + >+ " }\n" + >+ " int foo(int x) {\n" + >+ " return x;\n" + >+ " } \n" + >+ "}\n" >+ }, >+ "", >+ null /* no extra class libraries */, >+ true /* flush output directory */, >+ compilerOptions /* custom options */ >+ ); >+} >+//https://bugs.eclipse.org/bugs/show_bug.cgi?id=406859, [1.8][compiler] Bad hint that method could be declared static >+public void test406859b() { >+ Map compilerOptions = getCompilerOptions(); >+ compilerOptions.put(CompilerOptions.OPTION_ReportMethodCanBeStatic, CompilerOptions.ERROR); >+ compilerOptions.put(CompilerOptions.OPTION_ReportMethodCanBePotentiallyStatic, CompilerOptions.ERROR); >+ this.runNegativeTest( >+ new String[] { >+ "X.java", >+ "interface I {\n" + >+ " void doit (Y y);\n" + >+ "}\n" + >+ "\n" + >+ "class Y {\n" + >+ " void foo() {\n" + >+ " return;\n" + >+ " }\n" + >+ "}\n" + >+ "\n" + >+ "public class X {\n" + >+ " public static void main(String[] args) {\n" + >+ " I i = Y::foo; \n" + >+ " Y y = new Y();\n" + >+ " i.doit(y);\n" + >+ " }\n" + >+ "}\n" >+ }, >+ "", >+ null /* no extra class libraries */, >+ true /* flush output directory */, >+ compilerOptions /* custom options */ >+ ); >+} >+//https://bugs.eclipse.org/bugs/show_bug.cgi?id=406859, [1.8][compiler] Bad hint that method could be declared static >+public void test406859c() { >+ Map compilerOptions = getCompilerOptions(); >+ compilerOptions.put(CompilerOptions.OPTION_ReportMethodCanBeStatic, CompilerOptions.ERROR); >+ compilerOptions.put(CompilerOptions.OPTION_ReportMethodCanBePotentiallyStatic, CompilerOptions.ERROR); >+ this.runNegativeTest( >+ new String[] { >+ "X.java", >+ "interface I {\n" + >+ " void doit ();\n" + >+ "}\n" + >+ "\n" + >+ "class Y {\n" + >+ " void foo() { \n" + >+ " return;\n" + >+ " }\n" + >+ "}\n" + >+ "\n" + >+ "public class X {\n" + >+ " public static void main(String[] args) {\n" + >+ " I i = new Y()::foo;\n" + >+ " i.doit();\n" + >+ " }\n" + >+ "}\n" >+ }, >+ "", >+ null /* no extra class libraries */, >+ true /* flush output directory */, >+ compilerOptions /* custom options */ >+ ); >+} >+//https://bugs.eclipse.org/bugs/show_bug.cgi?id=406859, [1.8][compiler] Bad hint that method could be declared static >+public void test406859d() { >+ Map compilerOptions = getCompilerOptions(); >+ compilerOptions.put(CompilerOptions.OPTION_ReportMethodCanBeStatic, CompilerOptions.ERROR); >+ compilerOptions.put(CompilerOptions.OPTION_ReportMethodCanBePotentiallyStatic, CompilerOptions.WARNING); >+ this.runNegativeTest( >+ new String[] { >+ "Y.java", >+ "public class Y {\n" + >+ " void foo() {\n" + >+ " return;\n" + >+ " }\n" + >+ "}", >+ "X.java", >+ "interface I {\n" + >+ " void doit ();\n" + >+ "}\n" + >+ "\n" + >+ "public class X {\n" + >+ " public static void main(String[] args) {\n" + >+ " I i = new Y()::foo;\n" + >+ " i.doit();\n" + >+ " }\n" + >+ "}\n" >+ }, >+ "----------\n" + >+ "1. WARNING in Y.java (at line 2)\n" + >+ " void foo() {\n" + >+ " ^^^^^\n" + >+ "The method foo() from the type Y can potentially be declared as static\n" + >+ "----------\n", >+ null /* no extra class libraries */, >+ true /* flush output directory */, >+ compilerOptions /* custom options */ >+ ); >+} > public static Class testClass() { > return NegativeLambdaExpressionsTest.class;
(In reply to comment #12) Please ignore comment #12.
