cemerick said: It would be nice if case were further enhanced to support references to enums and static final fields (and maybe other stably-referenceable values as well?). A userland workaround is shown here (added here instead of in a separate ticket at Rich's request).

Assembla Importer
added a comment - 29/Sep/10 9:30 AM cemerick said: It would be nice if case were further enhanced to support references to enums and static final fields (and maybe other stably-referenceable values as well?). A userland workaround is shown here (added here instead of in a separate ticket at Rich's request).

Per cemerick's request I'm providing a secondary patch which would provide for evaluating test-constant symbols at compile time. This would allow us to use switches where writing a literal value is difficult. E.g.:

Alexander Taggart
added a comment - 02/Mar/11 7:55 PM Per cemerick's request I'm providing a secondary patch which would provide for evaluating test-constant symbols at compile time. This would allow us to use switches where writing a literal value is difficult. E.g.:

Stuart Halloway
added a comment - 04/Mar/11 2:54 PM Rich: screened in this case means only that the tests pass. I am continuing to dig into this patch, adding comments as I go. Let me know if there are specific things I should be investigating.

Rich: to the extent that we believe in testing for things like reflection warnings, it would be nice to have a way to get to that information other than be scraping stderr. If this is of interest let me know and I will make a ticket and/or start a design discussion as appropriate.

Stuart Halloway
added a comment - 04/Mar/11 3:12 PM Rich: to the extent that we believe in testing for things like reflection warnings, it would be nice to have a way to get to that information other than be scraping stderr. If this is of interest let me know and I will make a ticket and/or start a design discussion as appropriate.

The primary patch treats symbols which do not resolve to enums as symbols, but also treats enums as symbols when the set of test-constants was not all-enum. Both parts were an attempt at backwards compatibility.

I agree that the better behaviour would be to error on the mixed case, though this would break any code where a test-constant symbol happens to resolve to an enum.

Regarding eval:

It doesn't get around the "compile-time" restriction, but rather the "literal" restriction.

Alexander Taggart
added a comment - 04/Mar/11 4:45 PM Regarding the mixed enum/non-enum case:
The primary patch treats symbols which do not resolve to enums as symbols, but also treats enums as symbols when the set of test-constants was not all-enum. Both parts were an attempt at backwards compatibility.
I agree that the better behaviour would be to error on the mixed case, though this would break any code where a test-constant symbol happens to resolve to an enum.
Regarding eval:
It doesn't get around the "compile-time" restriction, but rather the "literal" restriction.

Rich Hickey
added a comment - 04/Mar/11 5:03 PM Unless someone can provide some simple semantics for enum support, I'd rather not support them.
Re: lookupswitch - I don't see any reason to support it - is the tableswitch code in some way wrong?

As the updated doc states: "Java enums are supported if all test constants are enums of the same type."
Stu's example violates the above rule. The issue then becomes what should be the response: error at compile-time, or treat as symbol. More on this in a following comment.

Re: lookupswitch ... is the tableswitch code in some way wrong?

Not at all, but the current version will throw an exception when it can't find a sufficient shift-mask:

Support for enums is really just a degenerate case of supporting named constant values, e.g., java.lang.Math/PI, (def init-state 0).

The basic requirement of case is that the test constants have a known value at compile-time, and that value has consistent hashing. The current implementation assumes the latter requirement will be met by restricting test constants to literal types. This mostly works, but fails with things like regex patterns.

The above basic requirements can be met while removing the incidental "literal" restriction. To do so means resolving literal symbols into constant values at compile-time, and then validating that the types are suitable (e.g., number, string).

The simplest and most consistent approach would be to require quoting test constant symbols when used as symbols. Setting aside all other discussion of enums, etc., is this change to the treatment of symbols acceptable?

Alexander Taggart
added a comment - 04/Mar/11 9:50 PM Support for enums is really just a degenerate case of supporting named constant values, e.g., java.lang.Math/PI, (def init-state 0).
The basic requirement of case is that the test constants have a known value at compile-time, and that value has consistent hashing. The current implementation assumes the latter requirement will be met by restricting test constants to literal types. This mostly works, but fails with things like regex patterns.
The above basic requirements can be met while removing the incidental "literal" restriction. To do so means resolving literal symbols into constant values at compile-time, and then validating that the types are suitable (e.g., number, string).
The simplest and most consistent approach would be to require quoting test constant symbols when used as symbols. Setting aside all other discussion of enums, etc., is this change to the treatment of symbols acceptable?

doc change: "The test-constants can be any combination of numbers, chars, strings, keywords, quoted symbols, and (Clojure) composites thereof. Unquoted symbols will be evaluated at compile time, and must resolve to one of the preceding types."

426-supports-enums-and-named-values.patch:

all from above, plus...

conditionally supports java enums

doc change: "Java enums are supported only if all test constants are enums of the same type."

Alexander Taggart
added a comment - 04/Mar/11 10:39 PM To make life easier, I'm altering the set of patches.
426-basic.patch:

handles hash collisions

can emit return type

performance path for all-int or all-char test constants

no documentation change

426-supports-named-values.patch:

all from above, plus...

evaluates unquoted test constant symbols at compile-time

validates test constants are suitable for case

doc change: "The test-constants can be any combination of numbers, chars, strings, keywords, quoted symbols, and (Clojure) composites thereof. Unquoted symbols will be evaluated at compile time, and must resolve to one of the preceding types."

426-supports-enums-and-named-values.patch:

all from above, plus...

conditionally supports java enums

doc change: "Java enums are supported only if all test constants are enums of the same type."

The use of eval to detect enums feels icky, and counter to the stated behavior of allowing only compile-time constants.

I assume both of you are actually talking about literals. Limiting case to literal test values is simply too much of a handicap, even ignoring interop uses. One should not have to macro their way out of repeating a constant value (e.g. (def *mole* 6.02214179e23)) throughout a codebase that needs to use case extensively.

Leaving aside questions about mechanics, static final fields and enums are absolutely constants, and not being able to use them within case would be (and is today) a significant pain point. Yes, their value w.r.t. case can change after a case form has been AOT-compiled – which exactly mirrors switch (a good thing, IMO, making case a proper superset of switch). Again, something one can macro out of, but something I don't think people should have to macro out of.

The use of eval to detect enums feels icky, and counter to the stated behavior of allowing only compile-time constants.

I assume both of you are actually talking about literals. Limiting case to literal test values is simply too much of a handicap, even ignoring interop uses. One should not have to macro their way out of repeating a constant value (e.g. (def *mole* 6.02214179e23)) throughout a codebase that needs to use case extensively.
Leaving aside questions about mechanics, static final fields and enums are absolutely constants, and not being able to use them within case would be (and is today) a significant pain point. Yes, their value w.r.t. case can change after a case form has been AOT-compiled – which exactly mirrors switch (a good thing, IMO, making case a proper superset of switch). Again, something one can macro out of, but something I don't think people should have to macro out of.

Thanks for splitting this up. I'm not in favor of anything beyond basic. The enum stuff is a snake pit full of new semantics, contradictions and special cases. I had told Chas as much. Sorry I hadn't realized the enum thing had crept into this scope sooner.

Rich Hickey
added a comment - 05/Mar/11 7:35 AM Thanks for splitting this up. I'm not in favor of anything beyond basic. The enum stuff is a snake pit full of new semantics, contradictions and special cases. I had told Chas as much. Sorry I hadn't realized the enum thing had crept into this scope sooner.
Rich

I should say that I'd consider it perfectly reasonable if case were kept limited to using literals (if that limitation is considered important enough to retain at some level), but clojure.core provided a switch macro that evaluated symbols as Alexander has been reaching for.

Chas Emerick
added a comment - 05/Mar/11 7:36 AM I should say that I'd consider it perfectly reasonable if case were kept limited to using literals (if that limitation is considered important enough to retain at some level), but clojure.core provided a switch macro that evaluated symbols as Alexander has been reaching for.

Chas, I'm not sure what you mean by 'macro their way out of..." - are you saying def is too much work? Or is what you'd want, but doesn't work?

What's missing is not some extension to case, but possibly a broadening of what constitutes a compile time constant. At no point should that involve compile time evaluation in case. A defconst has long been requested and is a fine idea. Ditto static finals as constants. As long as case is defined to work with constants, we can enhance 'constants' and case can do more transparently (at least semantically transparently). All of this stuff around unquoted constant symbols, conditional compile time evaluation (when does that otherwise happen?) etc is a mess. Note that case is currently defined in terms of literals because Clojure doesn't have a strong notion of constant yet.

Also, I think you are not adequately considering the use of case in symbolic computation. People use case matching of symbols all the time when writing macros and compilers etc in CL, and wouldn't appreciate having to quote everything. Also, given we can match aggregates, how might one match (quote x), and why wouldn't that match 'x?

This is scope creep, and insufficiently considered, plain and simple. Let's leave it as a separate ticket. It's going to require more thought, in any case

Rich Hickey
added a comment - 05/Mar/11 8:04 AM - edited Chas, I'm not sure what you mean by 'macro their way out of..." - are you saying def is too much work? Or is what you'd want, but doesn't work?
What's missing is not some extension to case, but possibly a broadening of what constitutes a compile time constant. At no point should that involve compile time evaluation in case. A defconst has long been requested and is a fine idea. Ditto static finals as constants. As long as case is defined to work with constants, we can enhance 'constants' and case can do more transparently (at least semantically transparently). All of this stuff around unquoted constant symbols, conditional compile time evaluation (when does that otherwise happen?) etc is a mess. Note that case is currently defined in terms of literals because Clojure doesn't have a strong notion of constant yet.
Also, I think you are not adequately considering the use of case in symbolic computation. People use case matching of symbols all the time when writing macros and compilers etc in CL, and wouldn't appreciate having to quote everything. Also, given we can match aggregates, how might one match (quote x), and why wouldn't that match 'x?
This is scope creep, and insufficiently considered, plain and simple. Let's leave it as a separate ticket. It's going to require more thought, in any case

By "macro their way out", I meant doing something like I did here to use non-literal constants in case forms. When I wrote that comment this morning, I was under the impression that only Clojure literals were considered acceptable.

Anyway, what follows is either incredibly pedantic, or a draft of a wiki page where a more agreeable solution can be hashed out.

It occurs to me that being explicit about what constants exist and what's on the table here would be helpful, at least for me:

Clojure literals and composites thereof, as implemented in 1.2.0 case

static final fields

def'ed values, presumed to be unchanging

enums

(1) is granted, and it sounds like moving beyond this is not desired at this time without significant further analysis/discussion (implying the application of Alexander's "basic" patch only).

(2) appears to be desired ("Ditto static finals as constants"). However, as soon as we allow for non-literal constant values (whether fields, def's, enums, or other), I'm very confused as to:

how what we use to refer to those values (surely symbols?) aren't turned into a second-class test values

how we're not faced with conditional resolution of those values (there's no calls anywhere, so it seems there's little difference between eval and direct usage of Reflector, at least for fields and enums)

Perhaps the defconst variation hinted at in conjunction with (3) would somehow avoid compile-time evaluation for def'ed values? I'm no longer familiar with CL and the defconstant prior art to reasonably guess at the relevant semantics of defconst.

Re: (4) and "The enum stuff is a snake pit full of new semantics, contradictions and special cases": I'm pretty sure I didn't know that. It seems like enum support is as close as:

(.ordinal (Reflector/getStaticField EnumClassName "EnumName"))

…with all reasonable error-checking and such to ensure the domain of the case form includes only enums of the same type (though again, the issues with resolving symbols remains).

Anyway, I'm sorry to have encouraged (egged on?) Alexander. I appear to have misread/misunderstood the desired scope of changes.

Chas Emerick
added a comment - 05/Mar/11 1:07 PM Well, everything is scope creep in the end.
By "macro their way out", I meant doing something like I did here to use non-literal constants in case forms. When I wrote that comment this morning, I was under the impression that only Clojure literals were considered acceptable.
Anyway, what follows is either incredibly pedantic, or a draft of a wiki page where a more agreeable solution can be hashed out.
It occurs to me that being explicit about what constants exist and what's on the table here would be helpful, at least for me:

Clojure literals and composites thereof, as implemented in 1.2.0 case

static final fields

def'ed values, presumed to be unchanging

enums

(1) is granted, and it sounds like moving beyond this is not desired at this time without significant further analysis/discussion (implying the application of Alexander's "basic" patch only).
(2) appears to be desired ("Ditto static finals as constants"). However, as soon as we allow for non-literal constant values (whether fields, def's, enums, or other), I'm very confused as to:

how what we use to refer to those values (surely symbols?) aren't turned into a second-class test values

how we're not faced with conditional resolution of those values (there's no calls anywhere, so it seems there's little difference between eval and direct usage of Reflector, at least for fields and enums)

Perhaps the defconst variation hinted at in conjunction with (3) would somehow avoid compile-time evaluation for def'ed values? I'm no longer familiar with CL and the defconstant prior art to reasonably guess at the relevant semantics of defconst.
Re: (4) and "The enum stuff is a snake pit full of new semantics, contradictions and special cases": I'm pretty sure I didn't know that. It seems like enum support is as close as:

(.ordinal (Reflector/getStaticField EnumClassName "EnumName"))

…with all reasonable error-checking and such to ensure the domain of the case form includes only enums of the same type (though again, the issues with resolving symbols remains).
Anyway, I'm sorry to have encouraged (egged on?) Alexander. I appear to have misread/misunderstood the desired scope of changes.

Chas, no worries. Being able to use named constant values as test constants makes my life so much easier, especially when dealing with java interop. Until now I've had to use (condp = x ...). Regardless of whether it makes it into core, I will still be using it.

Alexander Taggart
added a comment - 05/Mar/11 2:04 PM - edited Chas, no worries. Being able to use named constant values as test constants makes my life so much easier, especially when dealing with java interop. Until now I've had to use (condp = x ...). Regardless of whether it makes it into core, I will still be using it.

1) Is all we've got right now. Literals compile into themselves. There is no notion of constant in the language.

2) Treating finals as constants would require the compiler compile MyClass/MY_FINAL into its value, which it does not currently do. It currently generates a getStaticField opcode.

3) I never suggested treating defs like constants. A defconst would require some notion of constant. Since its value would be compiled into code, it must be different from def, the semantics of which are to compile into a deref of the var. Only the latter currently occurs. Doing otherwise automagically just inside case would be bizarre. Languages typically constrain the initializers of such constants, esp. to constant expressions themselves. CL allows for arbitrary initializers, but requires they be eval-able at compile time, always evaluate to the same value, and with much hand waving about execution order.

e.g. if as you had suggested:

(def x (launch-missiles))

(case foo x ...)

Would missiles be launched during compilation or loading or both? Would x be nil or the number of missiles launched in the case? What if the case were wrapped in (binding/let [x ...] ...)?

4) is like 2.

You are correct, 2,3,4 all conflict with the use of symbols in case, which is why supporting them is a breaking change, glommed on to a ticket that nominally is a bug fix, augmented by a perf optimization.

Fixing the conflict isn't as simple as saying "use 'x for symbols" in case. You'd need to consider the interaction of constants and all special forms and macros, the relationship between constants and '[un]evaluated' positions everywhere. It's not a trivial thing. CL, FWIW, does not in fact support its own defconstants as keys in its case, considering them constants only in evaluated positions.

I don't have the time right now to consider all of that, but I won't accept a design that hasn't.

Rich Hickey
added a comment - 06/Mar/11 11:05 AM Chas -
1) Is all we've got right now. Literals compile into themselves. There is no notion of constant in the language.
2) Treating finals as constants would require the compiler compile MyClass/MY_FINAL into its value, which it does not currently do. It currently generates a getStaticField opcode.
3) I never suggested treating defs like constants. A defconst would require some notion of constant. Since its value would be compiled into code, it must be different from def, the semantics of which are to compile into a deref of the var. Only the latter currently occurs. Doing otherwise automagically just inside case would be bizarre. Languages typically constrain the initializers of such constants, esp. to constant expressions themselves. CL allows for arbitrary initializers, but requires they be eval-able at compile time, always evaluate to the same value, and with much hand waving about execution order.
e.g. if as you had suggested:
(def x (launch-missiles))
(case foo x ...)
Would missiles be launched during compilation or loading or both? Would x be nil or the number of missiles launched in the case? What if the case were wrapped in (binding/let [x ...] ...)?
4) is like 2.
You are correct, 2,3,4 all conflict with the use of symbols in case, which is why supporting them is a breaking change, glommed on to a ticket that nominally is a bug fix, augmented by a perf optimization.
Fixing the conflict isn't as simple as saying "use 'x for symbols" in case. You'd need to consider the interaction of constants and all special forms and macros, the relationship between constants and '[un]evaluated' positions everywhere. It's not a trivial thing. CL, FWIW, does not in fact support its own defconstants as keys in its case, considering them constants only in evaluated positions.
I don't have the time right now to consider all of that, but I won't accept a design that hasn't.

