Re: [PHP-DEV] Declaration-aware attributes

2023-05-29 Thread Stephen Reay



> On 30 May 2023, at 07:48, Andreas Hennings  wrote:
> 
> Hello internals,
> I am picking up an idea that was mentioned by Benjamin Eberlei in the past.
> https://externals.io/message/110217#110395
> (we probably had the idea independently, but Benjamin's is the first
> post where I see it mentioned in the list)
> 
> Quite often I found myself writing attribute classes that need to fill
> some default values or do some validation based on the symbol the
> attribute is attached to.
> E.g. a parameter attribute might require a specific type on that
> parameter, or it might fill a default value based on the parameter
> name.
> 
> Currently I see two ways to do this:
> 1. Do the logic in the code that reads the attribute, instead of the
> attribute class. This works ok for one-off attribute classes, but it
> becomes quite unflexible with attribute interfaces, where 3rd parties
> can provide their own attribute class implementations.
> 2. Add additional methods to the attribute class that take the symbol
> reflector as a parameter, like "setReflectionMethod()", or
> "setReflectionClass()". Or the method in the attribute class that
> returns the values can have a reflector as a parameter.
> 
> Both of these are somewhat limited and unpleasant.
> 
> I want to propose a new way to do this.
> Get some feedback first, then maybe an RFC.
> 
> The idea is to mark constructor parameters of the attribute class with
> a special parameter attribute, to receive the reflector.
> The other arguments are then shifted to skip the "special" parameter.
> 
> #[Attribute]
> class A {
>  public function __construct(
>public readonly string $x,
>#[AttributeContextClass]
>public readonly \ReflectionClass $class,
>public readonly string $y,
>  ) {}
> }
> 
> $a = (new ReflectionClass(C::class))->getAttributes()[0]->newInstance();
> assert($a instanceof A);
> assert($a->x === 'x');
> assert($a->class->getName() === 'C');
> assert($a->y === 'y');
> 
> Note that for methods, we typically need to know the method reflector
> _and_ the class reflector, because the method could be defined in a
> base class.
> 
> #[Attribute]
> class AA {
>  public function __construct(
>#[AttributeContextClass]
>public readonly \ReflectionClass $class,
>#[AttributeContextMethod]
>public readonly ReflectionMethod $method,
>  ) {}
> }
> 
> class B {
>  #[AA]
>  public function f(): void {}
> }
> 
> class CC extends B {}
> 
> $aa = (new ReflectionMethod(CC::class, 
> 'f))->getAttributes()[0]->newInstance();
> assert($a->class->getName() === 'CC');
> assert($a->method->getName() === 'f');
> 
> ---
> 
> Notice that the original proposal by Benjamin would use an interface
> and a setter method, ReflectorAwareAttribute::setReflector().
> 
> I prefer to use constructor parameters, because I generally prefer if
> a constructor creates a complete and immutable object.
> 
> 
> 
> Thoughts?
> 
> -- Andreas
> 
> -- 
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
> 

Hi Andreas,

I too have wondered (and I think asked in room11?) about such a concept. From 
memory the general response was “just do it in userland with a wrapper” so its 
good to see someone else is interested in this being part of the language.

While I agree that it’s most useful if the `Reflector` instance is available in 
the constructor, I’m not keen on the proposed magic “skipping” of arguments as 
you suggest. It seems way too easy to confuse someone (particularly if the 
attribute class itself has reason to be instantiated directly in code)

I think a better approach would be to suggest authors put the parameter at the 
*end* of the parameter list, so that no ‘skipping' is required when passing 
arguments without names (or put it where you like if you’re always using named 
arguments)


Cheers

Stephen 

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



[PHP-DEV] Let ReflectionMethod keep track of original class

2023-05-29 Thread Andreas Hennings
Hello list,
this proposal will be useful in combination with "Declaration-aware attributes"


Problem

Currently, ReflectionMethod is not aware of the original class, if the
method is declared in a parent class.
Methods that are called during a discovery algorithm that need to
process a method with its original class typically need two
parameters:

function processMethod(\ReflectionClass $class, \ReflectionMethod $method) {..}


Proposal

Let a ReflectionMethod object keep track of the original class.
Introduce a new method ReflectionMethod->getOriginalClass() to retrieve it.

class B {
  function f($x) {}
}
class C extends B {}

foreach ([
  // There are different ways to get a reflection method object, all
of them track the original class.
  new ReflectionMethod('C', 'f'),
  (new ReflectionClass('C'))->getMethod('f'),
  (new ReflectionMethod('C', 'f'))->getParameters()[0]->getDeclaringFunction(),
] as $rm) {
  // The following won't change:
  assert($rm->class === 'B');
  assert($rm->getDeclaringClass()->getName() === 'B');
  // New method:
  assert($rm->getOriginalClass()->getName() === 'C');


Alternatives
==

At first I thought we might introduce a new class like
"VirtualReflectionMethod" which behaves as if the method was declared
on the child class. But this is awkward.

I think the ->getOriginalClass() is much simpler.


--- Andreas

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



[PHP-DEV] Re: Declaration-aware attributes

2023-05-29 Thread Andreas Hennings
I just notice a flaw in my thinking.

On Tue, 30 May 2023 at 02:48, Andreas Hennings  wrote:
>
> Note that for methods, we typically need to know the method reflector
> _and_ the class reflector, because the method could be defined in a
> base class.

This is true when doing a discovery using attributes.
However, PHP does not know the original class when we call (new
ReflectionClass('C'))->getMethod('f')->getAttributes().
So it cannot pass the original class into the attribute constructor.

Currently, only the userland code that does the discovery knows the
original class.

We would need a type of method reflector that keeps track of the original class.
This could well be its own separate RFC.
Once this is done, we no longer need to pass the class as a separate argument.

So to keep this RFC independent, we should only pass the reflector
object where ->getAttributes()[*]->newInstance() was called on.
Then if in a separate RFC we keep track of the original class in
ReflectionMethod, the same information will be available when the
method reflector is injected into an attributes.

This also means that the ReflectionAttribute object needs to keep a
reference of the original reflector.
And if it does that, we can as well provide a method to return it.

assert((new ReflectionClass('C'))->getAttributes()[0]->getReflector()->getName()
=== 'C');

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] Declaration-aware attributes

2023-05-29 Thread Andreas Hennings
Thanks for the feedback!

On Tue, 30 May 2023 at 03:43, Dusk  wrote:
>
> On May 29, 2023, at 17:48, Andreas Hennings  wrote:
> > Quite often I found myself writing attribute classes that need to fill
> > some default values or do some validation based on the symbol the
> > attribute is attached to.
> > E.g. a parameter attribute might require a specific type on that
> > parameter, or it might fill a default value based on the parameter
> > name.
>
> +1. This is a substantial limitation in the attribute system.
>
> > Currently I see two ways to do this:
> > 1. Do the logic in the code that reads the attribute, instead of the
> > attribute class. This works ok for one-off attribute classes, but it
> > becomes quite unflexible with attribute interfaces, where 3rd parties
> > can provide their own attribute class implementations.
> > 2. Add additional methods to the attribute class that take the symbol
> > reflector as a parameter, like "setReflectionMethod()", or
> > "setReflectionClass()". Or the method in the attribute class that
> > returns the values can have a reflector as a parameter.
>
> I see a third way which introduces less "magic":

Actually, these two options are what is possible with the current
version of PHP, but it requires to write php code to solve these
problems each time.
The actual proposal in terms of "language design" solution was further
below in my email :)


>
> 3.a. Add a method to ReflectionAttribute which retrieves the target of the 
> attribute as an appropriate reflection object. (Sadly, the obvious name 
> "getTarget" is already taken; I'll call it "getReflectionTarget" for now.)
>
> 3.b. Add a static method to ReflectionAttribute which, when called within an 
> Attribute constructor which is being called by 
> ReflectionAttribute::newInstance(), returns the ReflectionAttribute object 
> which is being instantiated.
>
> These features could be used together to set a default property on an 
> attribute based on its target, e.g.
>
> #[Attribute(Attribute::TARGET_PROPERTY)]
> class PropertyAnnotation {
>   public string $name;
>
>   public function __construct(?string $name = null) {
> $this->name = $name ?? 
> ReflectionAttribute::underConstruction()->getReflectionTarget()->getName();
>   }
> }
>
> Another variant that comes to mind is:
>
> 3.b. Add a new flag to attributes which causes the ReflectionAttribute object 
> to be passed to the constructor as the first argument, e.g.
>
> #[Attribute(Attribute::TARGET_PROPERTY | Attribute::WHO_MADE_ME)]
> class PropertyAnnotation {
>   public string $name;
>
>   public function __construct(ReflectionAttribute $attr, ?string $name = 
> null) {
> $this->name = $name ?? $attr->getReflectionTarget()->getName();
>   }
> }
>
> This is a little messier because it can't be used "under the covers" by an 
> attribute base class, but it accomplishes the same goals.

This is close to the parameter attribute I proposed.
The benefit of the parameter attribute is that a developer can clearly
see which parameter will be skipped for regular arguments.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] Declaration-aware attributes

2023-05-29 Thread Dusk
On May 29, 2023, at 17:48, Andreas Hennings  wrote:
> Quite often I found myself writing attribute classes that need to fill
> some default values or do some validation based on the symbol the
> attribute is attached to.
> E.g. a parameter attribute might require a specific type on that
> parameter, or it might fill a default value based on the parameter
> name.

+1. This is a substantial limitation in the attribute system.

> Currently I see two ways to do this:
> 1. Do the logic in the code that reads the attribute, instead of the
> attribute class. This works ok for one-off attribute classes, but it
> becomes quite unflexible with attribute interfaces, where 3rd parties
> can provide their own attribute class implementations.
> 2. Add additional methods to the attribute class that take the symbol
> reflector as a parameter, like "setReflectionMethod()", or
> "setReflectionClass()". Or the method in the attribute class that
> returns the values can have a reflector as a parameter.

I see a third way which introduces less "magic":

3.a. Add a method to ReflectionAttribute which retrieves the target of the 
attribute as an appropriate reflection object. (Sadly, the obvious name 
"getTarget" is already taken; I'll call it "getReflectionTarget" for now.)

3.b. Add a static method to ReflectionAttribute which, when called within an 
Attribute constructor which is being called by 
ReflectionAttribute::newInstance(), returns the ReflectionAttribute object 
which is being instantiated.

These features could be used together to set a default property on an attribute 
based on its target, e.g.

#[Attribute(Attribute::TARGET_PROPERTY)]
class PropertyAnnotation {
  public string $name;
  
  public function __construct(?string $name = null) {
$this->name = $name ?? 
ReflectionAttribute::underConstruction()->getReflectionTarget()->getName();
  }
}

Another variant that comes to mind is:

3.b. Add a new flag to attributes which causes the ReflectionAttribute object 
to be passed to the constructor as the first argument, e.g.

#[Attribute(Attribute::TARGET_PROPERTY | Attribute::WHO_MADE_ME)]
class PropertyAnnotation {
  public string $name;

  public function __construct(ReflectionAttribute $attr, ?string $name = 
null) {
$this->name = $name ?? $attr->getReflectionTarget()->getName();
  }
}

This is a little messier because it can't be used "under the covers" by an 
attribute base class, but it accomplishes the same goals.
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



[PHP-DEV] Re: Declaration-aware attributes

2023-05-29 Thread Andreas Hennings
A big TBD would be the new attribute classes we need to mark these parameters.
Perhaps we could use just one attribute class, e.g.
"AttributeDeclaration", and use the parameter type to determine which
part of the declaration is expected.

Also, if an attribute is allowed on different symbol types, then these
parameters could be made nullable, or allow multiple types.
Of course some combinations would be ambiguous, e.g.
`\ReflectionClass|\ReflectionMethod`.

#[Attribute(Attribute::TARGET_METHOD, Attribute::TARGET_PROPERTY,
Attribute::TARGET_CLASS_CONSTANT)]
class A {
  public function __construct(
public readonly string $x,
#[AttributeDeclaration] public readonly \ReflectionClass $class,
#[AttributeDeclaration] public readonly ?\ReflectionMethod $method,
#[AttributeDeclaration] public readonly
?\ReflectionClassConstant|\ReflectionProperty $constantOrProperty,
public readonly string $y,
  ) {}
}

