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

Improve error messages for shape and vec -like types #429

Conversation

vrielsa
Copy link

@vrielsa vrielsa commented Oct 27, 2023

Given:

use Psl\Type;
$shape = Type\shape([
    'name' => Type\string(),
    'articles' => Type\vec(Type\shape([
        'title' => Type\string(),
        'content' => Type\string(),
        'likes' => Type\int(),
        'comments' => Type\optional(Type\vec(Type\shape([
            'user' => Type\string(),
            'comment' => Type\string()
        ]))),
    ])),
    'dictionary' => Type\dict(Type\string(), Type\vec(Type\shape([
        'title' => Type\string(),
        'content' => Type\string(),
    ]))),
    'pagination' => Type\Shape([
        'currentPage' => Type\uint(),
        'totalPages' => Type\uint(),
        'perPage' => Type\uint(),
        'totalRows' => Type\uint(),
    ])
]);

try {
    $shape->coerce([
        'name' => 'ok',
        'articles' => [
            [
                'title' => 'ok',
                'content' => 'ok',
                'likes' => 1,
                'comments' => [
                    [
                        'user' => 'ok',
                        'comment' => 'ok'
                    ],
                    [
                        'user' => 'ok',
                        'comment' =>'ok',
                    ]
                ]
            ]
        ],
       'dictionary' => [
           'key' => [
               [
                   'title' => 'ok',
                   'content' => 'ok',
               ]
           ]
       ]
    ]);
} catch (Type\Exception\CoercionException $e) {
    echo $e->getMessage().PHP_EOL;
}

Results in:

Could not coerce "array" to type "array{'name': string, 'articles': vec<array{'title': string, 'content': string, 'likes': int, 'comments'?: vec<array{'user': string, 'comment': string}>}>, 'dictionary': dict<string, vec<array{'title': string, 'content': string}>>, 'pagination': array{'currentPage': uint, 'totalPages': uint, 'perPage': uint, 'totalRows': uint}}": at path pagination

The changed I made do have quite an impact on performance, but tried to keep it at a minimum. Mainly vecs and dicts are suffering. Not sure what the accepted range is.
image

image

The impact on shapes are minimal:
image

(See #412)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6666122755

  • 26 of 26 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 99.003%

Totals Coverage Status
Change from base Build 6487837444: 0.004%
Covered Lines: 4170
Relevant Lines: 4212

💛 - Coveralls

Copy link
Collaborator

@veewee veewee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 for this change!

To me, It is (at first feeling acceptibally) slower but at least it will give you helpful error messages.
Not sure how other people see this.

Some additional comments on the code:

  • You only cover coerce(). We could have this on assert() as well?
  • You currently don't cover paths for the types: map, mutable_map, vector, mutable_vector, non_empty_vec, non_empty_dict, mixed_dict, mixed_vec

$result = [];

/**
* @var Tk $k
* @var Tv $v
*/
foreach ($value as $k => $v) {
$value_type = $this->value_type->withTrace(
$trace->withFrameAtPath(
'dict<_, ' . $this->value_type->toString() . '>',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string concatenations are fast, but maybe it helps to build the frame name outside of the loop and use it instead of always recalculating the frame name?
It will always be the same value.

/** @var Type\Type<Tv> $value_type */
$value_type = $this->value_type->withTrace(
$this->getTrace()
->withFrameAtPath($this->toString(), (string) $i)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here It might help to build the frame name outside of the loop.

@@ -94,4 +94,29 @@ public function testConversionFailure(): void
static::assertCount(0, $frames);
}
}

public function testCoercionPath(): void
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a start, but we'll need tests for every changed type individually.

@veewee veewee added Priority: Low This issue can probably be picked up by anyone looking to contribute to the project, as an entry fix Status: Revision Needed At least two people have seen issues in the PR that makes them uneasy. Type: Enhancement Most issues will probably ask for additions or changes. labels Nov 22, 2023
@veewee
Copy link
Collaborator

veewee commented Nov 22, 2023

This might become a bit too slow on big data structures.
An alternative approach could be to re-run the assertions / coercions and collect that more advanced path information on failure only.

This issue needs some more thoughts. I'm gonna keep it open for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low This issue can probably be picked up by anyone looking to contribute to the project, as an entry fix Status: Revision Needed At least two people have seen issues in the PR that makes them uneasy. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants