hack-json-schema icon indicating copy to clipboard operation
hack-json-schema copied to clipboard

[API design | RFC] `Validator<T>` is contravariant, causing suboptimal typing.

Open lexidor opened this issue 5 years ago • 1 comments

// From hack-json-schema/src/BaseValidator.php

interface Validator<+T> {
    public function validate(): void;
    public function isValid(): bool;
    public function getErrors(): vec<TFieldError>;
    public function getValidatedInput(): ?T;
    //                                   ^^ nullable T
}

abstract class BaseValidator<+T> implements Validator<T> {
   //...
    final public function getValidatedInput(): T {
      //...                                    ^ non-nullable T
    }
}

Let's say we want to make an API that can either be used using runtime typechecking (using a json schema) or by trusting the typechecker, in cases where the performance of validating a bajillion-element array is not feasible.

use namespace Our\Json;
use namespace Slack\Hack\JsonSchema;

class OptionallyTypedAPI<T> {
    public function __construct(
        private (function(mixed): JsonSchema\Validator<T>) $validator_function
) {}
    public function trustTheTypesystem(T $argument): void {
        echo json_encode($argument);
    }
    public function useTheValidator(mixed $argument): void {
        $validator = ($this->validator_function)($argument);
        $validator->validate();
        if (!$validator->isValid()) {
            throw new TypeError("some runtime message");
        }
        echo json_encode($argument);
    }
}

<<__EntryPoint>>
async function main_api_design_rfc_async(): Awaitable<void> {
    // $foo is not OptionallyTypedAPI<shape('success' => bool)>
    $foo = new OptionallyTypedAPI($json ==> new Our\SuccessBoolSchema($json));

    // $foo now becomes OptionallyTypedAPI<(shape('success' => bool) | darray<int, int>)>
    // depending on the type inference of your hh_client.
    // I think Slack's hh_client will actually use Unresolved[...] instead, but don't quote me on that.
    $foo->trustTheTypesystem([5 => 5]); // This is fine!?
}

You will quickly run into the situation where the contravariance-ness of Validator<T> means that hh_client will just make your OptionallyTypedAPI<T> into OptionallyTypedAPI<some_super_type_or_mixed>.

I would like to discuss adding another interface which solves these two issues in one go.

interface InvariantValidator<T> extends Validator<T> {
    public function getValidatedInput(): T;
}

This would prevent the typechecker from generalizing your API, since it must use the exact type of T in: private (function(mixed): JsonSchema\InvariantValidator<T>) $validator_function And as a bonus, I can make getValidatedInput() return T instead of ?T, without breaking BC.

We could add an implements JsonSchema\InvariantValidator<TYourCurrentSchema> to the generated classes. This would not prevent you from doing contravariant things if you refer to them as Validator<T> or BaseValidator<T>.

I would love to get some feedback on this.

lexidor avatar Oct 22 '19 18:10 lexidor

The Validator interface returning ?T is an oversight. We used to return ?T if you called getValidatedInput and it hadn't been validated, but it was annoying to have to cast that to nonnull so we added throwing the exception if you tried to access the value before calling validate. I think I just forgot to update the interface.

mwildehahn avatar Oct 26 '19 02:10 mwildehahn