Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
lexidor opened this issue Oct 22, 2019 · 1 comment

Comments

@lexidor
Copy link
Contributor

lexidor commented Oct 22, 2019

// 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 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.

@mwildehahn
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants