Skip to content
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

[JEP 488][Preview][Primitive Patterns] ECJ crashes compiling switch over void typed selector expression #3369

Closed
srikanth-sankaran opened this issue Nov 29, 2024 · 17 comments
Assignees

Comments

@srikanth-sankaran
Copy link
Contributor

Found by code inspection and white box testing:

ECJ crashes compiling the following code; javac complains error: the switch statement does not cover all possible input values

public class X {
	public static void main(String[] args) {
		switch (main(null)) {
		
		}
	}
}

Stack trace on master (problem can be seen from 4.34 M2)

Caused by: java.util.EmptyStackException
	at java.base/java.util.Stack.peek(Stack.java:103)
	at org.eclipse.jdt.internal.compiler.codegen.OperandStack.peek(OperandStack.java:123)
	at org.eclipse.jdt.internal.compiler.codegen.CodeStream.dup(CodeStream.java:1082)
	at org.eclipse.jdt.internal.compiler.ast.SwitchStatement.generateCodeSwitchPatternPrologue(SwitchStatement.java:850)
	at org.eclipse.jdt.internal.compiler.ast.SwitchStatement.generateCode(SwitchStatement.java:708)
	at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.generateCode(AbstractMethodDeclaration.java:395)
	at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.generateCode(AbstractMethodDeclaration.java:331)
	at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:773)
	at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:843)
	at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.generateCode(CompilationUnitDeclaration.java:403)
	at org.eclipse.jdt.internal.compiler.Compiler.resolve(Compiler.java:1119)
	at org.eclipse.jdt.internal.compiler.Compiler.resolve(Compiler.java:1155)
	at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:292)
@srikanth-sankaran
Copy link
Contributor Author

srikanth-sankaran commented Nov 29, 2024

From a cursory reading of JEP488 and Draft JLS changes I haven't been able to ascertain the right behavior here:

14.11 has been modified to say The type of the selector expression may be any type. and then if it is void what ? I haven't been able to determine.

@srikanth-sankaran
Copy link
Contributor Author

Not sure this should compile, but does with both javac and ECJ:

public class X {
	public static void main(String[] args) {
		switch (null) {
			case null -> System.out.println("Null");
			default-> System.out.println("Default");
		}
	}
}

Not sure this should not (two nots there) compile, but doesn't with both ECJ and javac: Both complain about missing default

public class X {
	public static void main(String[] args) {
		switch (null) {
			case null -> System.out.println("Null");
			// default-> System.out.println("Default");
		}
	}
}

@stephan-herrmann
Copy link
Contributor

javac complains error: the switch statement does not cover all possible input values

nice try :)

From a cursory reading of JEP488 and Draft JLS changes I haven't been able to ascertain the right behavior here:

14.11 has been modified to say The type of the selector expression may be any type. and then if it is void what ? I haven't been able to determine.

void is not a type, is it? (cf. https://docs.oracle.com/javase/specs/jls/se23/html/jls-8.html#jls-8.4.5)

I guess we should complain something like "value needed", i.e., IProblem.InvalidVoidExpression?

@jukzi
Copy link
Contributor

jukzi commented Jan 20, 2025

i got a EmptyStackException in
https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-3573/2/consoleText
is that related?

@srikanth-sankaran
Copy link
Contributor Author

i got a EmptyStackException in https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-3573/2/consoleText is that related?

In that console log, I don't see a stack trace, just one instance of the text EmptySta - With that it is not possible to conclude much. Any pop()/peek() on any stack is a candidate. I see 68 calls to java.util.Stack.pop() in my workspace (only two in batch compiler)

Are you able to get any more information ??

@srikanth-sankaran
Copy link
Contributor Author

You are not going to find any code that switches over void type in JDT any case :)

@jukzi
Copy link
Contributor

jukzi commented Jan 20, 2025

Don't know how to get the stacktrace. i could reproduce it with https://github.com/eclipse-jdt/eclipse.jdt.core/pull/3578/files :

import java.util.Arrays;

public final record CharCharArray(char[][] key) implements Comparable<CharCharArray> {

	public CharCharArray(char[][] key) {
		this.key = key;
	}