On Tue, 30 May 2023 at 02:48, Andreas Hennings  wrote:
>
> Hello internals,
> I am picking up an idea that was mentioned by Benjamin Eberlei in the past.
> https://externals.io/message/110217#110395
> (we probably had the idea independently, but Benjamin's is the first
> post where I see it mentioned in the list)
>
> Quite often I found myself writing attribute classes that need to fill
> some default values or do some validation based on the symbol the
> attribute is attached to.
> E.g. a parameter attribute might require a specific type on that
> parameter, or it might fill a default value based on the parameter
> name.
>
> Currently I see two ways to do this:
> 1. Do the logic in the code that reads the attribute, instead of the
> attribute class. This works ok for one-off attribute classes, but it
> becomes quite unflexible with attribute interfaces, where 3rd parties
> can provide their own attribute class implementations.
> 2. Add additional methods to the attribute class that take the symbol
> reflector as a parameter, like "setReflectionMethod()", or
> "setReflectionClass()". Or the method in the attribute class that
> returns the values can have a reflector as a parameter.
>
> Both of these are somewhat limited and unpleasant.
>
> I want to propose a new way to do this.
> Get some feedback first, then maybe an RFC.
>
> The idea is to mark constructor parameters of the attribute class with
> a special parameter attribute, to receive the reflector.
> The other arguments are then shifted to skip the "special" parameter.
>
> #[Attribute]
> class A {
>   public function __construct(
> public readonly string $x,
> #[AttributeContextClass]
> public readonly \ReflectionClass $class,
> public readonly string $y,
>   ) {}
> }
>
> $a = (new ReflectionClass(C::class))->getAttributes()[0]->newInstance();
> assert($a instanceof A);
> assert($a->x === 'x');
> assert($a->class->getName() === 'C');
> assert($a->y === 'y');
>
> Note that for methods, we typically need to know the method reflector
> _and_ the class reflector, because the method could be defined in a
> base class.
>
> #[Attribute]
> class AA {
>   public function __construct(
> #[AttributeContextClass]
> public readonly \ReflectionClass $class,
> #[AttributeContextMethod]
> public readonly ReflectionMethod $method,
>   ) {}
> }
>
> class B {
>   #[AA]
>   public function f(): void {}
> }
>
> class CC extends B {}
>
> $aa = (new ReflectionMethod(CC::class, 
> 'f))->getAttributes()[0]->newInstance();
> assert($a->class->getName() === 'CC');
> assert($a->method->getName() === 'f');
>
> ---
>
> Notice that the original proposal by Benjamin would use an interface
> and a setter method, ReflectorAwareAttribute::setReflector().
>
> I prefer to use constructor parameters, because I generally prefer if
> a constructor creates a complete and immutable object.
>
> 
>
> Thoughts?
>
> -- Andreas

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



[PHP-DEV] Declaration-aware attributes

2023-05-29 Thread Andreas Hennings
Hello internals,
I am picking up an idea that was mentioned by Benjamin Eberlei in the past.
https://externals.io/message/110217#110395
(we probably had the idea independently, but Benjamin's is the first
post where I see it mentioned in the list)

Quite often I found myself writing attribute classes that need to fill
some default values or do some validation based on the symbol the
attribute is attached to.
E.g. a parameter attribute might require a specific type on that
parameter, or it might fill a default value based on the parameter
name.

Currently I see two ways to do this:
1. Do the logic in the code that reads the attribute, instead of the
attribute class. This works ok for one-off attribute classes, but it
becomes quite unflexible with attribute interfaces, where 3rd parties
can provide their own attribute class implementations.
2. Add additional methods to the attribute class that take the symbol
reflector as a parameter, like "setReflectionMethod()", or
"setReflectionClass()". Or the method in the attribute class that
returns the values can have a reflector as a parameter.

Both of these are somewhat limited and unpleasant.

I want to propose a new way to do this.
Get some feedback first, then maybe an RFC.

The idea is to mark constructor parameters of the attribute class with
a special parameter attribute, to receive the reflector.
The other arguments are then shifted to skip the "special" parameter.

#[Attribute]
class A {
  public function __construct(
public readonly string $x,
#[AttributeContextClass]
public readonly \ReflectionClass $class,
public readonly string $y,
  ) {}
}

$a = (new ReflectionClass(C::class))->getAttributes()[0]->newInstance();
assert($a instanceof A);
assert($a->x === 'x');
assert($a->class->getName() === 'C');
assert($a->y === 'y');

Note that for methods, we typically need to know the method reflector
_and_ the class reflector, because the method could be defined in a
base class.

#[Attribute]
class AA {
  public function __construct(
#[AttributeContextClass]
public readonly \ReflectionClass $class,
#[AttributeContextMethod]
public readonly ReflectionMethod $method,
  ) {}
}

class B {
  #[AA]
  public function f(): void {}
}

class CC extends B {}

$aa = (new ReflectionMethod(CC::class, 'f))->getAttributes()[0]->newInstance();
assert($a->class->getName() === 'CC');
assert($a->method->getName() === 'f');

---

Notice that the original proposal by Benjamin would use an interface
and a setter method, ReflectorAwareAttribute::setReflector().

I prefer to use constructor parameters, because I generally prefer if
a constructor creates a complete and immutable object.



Thoughts?

-- Andreas

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] [RFC] [Discussion] Deprecate functions with overloaded signatures

2023-05-29 Thread Máté Kocsis
Hi Claude,

The replacement methods for IntlCalendar::set() (namely
> IntlCalendar::setDate() and IntlCalendar::setDateTime()) must not have a
> return type of `void`, but `true`, like the original method, for the two
> following reasons:
>
> 1. By changing the returned value, you are introducing an unnecessary
> hazard when migrating code.
>
> 2. Per https://www.php.net/IntlCalendar, all modification methods of that
> class (clear(), roll(), setTime(), etc.) consistently return `true` on
> success and `false` on failure, even when the method is infallible (and
> thus would always return `true`). Don’t break consistency.
>

1.  I don't see much risk with this change, since we clearly indicate at
the very beginning that the new methods return void, so programmers
migrating their code
should pay attention to the return types. IDEs and static analysers can
surely warn them in case of inattention.

2. Some of the methods you mentioned have a "true" return type for a few
weeks now. The biggest motivation for introducing these was to at least
have some chance to
make them void in the future. Doing so is a risky move indeed, so should be
carried out very carefully, if it's possible at all... That's why I
consider it beneficial to spare the
effort and start with a clean slate by adding the new methods in question
with their correct return type.

Regards,
Máté


Re: [PHP-DEV] [RFC] [Discussion] Clone with

2023-05-29 Thread Máté Kocsis
Hi Michał and Larry,

As Tim has already clarified, using literal strings in the left-hand side
of "clone with expressions" won't cause any issues
for IDEs and static analysers to identify the correct property. I got rid
of the shorthand syntax from the proposal because
it is not strictly required (since it has an equivalent alternative), and I
acknowledge the fact that there are certain people
who don't want to have two syntaxes to express the same thing. If it turns
out that we really want to have the short-hand
syntax as well, we can always add support for it later.

Conversely, using the named arguments style syntax (what future scope calls
> a shorthand, which I don't think is accurate as it's nearly the same amount
> of typing)


I'm just using the same wording as Nikita used in his named argument RFC.
Please take a look at the following section:
https://wiki.php.net/rfc/named_params#shorthand_syntax_for_matching_parameter_and_variable_name
Especially this sentence is important:

If I wanted to put these ideas into a general framework, I think one way to
> go about this would be as follows:
> - Consider identifier: $expr as a shorthand for "identifier" => $expr.


Even though the shorthand syntax is only 4 characters shorter than the
normal one (including the missing space before the colon),
for a typical property name which may be 4-8 characters long (I guess),
it's a significant reduction of typing.

That approach, taken now, gives us all the flexibility people have been
> asking for, reuses the existing named arguments syntax entirely, and is the
> most refactorable option.
>

Your suggestion is very interesting/promising... However, I'm a bit worried
that if I only supported the "identifier: expression" syntax for
"clone assignments", and I required people to pass an array (or splatted
array) instead of the "expression => expression" syntax,
they would start complaining why they cannot directly use expressions as
property names... That's why I think all the alternatives are useful
on their own, but only the "expressions => expression" syntax is
vital, since all the other use-cases can be written like this.


> Additionally, the current "property names expression" section shows a very
> odd example.  It's recloning the object N times, reinvoking the __clone
> method each time, and reassigning a single property.  If you're updating
> several properties at once, that is extremely inefficient.  Dynamic
> property names are not the solution there; allowing the list to be dynamic
> in the first place is, and that should not be punted to future scope.
>

I'm aware that it's currently inefficient, but it's the case only when you
want to update a dynamic list of properties. In my opinion, you update
a fixed set of properties most of the time
(e.g. Psr\Http\Message\ResponseInterface::withStatus()), and it's only a
rare case when you need
more dynamic behavior. That's why I believe it's not a huge problem to keep
cloning objects unnecessarily until we add support for the stuff
mentioned in the future scope section.

I am also confused by the choice to make __clone() run before with with
> properties.  I would have expected it to be the other way around.  Can you
> explain (in the RFC) why you did it that way?  That seems more limiting
> than the alternative, as you cannot forward-pass information to __clone()
> this way.
>

To be honest, the current behavior seemed like the natural choice for
me, and I didn't really consider to execute the __clone() method after the
clone assignments.
Do you have a use-case in mind when you need to forward-pass information to
__clone()?

Regards,
Máté


Re: [PHP-DEV] [RFC] Property hooks, nee accessors

2023-05-29 Thread Claude Pache



> Le 8 mai 2023 à 23:38, Larry Garfield  a écrit :
> 
> Ilija Tovilo and I would like to offer another RFC for your consideration.  
> It's been a while in coming, and we've evolved the design quite a bit just in 
> the last week so if you saw an earlier draft of it in the past few months, I 
> would encourage you to read it over again to make sure we're all on the same 
> page.  I'm actually pretty happy with where it ended up, even if it's not the 
> original design.  This approach eliminates several hard-to-implement edge 
> cases while still providing a lot of functionality in one package.
> 
> https://wiki.php.net/rfc/property-hooks
> 

Hi,

If I understand correctly, given:

 $field;
set => $field = $value;
}

public float $someFloatWithHook {
get => $field;
set => $field = $value;
}

}
?>

we have:

someInt = 42.0); // int(42)
var_dump($obj->someFloat = 42); // float(42)
?>