In the previous patch, if all the test expressions were ints but the tested expression was not known to be primitive, the hashcode branch was used. The hashcode of Integers is equal to their value, but for Long's that's only true for the positive range. This was the source of the bug you found, namely the value in the switch (-1) didn't match the hash of Long -1 (0).

Instead of reusing the hashcode branch, bytecode like the following is now emitted:

Alexander Taggart
added a comment - 06/Apr/11 3:41 AM - edited Update 5 fixes the above bug.
In the previous patch, if all the test expressions were ints but the tested expression was not known to be primitive, the hashcode branch was used. The hashcode of Integers is equal to their value, but for Long's that's only true for the positive range. This was the source of the bug you found, namely the value in the switch (-1) didn't match the hash of Long -1 (0).
Instead of reusing the hashcode branch, bytecode like the following is now emitted:

When multiple test constants have a hash collision, the respective thens are combined with a condp, and the colliding hash value is used as the test constant. Since the combined then performs its own post-switch validation, the normal (non-colliding) behaviour of checking that the original test value equals the tested expression must be skipped.

The bug occurred because the skip-check set of case ints was not getting the necessary shift-mask applied to it.

The bug was not caught earlier because there was incomplete test coverage, namely, for the case of both hash collision and shift-mask application.

Alexander Taggart
added a comment - 22/Apr/11 2:14 PM Updated patch to fix above.
Explanation of bug:
When multiple test constants have a hash collision, the respective thens are combined with a condp, and the colliding hash value is used as the test constant. Since the combined then performs its own post-switch validation, the normal (non-colliding) behaviour of checking that the original test value equals the tested expression must be skipped.
The bug occurred because the skip-check set of case ints was not getting the necessary shift-mask applied to it.
The bug was not caught earlier because there was incomplete test coverage, namely, for the case of both hash collision and shift-mask application.