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

[Patterns] Unconditional pattern in instanceof shouldn't compile at compliance level of 17 #3478

Open
fedejeanne opened this issue Dec 18, 2024 · 11 comments
Assignees
Milestone

Comments

@fedejeanne
Copy link
Contributor

fedejeanne commented Dec 18, 2024

I found this interesting case when using pattern matching for instanceof: https://stackoverflow.com/questions/74978665/pattern-matching-for-instanceof-throwing-a-confusing-error-expression-type-trip

This code should cause a compile error when compiling it with a compliance level of 17:

public class Repro {
	public static void main(String[] args) {
		String s = null;
		if (s instanceof String string) { }
	}
}

... but it doesn't, at least not in Eclipse.

If I compile this with javac 17 then I get:

.\Repro.java:17: error: expression type String is a subtype of pattern type String
                if (s instanceof String string) {
                      ^
1 error

It's the same output with both JDKs:

  • corretto-jdk17.0.5.8.1_x86_64
  • temurin-jdk-17.0.13+11

This answer quotes the section 15.20.2 of the JLS (version 19), which says:

image

Version 17 of the same JLS (jls-15.20.2) says the same:

https://docs.oracle.com/javase/specs/jls/se17/html/jls-15.html#jls-15.20.2

Environment

  • OS: Windows 10
  • JDK and javac: corretto-jdk17.0.5.8.1_x86_64
  • JDK and javac: temurin-jdk-17.0.13+11
  • Compiler compliance level (Eclipse project)l: 17

Settings

  • Compliance level: 17
    image

  • Reproducible with these 2 JDKs
    image


image

@srikanth-sankaran srikanth-sankaran self-assigned this Dec 19, 2024
@srikanth-sankaran
Copy link
Contributor

srikanth-sankaran commented Dec 19, 2024

  1. Please always provide full standalone snippets - not just fragments.
  2. You don't mention which version of javac you tried.
  3. You are looking at JLS 19 - Always check the latest JLS
  4. Searching for the text "If the type of the RelationalExpression is a subtype of the type of the Pattern, then a compile-time error occurs" in the github issues reporting page pulls up instanceof pattern matching should reject supertypes #1356 😄

@srikanth-sankaran srikanth-sankaran closed this as not planned Won't fix, can't repro, duplicate, stale Dec 19, 2024
@fedejeanne
Copy link
Contributor Author

  1. Please always provide full standalone snippets - not just fragments.

👍 I adapted the description

2. You don't mention which version of javac you tried.

I see the error with corretto-jdk17.0.5.8.1_x86_64 and with temurin-jdk-17.0.13+11 so it's javac 17. I added this to the description.

3. You are looking at JLS 19 - Always check the latest JLS

Thank you for the pointer. Since I am interested in a fix for Java compliance level of 17, this is a relevant point so I added it to the description too.

4. Searching for the text "If the type of the RelationalExpression is a subtype of the type of the Pattern, then a compile-time error occurs" in the github issues reporting page pulls up [instanceof pattern matching should reject supertypes #1356](https://github.com/eclipse-jdt/eclipse.jdt.core/issues/1356) 😄

I looked for the wrong keywords then, thank you for the pointer. Since that issue doesn't mention the JDK/javac version either and it doesn't say if this is still an issue then I would like to reopen this issue please. I'm interested in a fix for ECJ when compiling with compliance level 17. Would that be possible?

@fedejeanne fedejeanne changed the title Pattern Matching for instanceof shouldn't compile Pattern Matching for instanceof shouldn't compile for compliance level of 17 Dec 19, 2024
@iloveeclipse
Copy link
Member

#1356 (comment) says it all.
Javac had an issue, and latest JLS is fixed, so ecj behavior matches JLS.
If you want that javac in Java 17 also accepts this code, please ask Temurin/Oracle for a fix.
As an alternative solution you can compile with latest javac.

@srikanth-sankaran
Copy link
Contributor

I looked for the wrong keywords then, thank you for the pointer. Since that issue doesn't mention the JDK/javac version either and it doesn't say if this is still an issue then I would like to reopen this issue please. I'm interested in a fix for ECJ when compiling with compliance level 17. Would that be possible?

It is extremely unlikely we would want to support that behavior since it is superseded and/but more importantly it is a case of rejecting something that was deemed invalid at some distant point in time in the past. If it was a case of accepting some code that was legal at one point and has ceased to be legal then it makes a strong case for implementing behavior specific to JDK17. In the present instant, there is no strong case to support historic behavior.

Who would benefit from implementing that behavior ? ie, what code that doesn't compile on master would start compiling fine ?

@fedejeanne
Copy link
Contributor Author

I am not interested in letting the code compile with older versions of javac from Temurin/Oracle or other vendors, I'm interested in not letting it compile in Eclipse so that the developer sees that the code breaks before reaching the CI pipeline.

Our CI pipeline compiles the nightly builds with javac and since there is a mismatch between ECJ and the JLS up to the compliance level of 19 (like @srikanth-sankaran pointed out #3478 (comment)) then this as an error in ECJ.

ECJ should comply with the JLS when specifically setting the desired compliance level. Or am I seeing this wrong? If this is a matter of time and effort, I can relate, but are you saying that this isn't an error?

@srikanth-sankaran
Copy link
Contributor

I am not interested in letting the code compile with older versions of javac from Temurin/Oracle or other vendors, I'm interested in not letting it compile in Eclipse so that the developer sees that the code breaks before reaching the CI pipeline.

Our CI pipeline compiles the nightly builds with javac and since there is a mismatch between ECJ and the JLS up to the compliance level of 19 (like @srikanth-sankaran pointed out #3478 (comment)) then this as an error in ECJ.

ECJ should comply with the JLS when specifically setting the desired compliance level. Or am I seeing this wrong? If this is a matter of time and effort, I can relate, but are you saying that this isn't an error?

In general, you are right, when compiling to a certain JDK level, ECJ should match the JLS at that level.

In this specific case, you have given an answer to my question "Who would benefit from implementing that behavior ?" but the question now shifts to "How likely is someone to write code like String s = null; if (s instanceof String string) { }

OIOW, are we dealing with a real problem here ?

@srikanth-sankaran
Copy link
Contributor

I'll frame it otherwise, did you hit this problem in your org with developer submitting code that compiles with ECJ but fails when reaching the CI pipeline? Or was it (ONLY) from perusing stack overflow discussion ?

@fedejeanne
Copy link
Contributor Author

The developer that commits code such as that one would benefit since he/she doesn't have to wait until the next day to see if the code is broken and if he/she completely stopped the pipeline.

Our developers use such code constructs as null-checks because it is readable and concise, real developers developing an expensive product and pushing commits even on Fridays. Imagine the frustration when coming in on a Monday and seeing that you broke the whole pipeline because such a small issue.

It is likely to happen, less likely since we now know that this is not supported in Eclipse but in a team of 90+ developers, this is bound to happen again sooner or later.

@srikanth-sankaran
Copy link
Contributor

srikanth-sankaran commented Dec 19, 2024

Thanks for the explanation @fedejeanne and thanks for insisting :)

If it causes/caused grief in the real world to any degree, no question that a fix is called for.

I will reopen this but will schedule it for 3.45 M2 - even though the fix is small, difficult to schedule it right away with the holidays looming.

@srikanth-sankaran srikanth-sankaran added this to the 4.35 M2 milestone Dec 19, 2024
@srikanth-sankaran srikanth-sankaran changed the title Pattern Matching for instanceof shouldn't compile for compliance level of 17 [Patterns] Unconditional pattern in instanceof shouldn't compile for compliance level of 17 Dec 19, 2024
@srikanth-sankaran srikanth-sankaran changed the title [Patterns] Unconditional pattern in instanceof shouldn't compile for compliance level of 17 [Patterns] Unconditional pattern in instanceof shouldn't compile at compliance level of 17 Dec 19, 2024
@fedejeanne
Copy link
Contributor Author

Thank you very much, I appreciate it :-)

Let me know if I can assist with something else (infos, testing, etc).

I wish you both happy holidays and a good start in the next year! 🎄 🎆

@srikanth-sankaran
Copy link
Contributor

I wish you both happy holidays and a good start in the next year! 🎄 🎆

Wishes to you too!

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