[ RFC ] Shapes::fromDict()
- Dominant language
- C++
- Stars
- 18.7k
- Forks
- 3.1k
- Avg merge
- 1h 47m
- Merged PRs (30d)
- 2
Description
**Is your feature request related to a problem? Please describe.**
As of today, dicts (previously darrays) have the same runtime representation as a `shape(...)`. From what I have gathered, it is common practice to assert that a database row matches a given shape. Right now this can be achieved using:
```HACK
$result->dictRowTyped()[0] as shape('id' => int);
// Previously
darray($result->mapRowTyped()[0]) as shape('id' => int);
```
This pattern is very useful, but it relies on an implementation detail. If shapes ever change their backing type in the future, this code will start to fail.
**Describe the solution you'd like**
I'd like to add `Shapes::fromDict(dict $dict): shape(...)` to ext_shapes.php. This function would be implemented as:
```HACK
final abstract class Shapes {
// ... Rest
public static function fromDict(dict $dict): shape(...) {
return $dict;
}
}
```
This would allow everyone to use the pattern described above, whilst being immune from the implementation detail. If HHVM changes the representation, this function could do whatever magic is required to transform a dict into a `shape(...)`.
**Describe alternatives you've considered**
You can not write this function in userland (without using `as shape(...)` and relying on implementation details).
```HACK
$out = shape();
// This does not typecheck, using a variable for shape key is not allowed.
foreach ($dict as $key => $value) { $out[$key] = $value; }
```
**Additional context**
#### Runtime overhead.
The function itself is zero overhead, since it is zero byte codes when inlined. If this does not happen reliably and performance is a concern, `Shapes::fromDict()` could be declared a special function, like `HH\FIX_ME\unsafe_cast()` is. This function is erased from your source, so it would actually be zero overhead.
#### Shouldn't the key type of the dict be string, because shapes can't have integer keys.
Hack currently disallows shape keys which are integer like or integers. This restriction has come up many a time and [this restriction will probably be lifted](https://github.com/facebook/hhvm/blob/49fdc4cb3df5e7bacb1fe4bf38d91cdf6ba11b86/hphp/hack/doc/HIPs/int_intish_shape_keys.md) some day. The array behavior this rule is trying to guard for was removed in hhvm 4.2. Having `Shapes::fromDict()` exist does not make Hack more unsafe, since you can also declare your keys to be class constants, which can already violate these rules.
#### Does this constrain the runtime from implementing a more efficient shape type?
I am not a runtime engineer, but my guess is maybe. Here is a hypothetical optimization that would be broken by this function existing.
If HHVM would enforce that all shapes must be created with `shape()` constructs, HHVM could optimize more. HHVM would know every possible string key and number them instead.
I'd counter that having `Shapes::fromDict()` would be preferable, even if it needs to be removed later. I'd rather have Hack being able to tell me that `Shapes::fromDict()` is deprecated than having to rely on the runtime to tell me that the LHS of an `as shape(...)` expression was a dict.
_Edits: I typoed this RFC quite bad._
Contributor guide
Research direction
Start with ext_shapes.php and the existing Shapes class, then review the RFC's proposed Shapes::fromDict signature and implementation constraints. Determine whether the API can safely convert a dict to a shape without relying on its current runtime representation; done means the design is accepted and the feature is implemented with the stated compatibility and overhead goals.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- compilers
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100