	@Override
	public int compareTo(CharCharArray other) {
		int length = Math.min(this.key.length, other.key.length);
		for (int i = 0; i < length; i++) {
			int c = Arrays.compare(this.key[i], other.key[i]);
			if (c != 0) {
				return c;
			}
		}
		return this.key.length - other.key.length;
	}

	public char[][] getKey() {
		return this.key;
	}

	@Override
	public boolean equals(Object obj) {
		if (obj instanceof CharCharArray other) {
			return Arrays.deepEquals(this.key, other.key);
		}
		return false;
	}

	@Override
	public int hashCode() {
		return Arrays.deepHashCode(this.key);
	}

	/**
	 * @return <code>Arrays.toString</code> of the underlying array
	 */
	@Override
	public String toString() {
		return Arrays.deepToString(this.key);
	}
}

@jukzi
Copy link
Contributor

jukzi commented Jan 20, 2025

could generate the stack, but seems unrelated:

00:03:02.975  Caused by: java.util.EmptyStackException
00:03:02.975      at java.util.Stack.peek (Stack.java:103)
00:03:02.975      at org.eclipse.pde.api.tools.internal.builder.Validator.getItem (Validator.java:138)
00:03:02.975      at org.eclipse.pde.api.tools.internal.builder.TagValidator.processMethodNode (TagValidator.java:317)
00:03:02.975      at org.eclipse.pde.api.tools.internal.builder.TagValidator.validateTags (TagValidator.java:186)
00:03:02.975      at org.eclipse.pde.api.tools.internal.builder.TagValidator.visit (TagValidator.java:118)
00:03:02.975      at org.eclipse.jdt.core.dom.Javadoc.accept0 (Javadoc.java:229)

@stephan-herrmann
Copy link
Contributor

stephan-herrmann commented Jan 28, 2025

Not sure this should compile, but does with both javac and ECJ:

public class X {
	public static void main(String[] args) {
		switch (null) {
			case null -> System.out.println("Null");
			default-> System.out.println("Default");
		}
	}
}

The first part of this is captured in "5.7 Testing Contexts" by:

If the expression has the null type, then the expression may be converted to any reference type.

For the next part "14.1.1 Switch Blocks" says:

The switch block of a switch statement or a switch expression is switch compatible with the type of the selector expression, T, if all of the following are true:

  • If a null literal is associated with the switch block, then T is a reference type.

In "4.1. The Kinds of Types and Values" the null type is introduced with no sign of it being a reference type.

Hence I agree that the example lacks backing by JLS.

Not sure this should not (two nots there) compile, but doesn't with both ECJ and javac: Both complain about missing default

public class X {
	public static void main(String[] args) {
		switch (null) {
			case null -> System.out.println("Null");
			// default-> System.out.println("Default");
		}
	}
}

I share your doubts.

Both points asked here: https://mail.openjdk.org/pipermail/compiler-dev/2025-January/029306.html

=> Answer is here: https://bugs.openjdk.org/browse/JDK-8348901
=> Corresponding test introduced via #3650

@stephan-herrmann stephan-herrmann added this to the BETA_JAVA24 RC1 milestone Feb 9, 2025
@stephan-herrmann
Copy link
Contributor

After that excursion about the null type, let's not forget that this issue is about void, not null.

stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Feb 11, 2025
void typed selector expression

+ report error rather than crashing

Fixes eclipse-jdt#3369
@stephan-herrmann
Copy link
Contributor

@srikanth-sankaran test failures pointed me to #2385 where you added tests expecting an error like

Cannot switch on a value of type void. ...

I admit I'm a bit terribly at a loss, what to make of void. I'd argue that there is no such thing as "a value of type void", since void means "no value".

Next I see that compilers consistently comment this ...

void foo() { }
Object o = foo();

... saying cannot convert from void to Object.

So I searched a bit more whether or not JLS considers void a type.

  • 8.4.5 says a method either has a return type or it is void
  • 15.1. an expression evaluates either to a variable, or to a value or to Nothing (the expression is said to be void) => void and value are disjoint concepts
    • The explanation goes on "An expression denotes nothing if and only if it is a method invocation (§15.12) that invokes a method that does not return a value, that is, a method declared void"
    • => methods can be void, not values or types
  • 15.8.2. cannot quite decide, speaking of "the pseudo-type void".
  • in 15.12.2.5. they introduce RT "let RT be the return type of MTT" and then list one case as "RT is void". => void is a return type?

