Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112769
User-Agent: Cyrus-JMAP/3.3.1-61-gb52c239-fm-20201210.001-gb52c2396
Mime-Version: 1.0
Message-ID: <e21b2013-2556-4282-bbba-1d9f28eec8c5@www.fastmail.com>
In-Reply-To: 
 <CAF+90c83WX-jxfoa6FC7yhgYYCcxTMTLC2W5raGrZQorUk+jWA@mail.gmail.com>
References: <eadd4a32-a05b-463b-a91f-a5d353c577b2@www.fastmail.com>
 <CAF+90c83WX-jxfoa6FC7yhgYYCcxTMTLC2W5raGrZQorUk+jWA@mail.gmail.com>
Date: Tue, 05 Jan 2021 11:56:01 -0600
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] [RFC] Enumerations, Round 2
From: larry@garfieldtech.com ("Larry Garfield")

On Tue, Jan 5, 2021, at 8:26 AM, Nikita Popov wrote:

> Nice work, I like the updated proposal. Some notes:
> 
> > Similarly, enum names and case names are both case insensitive.
> 
> I agree that enum names should be case insensitive (like class names), but
> why should case names be case insensitive? The closest analogon to a case
> would be a class constant, and those are case sensitive.

This is a left over from the earlier draft when Cases were also classes.  I've corrected it.  Thanks for the catch.

> > All Cases have a read-only property, case, that is the case-sensitive
> name of the case itself.
> 
> I can see how this makes sense, but it wouldn't be my first guess as to how
> you access the case name. I'm wondering if using $enumValue->name or
> $enumValue->caseName might be preferable.

We'll change it to ->name.

> > Scalar equivalent values must be literals. Constants and constant
> expressions are not supported.
> 
> Why? This seems inconsistent with the overall language. If I can use a
> constant expression as a class constant value, why can't I use it as an
> enum value?

Mostly because it was more challenging to do.  We're fine with including it if Ilija can make it work. (Suggestions for him on how to do so most efficiently are welcome.)

> > ScalarEnum exposes an additional static method from() that is
> automatically generated.
> 
> I think it would be good to be slightly more explicit here and call it
> fromValue(). (We could have fromName() to construct it from a case name,
> and any number of custom from* named constructors.)
> 
> > The from() method will up-cast from a scalar to its corresponding
> Enumerated Case. Invalid scalars with no matching Case will throw a
> ValueError. There is also a has() method, which will return boolean true if
> a case with that value exists and false otherwise
> 
> Just a thought, but rather than having has() and from(), it may make sense
> to have from() (throws if invalid) and tryFrom() (returns null if invalid).
> I think a has() method is pretty much useless in isolation, it will always
> be used in a combination of has() and from(), in which case it is better to
> combine them rather than have an implicit contract between them.

Hm, maybe.  My concern is that "try" to me implies "exceptions are involved somewhere", since PHP doesn't have the "TryX means nullable return" convention that Rust or C# have. 

Anyone else want to weigh in?

> > Manually defining a static from() or has() method on a Scalar Enum will
> result in a fatal error.
> 
> Or a non-static one :) You can't define a static and a non-static method
> with the same name in PHP.

Fixed.

> > Enums and cases may have attributes attached to them, like any other
> language construct. The TARGET_CLASS target filter will include Enums
> themselves. The TARGET_CONST target filter will include Enum Cases
> 
> TARGET_CONST should presumably be TARGET_CLASS_CONST. More generally
> though, I wonder if there should be a separate TARGET_CASE...

We had that in the spec until like 40 hours ago. :-)  We removed it mainly because beberlei was pushing back on it.  (And the corresponding reflection classes, which are still under discussion.)

Personally I'd rather have the flexibility of targeting enums and cases specifically.  Ilija and Benjamin feel it's better to "embrace the object-y-ness" of enums and just use the existing targets.  It's not a hill I'm going to die on either way, so if you can convince them I'm happy to add them back in. 

> > Returns the scalar equivalent type of the Enum, if any. If it doesn't
> have one, it returns a ReflectionType on null.
> 
> Possibly I'm misunderstanding the sentence, but why does it return a
> "ReflectionType on null" rather than just "null"? It's not like ::$value
> will have a null value in this case, it will not exist at all.

More likely I'm misunderstanding the nuances of the reflection API.  The ideal here is to avoid a nullable return if at all possible.  As noted above reflection is the shakiest part left right now, so if you have specific suggestions for how it would work better, please share.

> > Additionally, a new function is_enum(mixed): bool returns true if the
> value passed is an enum or case object.
> 
> Could you please clarify what "an enum or case object" means? Is this
> function both for checking whether an object is a case object and a
> (string) class an enum class?

It would return true if given an enum case, a variable that is assigned to an enum case, or a class name string that refers to a class that is an enum.  Other than replicating the test for it I;m not sure how better to describe it. (Or, just use that sentence?)

> Some typo/etc notes on the text:
> 
> > All Unit Enums as implemented as instances of their enum type.
> 
> "as" -> "are" possibly?
> 
> > That is one way to determine an Enum from any other object:
> 
> "determine" -> "distinguish" possibly?
> 
> > The above hierarchy is logically similar to the following class structure
> (although this is not the actual code that runs):
> 
> You might want to change the constructor to
> 
>     private function __construct(public string $case) {}
> 
> in this example. The use of "new static" for a final class is also
> confusing.

Got them all, thanks.  (new static is because at this point I basically never use self and always use static, as it's what I actually want 99.4% of the time.)

--Larry Garfield