Skip to content

Conversation

@Evemose
Copy link
Contributor

@Evemose Evemose commented Jun 21, 2024

Please review this trivial enhancement to informativity of error emiited when switch expression does not yield value in any branch lacks reasoning.

Issue body:
Currently, when switch expression does not yield any value, compiler emits following error: "switch expression does not have any result expressions". While this gives information on how to solve the issue, the exact reason of this error stays unclear, expecially because switch statements do not have same requirement. Recently, there even were question about this in one of jdk mailing lists.

The source of issue becomes obvious if we look where this error is reported: it is emmited in Attr class when javac is unable to assign type to switch expression. Therefore, I propose to change the emitted error text to following: "Unable to determine switch expression type: no branch that yields value found".

If anyone have ideas on how to furthernore improve this error message, please feel free to share.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8334722: Error emitted when switch expression does not yield any value lacks reasoning (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19837/head:pull/19837
$ git checkout pull/19837

Update a local copy of the PR:
$ git checkout pull/19837
$ git pull https://git.openjdk.org/jdk.git pull/19837/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19837

View PR using the GUI difftool:
$ git pr show -t 19837

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19837.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 21, 2024

👋 Welcome back Evemose! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Jun 21, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 21, 2024
@openjdk
Copy link

openjdk bot commented Jun 21, 2024

@Evemose The following label will be automatically applied to this pull request:

  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the compiler compiler-dev@openjdk.org label Jun 21, 2024
@mlbridge
Copy link

mlbridge bot commented Jun 21, 2024

Webrevs

@lahodaj
Copy link
Contributor

lahodaj commented Jun 24, 2024

Sorry, but I am not quite sure what's the problem with the (existing) error, and if the new one is better.

There are two reasons for that:
a) the specification:
https://docs.oracle.com/javase/specs/jls/se22/html/jls-15.html#jls-15.28.1
literally says:

It is a compile-time error if a switch expression has no result expressions.

It does not say it is a compile-time error is that the switch expression's type cannot be determined (see below), but that it is an error if there are not result expression. And while trouble determining the expressions type are likely a part of the motivation for this assertion, it is not the only one, I think.

b) for code like:

    private void c() {
        test(switch (0) {
            default -> 0;
        });
    }
    private void test(int i) {}

the type of the switch expression is not determined by the result expressions, but by the target type. I.e. in this case, the type of the expression is int, regardless whether there is any yield returning int or not. It would, in principle, be possible to permit no result expression in cases like this, I think. It is unclear what would be the usefulness of that, though.

@Evemose
Copy link
Contributor Author

Evemose commented Jun 24, 2024

Sorry, but I am not quite sure what's the problem with the (existing) error, and if the new one is better.

There are two reasons for that:
a) the specification:
https://docs.oracle.com/javase/specs/jls/se22/html/jls-15.html#jls-15.28.1
literally says:

It is a compile-time error if a switch expression has no result expressions.

It does not say it is a compile-time error is that the switch expression's type cannot be determined (see below), but that it is an error if there are not result expression. And while trouble determining the expressions type are likely a part of the motivation for this assertion, it is not the only one, I think.

b) for code like:

    private void c() {
        test(switch (0) {
            default -> 0;
        });
    }
    private void test(int i) {}

the type of the switch expression is not determined by the result expressions, but by the target type. I.e. in this case, the type of the expression is int, regardless whether there is any yield returning int or not. It would, in principle, be possible to permit no result expression in cases like this, I think. It is unclear what would be the usefulness of that, though.

Firstly, I would like to note that I don't view this as an issue, that's why I filed issue as enhancement, not a bug.

Now with this said, I guess it would be appropriate to identify problem more formally. I would formulate it as something as following: "Switch expression have requirement to contain at least one branch that results in value yielded at least in one of possible execution execution scenarios, unlike the switch statements. The previous error, while provides explanation on how to fix this compilation error, does not explain why it is reported. It is confusing due to inconsistency between switch expression and statement".

