Issue 1720 - Switch on non-constant expression needs better error message - haxe

[issuesbot]

[Google Issue #1720 : https://code.google.com/p/haxe/issues/detail?id=1720] by mwe...@gmail.com, at 19/04/2013, 20:13:29 If a user tries to use a variable or other non-constant expression as a case for a switch statement, they will receive warnings and errors such as:

This pattern is unused Warning : This variable is unused Unrecognized_pattern

This is confusing behavior when coming from languages such as AS3, where such non-constant switches are allowed. For example:

var a = 3; var b = 2;

switch(1) {
    // Pattern shadows outer variable.
    // Unexpected behavior, unhelpful "Variable is unused" warning
    case a: trace("a"); 
    // Unhelpful Unrecognized_pattern error
    case b+1: trace("b");   // Sh
    }

The compiler should detect these cases--in particular, when a case expression is a single variable that shadows an outer variable--and provide a better error/warning message describing the behavior.

For example:

Case expressions must be constant. Please use an if...else statement or guards instead.

Further discussion: https://groups.google.com/forum/#!topic/haxelang/VE_eji0WmBc

[issuesbot]

[comment from mwe...@gmail.com, published at 19/04/2013, 20:14:32] Sorry, a more accurate description is "Non-constant case statements need better error message"

[issuesbot]

[comment from mwe...@gmail.com, published at 19/04/2013, 20:26:30] I guess "non-constant" isn't the right description, since patterns aren't really constant by nature? Maybe "Patterns cannot use outer variables."

[issuesbot]

[comment from si...@haxe.org, published at 19/04/2013, 21:14:48] We could maybe disallow toplevel naming conflicts, but I'm not in favor of it. Expressions like "case b+1" were never defined to work in haxe, so I don't think we have to worry about them.

I also don't see how we can reliably detect simple cases, given that "case a" is a valid expression. I will give this some thought, but I'm pretty sure it's better to get this out of the way right away instead of dragging some detection mechanism through future compiler versions.

[issuesbot]

[comment from back2dos@gmail.com, published at 19/04/2013, 23:46:31] I think saying "it was never defined to work in haxe" is a bit of an academic argument. While I am very appreciative of the fact that Haxe is getting increasingly well-defined, there was always (an in some sense still is) the design principle that it should be relatively close to ECMAScript to ease transition for newcomers. And in fact that is still a claim that can be found on haxe.org:

Familiar syntax: If you are familiar with Java, PHP, JavaScript or ActionScript, the Haxe syntax should be very familiar, allowing you to get down to coding right away with a minimal learning curve.

Reading that, it's not unrealistic to expect that the example code presented above will compile. And I am in no way suggesting that it should, but I believe we need to find a way to either make it very clear upfront that it won't (for example by revising the above statement - if only by amending a link to a cheatsheet summarizing major differences), or to better explain the problem in error messages as proposed here. Because people do keep running into this problem repeatedly.

My guess is that it would be a viable solution to treat warnings/errors specially, if the case expression has no guard and would be valid expression (i.e. causing no type errors) outside the switch statement. It's a pretty safe bet that the user is trying to perform what we might call a "classical" switch and adding a hint to the error message can hardly hurt. Also, error messages constitute a behavior that is really not covered by the spec. So while adding this hint for now meets an existent need for clarification, getting rid of it later if needed is always an option, because it doesn't actually break anything.

[issuesbot]

[comment from si...@haxe.org, published at 20/04/2013, 00:26:21] I guess we could scan for a singular identifier case which is unguarded and not the last one, but even that won't cover all the cases.

I'm not willing to mess up the pattern compilation trying to extract all possible previously valid use-cases and infer the usage intent from that, so we have to draw the line somewhere.

[issuesbot]

[comment from mwe...@gmail.com, published at 20/04/2013, 05:28:13] Trying to cover a few common cases should really improve usability -- hopefully it doesn't involve turning the pattern matching code inside out.

My thoughts:

case b+1: // "Unrecognized_pattern". Already an error, so just replace with descriptive error message if it applies to all situations like this. (case func(): etc)

var x; var y;

switch(foo) {
    case x: // Possible Variable is unused warning
    case y: // Pattern never used error
    case 3: // Pattern never used error
}

Single identifier followed by more single identifiers. User is probably expecting different behavior. Give a descriptive warning if possible.

var x; var y;

switch(foo) {
    // .. other cases
    case x: // May have "Variable is unused".
}

This kind of shadowing is often valid and common (macros repeatedly matching with e, for example). There's probably not much that can done in this case. I'm not really in favor of disallowing top-level naming conflicts, either.

[issuesbot]

[comment from back2dos@gmail.com, published at 20/04/2013, 09:55:06] @ Simon: I'm not at all suggesting anything that would involve scanning the AST ahead of time or otherwise interfere with the actual implementation. What I am proposing is that when you discover and error or warning, you type the case expression in the outer context and if that doesn't fail, you amend the error message by some generic hint like "consider using a guard condition instead" (just checked that googling "haxe guard condition" will lead you to the relevant documentation).

[issuesbot]

[comment from si...@haxe.org, published at 28/04/2013, 10:20:50] But which one is "the case expression"? In the examples here, the errors on the n-th patterns are caused by the n-1th case expression being misinterpreted as capture variable, so this already requires contextual information and a scan over all case expressions.

In that light, we may as well scan all case-expressions in a separate pass and detect non-final non-guarded identifiers. That's also easier to implement.

[issuesbot]

[comment from ncanna...@gmail.com, published at 05/05/2013, 05:19:30] I agree we should give something better.

One possibility would be to type the pattern-expression and see if it compiles. If it does and if it's not an enum value, then it's most likely not a pattern but the user tried entering an actual expression. We can then tell him something like "Case expression must be a constant value or a pattern, not an arbitrary expression"

[issuesbot]

[comment from si...@haxe.org, published at 05/05/2013, 08:25:25] That still doesn't quite work for all cases:

var a = 2;

switch(2) {
    case a:
    case 9:
}

You would get the unused pattern error on "case 9", but the problem is the previous "case a" pattern. This is why I think we need the context, and thus an additional pass.

[issuesbot]

[comment from ncanna...@gmail.com, published at 05/05/2013, 09:04:09] Ok, what about a two passes then : first pass we do as usual, we store but don't error coverage issues.

Second pass we select the appropriate case and error to display based on coverage failures.

[issuesbot]

[comment from si...@haxe.org, published at 05/05/2013, 10:16:25] This issue was closed by revision 0940956f8360bac621fe356a64b7104e1e7eafa9.

[issuesbot]

[comment from si...@haxe.org, published at 05/05/2013, 20:44:50] The fix is still not ideal because now we get strange errors in some cases:

class Main {
    static public function main() {
        switch(true) {
            case true:
            case false: // Case expression must be a constant value or a pattern, not an arbitrary expression 
            case true:
        }
    }
}

"false" can naturally be typed on its own, so the approach fails here.

[issuesbot]

[comment from si...@haxe.org, published at 12/05/2013, 09:36:52] This issue was closed by revision 460763cd2b999f4454f5184556b61bb55f62424c.

[issuesbot]

[comment from si...@haxe.org, published at 12/05/2013, 09:38:20] It reports a special error if a previous pattern is an identifier other than true, false, null which can be typed in the current context. That should cover 99% of the problem cases.