but:

someIntWithHook = 42.0); // float(42)
var_dump($obj->someFloatWithHook = 42); // int(42)
?>

If I am correct, it means that the “This also implies that adding a set hook to 
a property cannot change the result of the = operator” statement is a bit too 
optimistic.


—Claude

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] [RFC] [Discussion] PHP 8.3 deprecations

2023-05-29 Thread Tim Düsterhus

Hi

On 5/29/23 08:44, Go Kudo wrote:

I realized I was about to add the deprecation of `lcg_value()` and forgot
to do so, so I added it.

https://wiki.php.net/rfc/deprecations_php_8_3#global_combined_lcg

As usual, my English is of low quality, so I would appreciate it if you
could point out any problems.


I think it's too early for that. Because:

1. The replacement is only available as of PHP 8.3. Thus there won't be 
a single version where "replacement is available" and "the function does 
not emit deprecation notices" is both true. It should be deprecated with 
PHP 8.4 at the earliest to give folks at least (!) one version to 
cleanly migrate existing code without suppressing any errors / notices / 
deprecations.


2. It's not seedable, thus the implementation can be switched to use a 
different engine without affecting existing code.


3. It's not as commonly misused as mt_rand() is. Primarily because the 
possible use-cases are much more rare.


Best regards
Tim Düsterhus

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods (#[\Override])

2023-05-29 Thread Claude Pache



> Le 11 mai 2023 à 18:37, Tim Düsterhus  a écrit :
> 
> Hi
> 
> I'm now opening discussion for the RFC "Marking overridden methods 
> (#[\Override])":
> 
> 
> 
> RFC: Marking overridden methods (#[\Override])
> https://wiki.php.net/rfc/marking_overriden_methods
> 

Hi Tim,

One weakness of the proposal, is that there is no notice when a method without 
#[\Override] annotation accidentally overrides a parent method. This is 
necessary for the sake of BC, of course. Therefore, (inspired by the 
--noImplicitOverride flag of TypeScript), I suggest adding a complementary 
#[\NoImplicitOverride] annotation on the derived class, that makes #[\Override] 
mandatory on overriding methods. The following example would then trigger a 
compilation error since Laravel 5.4:

message = Http::get($this->url);
$this->save();
  }
}

?>

―Claude

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] [RFC] [Discussion] Clone with

2023-05-29 Thread Tim Düsterhus

Hi

On 5/29/23 13:47, Michał Marcin Brzuchalski wrote:

So there would be no option to vote on shorthand properties, right?

Array syntax with all properties as strings in quotes probably means no
support from IDE.
I think this is a really bad move, I'd be against it.



I expect any IDE or SA tool worth its salt to be able to understand that 
a string-literal is … a string-literal and treat it as constant. Whether 
there are quotes around that literal or not should not make a meaningful 
difference for automated processing.


I'm fine with the proposed syntax [1], but agree that "clone with 
expressions" would be useful to have even in the initial version.


Best regards
Tim Düsterhus

[1] It's consistent with existing syntax and that is important.

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods (#[\Override])

2023-05-29 Thread Tim Düsterhus

Hi

On 5/23/23 17:47, Sara Golemon wrote:

I think targeting 8.3 is aggressive as we're less than a month from FF
(accounting for discussion and voting period).


I didn't expect the proposal to need much of a discussion, as the 
functionality is known from existing programming languages and the 
proposed semantics are the direct result of how the existing LSP checks 
work.




The first argument (about not impacting callers) for "why an attribute and
not a keyword" feels like it's tying itself into a knot to make a specious
point.


To be clear: That is intended as a real argument that I gave some 
thought. The fact that it does not affect users of the method in 
question differs from the other keywords that are part of the method 
signature and thus I find it useful to have it visually separate.


The visibility decides who can call the method, abstract/final add 
restrictions for classes extending the class in question and 
static/non-static decides how the method is called.


The proposed override marker does nothing like that.



// Intentional LSP violations
class A {
   public function foo(): \SimpleXMLElement { return
simplexml_load_file("/tmp/file.xml"); }
}
class TestA extends A {
   #[\Override(Override::IGNORE_RETURN_TYPE_VIOLATION)]
   public function foo(): TestProxyClass { return TestProxy(parent::foo()); }
}

LSP checks are super valuable for writing clean and well debuggable code,
but sometimes they get in the way of mocking or some other non-production
activity.  This could provide a "get out of jail free card" for those
circumstances where you just want to tell the engine, "Shut up, I know what
I'm doing".



This is only tangentially to what my proposal intends to achieve and 
likely needs an entire discussion of its own. I believe it should be a 
separate thing, similarly to the #[\ReturnTypeWillChange] attribute.




// Specific parent override
class A {
   public function foo() { return 1; }
}
class B extends A {
   // Not present in older versions of library, just added by maintainers.
   public function foo() { return bar(); }
}
class C extends B {
   // Errors because we're now overriding B::foo(), not A::foo().
   #[\Override(A::class)]
   public function foo() { return parent::foo() + 1; }
}

C was written at a time before B::foo() was implemented and makes
assumptions about its behavior.  Then B adds their of foo() which breaks
those assumptions.  C gets to know about this more quickly because the
upgrade breaks those assumptions.  C should only use this subfeature in
places where the inheritance hierarchy matters (such as intentional LSP
violations).



This is something I could get behind. In fact one of the "complaints" 
that I read about Java's @Override is that it does not distinguish 
between a parent class and an interface.


However the semantics are much less clear, here. What about multiple 
interfaces that (intentionally) define the same method or a parent class 
+ an interface that both define the method?


The parameter would likely need to be an array and emit an error if any 
class provides the method that is *not* listed, as well as if a class is 
listed that does not provide the method.


However this likely gets a little funky if the method in your example 
was initially implemented in B and later added to A, because then A is 
not listed, but nothing changed about B which is the direct ancestor.


Interestingly this would also allow to handle the case of "this method 
is not overriding anything" by using `#[\Override([])]`.


I'd probably leave this as possible future scope. A new `?array $classes 
= null` (`class-string[]|null`) parameter could be added in the future 
without breaking anything. In any case I would need assistance with the 
implementation or someone else to implement that, because the added 
complexity is outside of what I'm comfortable with doing myself.


Best regards
Tim Düsterhus

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] [RFC] [Discussion] Clone with

2023-05-29 Thread Aleksander Machniak

On 29.05.2023 14:12, Aleksander Machniak wrote:
In "Property name expressions" section's first code example I think you 
intended to do `clone $self` (not `clone $this`) inside the foreach() loop.


Also, I have a feeling that it would be better to implement `clone 
$object with $properties` syntax first and if that is accepted think 
about expressions support. It's not a strong feeling though.


Actually, with some more thought, my opinion now is that introduction of 
property name expressions is not justified. And will be a reason for me 
to vote No.


I'm still not decided about which syntax I'd prefer, maybe you should do 
a poll with these basic options. Sorry, if there was one and I missed it.


1) clone $object with $properties;

2) clone($object, $properties);

3) clone($object, prop1: $var1, prop2: $var2);

4) clone($object, function ($clone) use ($properties) {
   foreach ($properties as $name => $value) {
   $clone->{$name} = $value;
   }
   });

5) clone $obj with (prop1: $var1, prop2: $var2);


note: $properties is iterable
note: in 2-4) brackets are potentially optional.
note: imho, 5) is similar to 3), but worse.

--
Aleksander Machniak
Kolab Groupware Developer[https://kolab.org]
Roundcube Webmail Developer  [https://roundcube.net]

PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] [RFC] [Discussion] Deprecate functions with overloaded signatures

2023-05-29 Thread Claude Pache


> Le 27 avr. 2023 à 23:28, Máté Kocsis  a écrit :
> 
> Hi Internals,
> 
> As you have possibly already experienced, overloaded signatures cause
> various smaller and bigger issues, while the concept is not natively
> supported by PHP. That's why I drafted an RFC which intends to phase out
> the majority of overloaded function/method signatures and also forbid
> the introduction of such functions in the future:
> https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures
> 
> 

Hi Máté,

The replacement methods for IntlCalendar::set() (namely IntlCalendar::setDate() 
and IntlCalendar::setDateTime()) must not have a return type of `void`, but 
`true`, like the original method, for the two following reasons:

1. By changing the returned value, you are introducing an unnecessary hazard 
when migrating code.

2. Per https://www.php.net/IntlCalendar, all modification methods of that class 
(clear(), roll(), setTime(), etc.) consistently return `true` on success and 
`false` on failure, even when the method is infallible (and thus would always 
return `true`). Don’t break consistency.

—Claude




Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods (#[\Override])

2023-05-29 Thread Tim Düsterhus

Hi

On 5/27/23 23:10, Rowan Tommins wrote:

So the argument is that the key estimate for whether to include it in the 
engine is how many users will add the attribute, but not run a static analysis 
tool. If that number is very low, adding it to the engine has a very low value.


As mentioned in my "sibling email": I would not underestimate the 
"advertising" power of having something included in the language and 
thus by extension being included in the PHP documentation and naturally 
seeping into various tutorials.


The fact that I came across Java's @Override annotation as part of my 
university studies that really did not focus on "Java" per se, but 
rather "programming in general" likely indicates that having something 
universally available helps its adoption by be general public.


Best regards
Tim Düsterhus

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods (#[\Override])

2023-05-29 Thread Tim Düsterhus

Hi

On 5/24/23 01:32, David Gebler wrote:

where you're coming from, however I'd refer back to my earlier comment: any
attributes added in the engine should provide a tangible benefit which
*can't be achieved in userland*.



I think this is a flawed premise: Any sort of analysis that PHP itself 
performs can also be performed in userland. IDEs and static analysis 
tools already do that. In fact the probably most-requested feature for 
PHP, Generics, is already implemented in existing userland static analyzers.


Ilija already explained the benefit of having the proposed #[\Override] 
attribute in the language and that benefit applies to all kinds of code 
analysis that could technically be (and already is) performed in userland:


It's part of the language and thus will *always* be available. If it's 
not always available, then folks who switch jobs or contribute to 
various open source projects that made a different choice with regard to 
analyzers will possibly:


- Be unable to rely on a specific check that they found useful in the 
past, because the new SA tool doesn't implement it.
- Be unable to rely on a specific check working as they learned it, 
because the new SA tool implements it differently.


Basically these developers are not working with PHP, but with PHP+Psalm, 
PHP+PHPStan or PHP+Phan or whatever new tool might come around or that I 
forgot about and they will need to (re)learn all the 
functionality/differences if a future endeavour makes a different choice.


By having it as part of the language by definition it would work 
identically for everyone and it can also be documented as part of the 
language. Developers will naturally come across it while learning PHP 
and it might be included in tutorials and so even if they never heard 
about the userland analyzers they might use it in their own code and 
reap the benefits.


As the proposal is a compile time check, the mistake would also be 
immediately apparent just by loading the class (e.g. as part of *any* 
kind of test), it is not necessary to actually execute the method in 
question. So it should be sufficiently visible and usable even in 
codebases that are in a less-than-stellar state.


In short: It should not be required to combine PHP with another tool to 
make it a usable language.



I'll put the email back on unread and hope to
be able to give a more expanded and nuanced reply later.



Do this if you feel it's beneficial to the wider audience of internals to
better understand the motivation and justification for your proposal, but
ultimately it's not me you need to persuade so certainly don't worry about
getting into further nuances on my account.


Yes, I believe it is beneficial for the wider audience, because folks 
might share your opinion, but not publicly speak up on the list.



suggestion of try it as a plugin for PHPStan first
I prefer to spend my time where I believe I can make the biggest impact. 
That is PHP itself and not the analyzer that I personally prefer.


Best regards
Tim Düsterhus

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] [RFC] [Discussion] PHP 8.3 deprecations

2023-05-29 Thread Go Kudo
2023年5月30日(火) 0:42 Nikita Popov :

> On Mon, May 29, 2023, at 08:05, Máté Kocsis wrote:
> > Hi Everyone,
> >
> > Together with multiple authors, we'd like to start the discussion of the
> > usual
> > deprecation RFC for the subsequent PHP version. You can find the link
> below:
> > https://wiki.php.net/rfc/deprecations_php_8_3
> >
> > Regards:
> > Máté Kocsis
>
> I don't think we should deprecate mt_rand().
>
> There are plenty of use-cases that require neither a seedable
> (predictable) RNG sequence, nor a cryptographically-secure RNG. For those
> use-cases (and especially one-off uses), mt_rand() is perfect, and going
> through Randomizer is an entirely unnecessary complication.
>
> I think I could get on board with deprecating srand/mt_srand to make
> rand/mt_rand non-seedable, directing people who need a seedable RNG to use
> Randomizer, which is much better suited to that use-case. However, we
> should retain rand/mt_rand themselves for non-seeded use-cases.
>
> With srand/mt_srand removed, we also would not have to produce any
> particular sequence, and would be free to switch the internal RNG to
> something other than Mt19937.
>
> The same extends to array_rand(), shuffle() and str_shuffle() -- in fact
> the RFC is missing an important voting option, which is "leave them alone",
> or rather "convert to some non-seedable non-CSPRNG" if you prefer.
>
> Regards,
> Nikita


+1

I too feel that the `Randomizer` is overkill for many applications.
However, I believe that there is a suitable alternative to `mt_rand()` /
`rand()` : `random_int()` It probably makes the most sense to use it.

On the other hand, there is no suitable alternative for `lcg_value()`. The
simpler `Randomizer::getFloat()` would be fine, but on the other hand,
floating-point random numbers are very hard to handle, so some say that the
`Randomizer` class should be used. I would like to hear your opinion on
this.

Best Regards,
Go Kudo


Re: [PHP-DEV] [RFC] [Discussion] PHP 8.3 deprecations

2023-05-29 Thread Nikita Popov
On Mon, May 29, 2023, at 08:05, Máté Kocsis wrote:
> Hi Everyone,
> 
> Together with multiple authors, we'd like to start the discussion of the
> usual
> deprecation RFC for the subsequent PHP version. You can find the link below:
> https://wiki.php.net/rfc/deprecations_php_8_3
> 
> Regards:
> Máté Kocsis

I don't think we should deprecate mt_rand().

There are plenty of use-cases that require neither a seedable (predictable) RNG 
sequence, nor a cryptographically-secure RNG. For those use-cases (and 
especially one-off uses), mt_rand() is perfect, and going through Randomizer is 
an entirely unnecessary complication.

I think I could get on board with deprecating srand/mt_srand to make 
rand/mt_rand non-seedable, directing people who need a seedable RNG to use 
Randomizer, which is much better suited to that use-case. However, we should 
retain rand/mt_rand themselves for non-seeded use-cases.

With srand/mt_srand removed, we also would not have to produce any particular 
sequence, and would be free to switch the internal RNG to something other than 
Mt19937.

The same extends to array_rand(), shuffle() and str_shuffle() -- in fact the 
RFC is missing an important voting option, which is "leave them alone", or 
rather "convert to some non-seedable non-CSPRNG" if you prefer.

Regards,
Nikita

Re: [PHP-DEV] [RFC] [Discussion] Clone with

2023-05-29 Thread Brandon Jackson
Hello everyone,

On Mon, May 29, 2023 at 6:48 AM Michał Marcin Brzuchalski
 wrote:

> So there would be no option to vote on shorthand properties, right?
>
> Array syntax with all properties as strings in quotes probably means no
> support from IDE.
> I think this is a really bad move, I'd be against it.

I agree. I think going to a single syntax was a good move, but using the literal
property name would have been better imo.

I really think the syntax would benefit from being more similar to
function calls,
with a syntax like:

clone $obj with (prop1: "value");

This brings the familiarity from named arguments, which in this case would
represent the object's properties.

Then for dynamic property name support, you can borrow array unpacking
into an argument list from function calls as well, allowing the dynamic
properties to be set like:

$myProp = 'prop1';
clone $obj with (...[$myProp => 'value']);

This also gives the ability to have create dynamic properties via anonymous
function as well, like so:

$myFunc = function ($original): array {
$props = [];
// add results
return $props;
};
clone $obj with (...$myFunc($obj));

You even have the ability to pass it whatever you want including the original
object to operate off of.

Regards,
Brandon Jackson

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] [RFC] [Discussion] Clone with

2023-05-29 Thread Larry Garfield
On Mon, May 29, 2023, at 6:47 AM, Michał Marcin Brzuchalski wrote:
> Hi Máté,
>
> pon., 29 maj 2023 o 11:18 Máté Kocsis  napisał(a):
>
>> Hi Everyone,
>>
>> In the meanwhile, I changed my proposal to use [] instead of {} after the
>> "with" clause due to its better receptance.
>> Additionally, I removed support for the shorthand "property assignment"
>> syntax (clone $this with [property1: "foo"]) in
>> favor the more powerful one where the left-hand side supports expressions
>> (clone $this with ["property1" => "foo"])
>> so that we have more time to decide whether the former one is really
>> needed.
>>
>
> So there would be no option to vote on shorthand properties, right?
>
> Array syntax with all properties as strings in quotes probably means no
> support from IDE.
> I think this is a really bad move, I'd be against it.

I concur, especially with a dynamic list pushed off to future scope.  As is, 
we're getting less IDE friendliness and a lot more ' characters in return for 
making the property name dynamic.  However, the *number* of property names is 
still fixed at code time.  That's an incomplete tradeoff.

Conversely, using the named arguments style syntax (what future scope calls a 
shorthand, which I don't think is accurate as it's nearly the same amount of 
typing) and allowing it to be "splatted", just like named arguments, gives us 
the best of all worlds.  The typical case is easy to read, easy to write, and 
IDE-refactor-friendly.  Allowing it to be splatted (just like named arguments) 
means that in the unusual case where you want the number or names of the 
properties to be dynamic (which is a valid use case, but a minority one), you 
can pre-build an array and splat it in place, exactly like you can named 
arguments.

Typical:

return clone $this with (
  statusCode: $code,
);

Fancy:

$props[strtolower('statusCode')] = $code;
if ($reason) {
  $props['reason'] = $reason;
}
return clone $this with ...$props;

That approach, taken now, gives us all the flexibility people have been asking 
for, reuses the existing named arguments syntax entirely, and is the most 
refactorable option.

Additionally, the current "property names expression" section shows a very odd 
example.  It's recloning the object N times, reinvoking the __clone method each 
time, and reassigning a single property.  If you're updating several properties 
at once, that is extremely inefficient.  Dynamic property names are not the 
solution there; allowing the list to be dynamic in the first place is, and that 
should not be punted to future scope.

I really want clone-with, but this seems like a big step backwards in terms of 
its capability.  It may be enough for me to vote no on it, but I'm not sure.

(Pushing clone-with-callable off to later is, I agree, the correct move.)

I am also confused by the choice to make __clone() run before with with 
properties.  I would have expected it to be the other way around.  Can you 
explain (in the RFC) why you did it that way?  That seems more limiting than 
the alternative, as you cannot forward-pass information to __clone() this way.

--Larry Garfield

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] [RFC] [Discussion] Clone with

2023-05-29 Thread Aleksander Machniak

On 29.05.2023 11:18, Máté Kocsis wrote:

Hi Everyone,

In the meanwhile, I changed my proposal to use [] instead of {} after the
"with" clause due to its better receptance.


In "Property name expressions" section's first code example I think you 
intended to do `clone $self` (not `clone $this`) inside the foreach() loop.


Also, I have a feeling that it would be better to implement `clone 
$object with $properties` syntax first and if that is accepted think 
about expressions support. It's not a strong feeling though.


--
Aleksander Machniak
Kolab Groupware Developer[https://kolab.org]
Roundcube Webmail Developer  [https://roundcube.net]

PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php



Re: [PHP-DEV] [RFC] [Discussion] Clone with

2023-05-29 Thread Michał Marcin Brzuchalski
Hi Máté,

pon., 29 maj 2023 o 11:18 Máté Kocsis  napisał(a):

> Hi Everyone,
>
> In the meanwhile, I changed my proposal to use [] instead of {} after the
> "with" clause due to its better receptance.
> Additionally, I removed support for the shorthand "property assignment"
> syntax (clone $this with [property1: "foo"]) in
> favor the more powerful one where the left-hand side supports expressions
> (clone $this with ["property1" => "foo"])
> so that we have more time to decide whether the former one is really
> needed.
>

So there would be no option to vote on shorthand properties, right?

Array syntax with all properties as strings in quotes probably means no
support from IDE.
I think this is a really bad move, I'd be against it.

Cheers,
Michał Macin Brzuchalski


Re: [PHP-DEV] [RFC] [Discussion] PHP 8.3 deprecations

2023-05-29 Thread Kamil Tekiela
I am not sure if others agree but in my opinion, Global Mersenne Twister
should have been a separate RFC. It has a discussion point that people
might want to discuss on mailing list first.


Re: [PHP-DEV] [RFC] [Discussion] Clone with

2023-05-29 Thread Máté Kocsis
Hi Everyone,

In the meanwhile, I changed my proposal to use [] instead of {} after the
"with" clause due to its better receptance.
Additionally, I removed support for the shorthand "property assignment"
syntax (clone $this with [property1: "foo"]) in
favor the more powerful one where the left-hand side supports expressions
(clone $this with ["property1" => "foo"])
so that we have more time to decide whether the former one is really needed.

After talking with Nicolas and Alexandru off-list (I accidentally didn't
click "Reply All" in my response), we identified
that their "clone callback" proposal could also be a future-scope of the
current RFC with the following syntax:

clone $this with function (object $clone): void {
$this->property1 = "foo";
}

In my opinion, this syntax is OK, albeit it's a bit long to write and a bit
difficult to decipher its meaning (maybe it's
just me). That's the main reason I prefer my originally proposed syntax.
Apart from this, I don't see a big issue with it,
neither conceptually, nor with the implementation.

Regards,
Máté


Re: [PHP-DEV] [RFC] [Discussion] PHP 8.3 deprecations

2023-05-29 Thread Go Kudo
2023年5月29日(月) 15:05 Máté Kocsis :

> Hi Everyone,
>
> Together with multiple authors, we'd like to start the discussion of the
> usual
> deprecation RFC for the subsequent PHP version. You can find the link
> below:
> https://wiki.php.net/rfc/deprecations_php_8_3
>
> Regards:
> Máté Kocsis
>

Hi.

I realized I was about to add the deprecation of `lcg_value()` and forgot
to do so, so I added it.

https://wiki.php.net/rfc/deprecations_php_8_3#global_combined_lcg

As usual, my English is of low quality, so I would appreciate it if you
could point out any problems.

Best regards.
Go Kudo


[PHP-DEV] [RFC] [Discussion] PHP 8.3 deprecations

2023-05-29 Thread Máté Kocsis
Hi Everyone,

Together with multiple authors, we'd like to start the discussion of the
usual
deprecation RFC for the subsequent PHP version. You can find the link below:
https://wiki.php.net/rfc/deprecations_php_8_3

Regards:
Máté Kocsis