This issue is an enhancement and not a bug, because compiler does not have to "explain itself" if it at least explains a way to fix it. However, noone was ever hurt by more robust error reporting.

Now to some statements in my problem description. Why I say it is confusing instead of it could be confusing. Well, because this confusion got even to jdk mailing lists. Also, I don't think personal experience is a good argument to provide, but I also encountered question regarding this error quite a bunch of times when I introduced someone to Java.

As for new error message itself, I couldn't be 100% sure, but when this question got to amber mailing list, I spent some time looking through javac codebase and the only place where this error is reported is Attr class, which is responsible for attribution (which includes type determination). That's why I say that inability to attribute type to switch expression is the effective cause of this error. I'm not sure if specification designers put more meaning in this than I am, but the other thoughts, at least, didn't make it to javac codebase (of course if I haven't missed some other place where this issue is reported, although I doubt it).

To yield part: im not sure if it is strictly correct, but I always thought of non-void one-expression lambdas as ones that imolecitly return the value. Moreover, if you look into decompiled sources, generated return statements is exactly what you will see. That's why I also have always thought that whether yield keyword is specified explicitly or not, value is always yielded.

That's my reasoning behind it. If you find it insufficient or find improvement not significant enough to be integrated, I could close the issue, no problem in it, I don't think lost results of those enormous efforts to change one line in errors file are too significant to be upset about after all lol.

@lahodaj
Copy link
Contributor

lahodaj commented Jun 24, 2024

[snip]

Now with this said, I guess it would be appropriate to identify problem more formally. I would formulate it as something as following: "Switch expression have requirement to contain at least one branch that results in value yielded at least in one of possible execution execution scenarios, unlike the switch statements. The previous error, while provides explanation on how to fix this compilation error, does not explain why it is reported. It is confusing due to inconsistency between switch expression and statement".

Technically speaking, the immediate reason for this error is the assertion in the JLS. And while saying "the error is reported because the type of the switch expression cannot be determined" may be a convenient informal explanation (and it was, I think, part of the consideration why the switch expressions without any result expressions are illegal), it is not technically correct. Please see below.

[snip]

To yield part: im not sure if it is strictly correct, but I always thought of non-void one-expression lambdas as ones that imolecitly return the value. Moreover, if you look into decompiled sources, generated return statements is exactly what you will see. That's why I also have always thought that whether yield keyword is specified explicitly or not, value is always yielded.

I probably didn't use the best example. Let me try with a new one:

    private void c() {
        test(switch (0) {
            default -> { yield ""; }
        });
    }

    private void test(CharSequence cs) {}

The type of the switch expression here is not determined by the yield statement at all. The switch expression is a poly expression here, and its type is determined by the target type, which is java.lang.CharSequence here. The expressions inside the yield statements are attributed against the target type, but whether or not there are any yield statements (or expressions that are expanded to yield statements) is not important for determining the type of the expression.

In other words, there are (I think) multiple reasons why code like this is illegal:

    private void c() {
        test(switch (0) {
            default -> throw new RuntimeException();
        });
    }

    private void test(CharSequence cs) {}

but inability to determine the type of the switch expression is not one of them. The type of this switch expression would still be java.lang.CharSequence.

The types of the expressions inside the yield statements are AFAIK only important for determining the type of the switch expression if the switch expression is standalone. Like:

        (switch (0) {
            default -> { yield ""; }
        }).length();

@Evemose
Copy link
Contributor Author

Evemose commented Jun 24, 2024

The types of the expressions inside the yield statements are AFAIK only important for determining the type of the switch expression if the switch expression is standalone.

Thats actually true! I didnt know about this detail honestly. Concidering this info, this new message will, in fact, be missleading. Therefore, I am closing this PR.

@Evemose Evemose closed this Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler compiler-dev@openjdk.org rfr Pull request is ready for review

Development

Successfully merging this pull request may close these issues.

2 participants