Add support for codegen xhp classes#138
Add support for codegen xhp classes#138fredemmott merged 5 commits intohhvm:masterfrom lexidor:add-xhp-class-support
Conversation
Adds xhp attribute codegen to traits and classes. No error is thrown when attributes are added to non-xhp classes. This syntax is valid, but not useful in the current class. Inheritance maybe?
| $this->generatedFrom ? $this->generatedFrom->render() : null; | ||
|
|
||
| $doc_block_parts = \array_filter(varray[$this->docBlock, $generated_from]); | ||
| $doc_block_parts = Vec\filter_nulls(vec[$this->docBlock, $generated_from]); |
There was a problem hiding this comment.
This behaves differently when either string is '' or 0. This is a breaking change, but it is very very minor. Can revert if need be.
There was a problem hiding this comment.
I rewrote this to stop relying on typechecker magic.
This kind of magic always catches me off guard.
I don't know if this is also surprising to others.
I am just being explicit for the reason of being explicit.
The behavior change is a wash 0 ✔️ '' ❌.
/*
* Calls to array_filter are rewritten depending on the type
* of argument to have one of the following signatures:
*
* function(array, ?(function(Tv):bool)): array
* function(KeyedContainer<Tk, Tv>, ?(function(Tv):bool)): array<Tk, Tv>
* function(Container<Tv>, ?(function(Tv):bool)): array<arraykey, Tv>
*
* Single argument calls additionally remove nullability of Tv, i.e.:
*
* function(Container<?Tv>): array<arraykey, Tv>
*
*/
| $this->decorator is nonnull, | ||
| ' '. | ||
| xhp_attribute_decorator_to_string( | ||
| $this->decorator ?? XHPAttributeDecorator::REQUIRED, |
There was a problem hiding this comment.
The ?? @required value will never be observed.
I previously said as nonnull here, but ->addIf() is not magic, so this argument is always evaluated, even when $this->decorator is null.
| $this->decorator is null, | ||
| 'XHP attributes with an %s decorator can not have a default value', | ||
| xhp_attribute_decorator_to_string($this->decorator), | ||
| ); |
There was a problem hiding this comment.
Do we want to throw for these things early?
string src = 'https://facebook.com' @required is invalid syntax.
There was a problem hiding this comment.
yep, early's good, and seems like it'll give the most useful call stack for the error
| * | ||
| * @see codegenXHPAttributef | ||
| */ | ||
| public function codegenXHPAttribute(string $name): CodegenXHPAttribute; |
There was a problem hiding this comment.
This breaks all current implementers of this interface out in the wild.
What do we think about this breaking change?
There was a problem hiding this comment.
Unavoidable: adding new language features to codegen needs to be acceptable
I copy pasted property and did not touch up the doc well enough.
| $this->decorator is null, | ||
| 'XHP attributes with an %s decorator can not have a default value', | ||
| xhp_attribute_decorator_to_string($this->decorator), | ||
| ); |
There was a problem hiding this comment.
yep, early's good, and seems like it'll give the most useful call stack for the error
| * | ||
| * @see codegenXHPAttributef | ||
| */ | ||
| public function codegenXHPAttribute(string $name): CodegenXHPAttribute; |
There was a problem hiding this comment.
Unavoidable: adding new language features to codegen needs to be acceptable
3 participants
Adds xhp attribute codegen to traits and classes.
No error is thrown when attributes are added to non-xhp classes.
This syntax is valid, but not useful in the current class.
Inheritance maybe?