So clearly JLS isn't clear whether void is a type or not.

Therefor we seem to be free to choose our error messages following one interpretation or another.

The error I opted for is from

void foo() {}
...
synchronized(foo()) { }

=> Expression must return a value

But even existing ecj is miserably inconsistent:

public class VoiDTest {
	void bar() {
		synchronized(bar()) {
			Object o = bar();
			assert bar() : bar();
		}
	}
}

gives:

----------
1. ERROR in /tmp/VoidTest.java (at line 3)
        synchronized(bar()) {
                     ^^^^^
Expression must return a value
----------
2. ERROR in /tmp/VoidTest.java (at line 4)
        Object o = bar();
                   ^^^^^
Type mismatch: cannot convert from void to Object
----------
3. ERROR in /tmp/VoidTest.java (at line 5)
        assert bar() : bar();
               ^^^^^
Type mismatch: cannot convert from void to boolean
----------
4. ERROR in /tmp/VoidTest.java (at line 5)
        assert bar() : bar();
                       ^^^^^
Expression must return a value
----------

javac is similarly (but differently) creative:

VoidTest.java:3: error: illegal start of type
                synchronized(bar()) {
                ^
VoidTest.java:4: error: incompatible types: void cannot be converted to Object
                        Object o = bar();
                                      ^
VoidTest.java:5: error: incompatible types: void cannot be converted to boolean
                        assert bar() : bar();
                                  ^
VoidTest.java:5: error: 'void' type not allowed here
                        assert bar() : bar();
                                          ^

Do you see the error at line 3? 👀

=> Do you see some system behind this madness, which we should try to adhere to, or is it up to personal taste of the one who introduces a new error detection?

stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Feb 11, 2025
void typed selector expression

+ advoid secondary error
+ tentatively adjust tests from eclipse-jdt#2385

Fixes eclipse-jdt#3369
@stephan-herrmann
Copy link
Contributor

See 252566c for tentative adjustment of tests from #2385. WDYT?

@stephan-herrmann
Copy link
Contributor

See 252566c for tentative adjustment of tests from #2385. WDYT?

@srikanth-sankaran do you need more time to reply / review?
Should I merge #2385 to BETA_JAVA24 and leave a more general decision for later?

@srikanth-sankaran
Copy link
Contributor Author

See 252566c for tentative adjustment of tests from #2385. WDYT?

@srikanth-sankaran do you need more time to reply / review? Should I merge #2385 to BETA_JAVA24 and leave a more general decision for later?

I will complete the review before end of business day today ...

@srikanth-sankaran
Copy link
Contributor Author

=> Do you see some system behind this madness, which we should try to adhere to, or is it up to personal taste of the one who introduces a new error detection?

I am OK with different verbiage in different circumstances - we don't have to bend over backwards to unify into a consistent single diagnostic - as long as each is clear. I have left a comment elsewhere suggesting we say "Expression has no value" rather than "Expression must return a value" - A void method cannot return a value, so "has no value" is better I feel

@srikanth-sankaran
Copy link
Contributor Author

Do you see the error at line 3? 👀

Strange indeed. This usually is indicative of a parse error - although I don't see why it should happen in this context

stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Feb 14, 2025
void typed selector expression

+ rephrase error message
+ add tests for pre-existing cases of that error

Fixes eclipse-jdt#3369
stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Feb 15, 2025
void typed selector expression

+ report error rather than crashing

Fixes eclipse-jdt#3369
stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Feb 15, 2025
void typed selector expression

+ advoid secondary error
+ tentatively adjust tests from eclipse-jdt#2385

Fixes eclipse-jdt#3369
stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Feb 15, 2025
void typed selector expression

+ rephrase error message
+ add tests for pre-existing cases of that error

Fixes eclipse-jdt#3369
stephan-herrmann added a commit that referenced this issue Feb 15, 2025
…ver void typed selector expression (#3722)

+ report error rather than crashing
+ avoid secondary error
+ rephrase pre-existing error message
+ add tests for pre-existing cases of that error
+ adjust tests from #2385

Fixes #3369
@stephan-herrmann
Copy link
Contributor

Fixed by #3722.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants