vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.56k stars 660 forks source link

DX: improve error messages #3379

Open staabm opened 4 years ago

staabm commented 4 years ago

InvalidArgument: Argument 2 of rex_instance_pool_trait::getInstance expects callable(string...):object|null, Closure(mixed):(null|rex_media&static) provided

The separation of the expected value and the actual value is done by a single ,. Its really hard to read. I would suggest something like

InvalidArgument: Argument 2 of rex_instance_pool_trait::getInstance expects callable(string...):object|null but Closure(mixed):(null|rex_media&static) provided

I cant think of something with even more words in between which would be even better

mr-feek commented 4 years ago

I do do agree the messaging could be a bit more concise, however I don't think : vs. , makes much of a difference. I've been contemplating another output format that's more visual based, but don't know exactly what it would look like yet.

The visualizations of the code snippets helps a lot, have you tried enabling them? --show-snippet=true

weirdan commented 4 years ago

Snippets are nice, but messages need to be readable too, for contexts like LSP diagnostics.

weirdan commented 4 years ago

Upper case words could provide easy visual anchors

InvalidArgument: Argument 2 of rex_instance_pool_trait::getInstance EXPECTS callable(string...):object|null GOT Closure(mixed):(null|rex_media&static) instead

mr-feek commented 4 years ago

I do agree that might help!

staabm commented 4 years ago

I do do agree the messaging could be a bit more concise, however I don't think : vs. , makes much of a difference. I've been contemplating another output format that's more visual based, but don't know exactly what it would look like yet.

In my example I changed , for the word but..

But I agree, as mentioned earlier, there will be better alternatives.

I also thought about a diff formatted style of actual vs expected types

mr-feek commented 4 years ago

I think some sort of diff style could be great -- that's what i had in mind.

Also, for the likes of this:

https://psalm.dev/r/1a5fd559cf

Maybe some sort of error message like "does not actually return declared null type" or something could be simpler and easier to understand.

psalm-github-bot[bot] commented 4 years ago

I found these snippets:

https://psalm.dev/r/1a5fd559cf ```php
weirdan commented 4 years ago

does not actually return declared null type

I think messages for any type contradiction should actually spell out types fully, in predictable format. Psalm actually has those special cases for null & false in some places, and I always find myself wandering if I passed ?A where B was expected when Psalm in cases like this: https://psalm.dev/r/b3beff7046

psalm-github-bot[bot] commented 4 years ago

I found these snippets:

https://psalm.dev/r/b3beff7046 ```php
githoober commented 4 years ago

Every time I get these errors, I get very discouraged to investigate them as it involves copy-pasting them elsewhere and unwrap them to be more easily deciphered image

mholubowski commented 4 years ago

We'd very much like this feature implemented.

The InvalidArgument error messages for even the most basic array types is very difficult to work with:

image

Is there any way to fund ($) this feature to make it happen asap?

@TysonAndre You seem like a fantastic contributor in this space, could you perhaps weigh in on this?

Can anyone else please chime in with how they work with these type of error messages? I must be doing something wrong.

TysonAndre commented 4 years ago

That's a concern about usability, but a slightly different one from what the original issue is - it's hard to read long array shapes no matter what

One feature I saw in TypeScript might be useful in the human readable (default text) output format - Nested explanations of why a complex type can't match with another union type. Other output formats such as pylint may not see much benefit from this by default due to not allowing newlines

Of course, there's probably hundreds of type mismatch issues, so this would be time consuming, and require refactoring the way issues get emitted

Invalid Argument::: some_function expects array<string, array{display: string, other: int}>|false, array<string, array{something: string, display: string, other: array}> provided
     Array element array{something: string, display: string, other: array} cannot cast to array{display: string, other: int}>
          Array field other: array cannot cast to other: int
                array cannot cast to int

It may also help just to reword the issue message template from , to but was passed ...actual... to distinguish this.

Highlighting union types and variables/functions differently may help in breaking this up, but I don't know what the psalm maintainers' stance on this is - I assume they went for a deliberately minimal usage of colors.

weirdan commented 4 years ago

Typescript output is somewhat more useful as it lists missing properties (for this code: https://codesandbox.io/s/plgek?file=/index.ts):

q.ts:18:5 - error TS2739: Type '{ display_name: string; length: string; show_list_table_column: true; show_on_edit_form: true; show_on_new_form: true; type: string; }' is missing the following properties from type '{ display_name: string; enum_options?: any[]; is_enum: boolean; length: string; required: boolean; show_list_table_column?: boolean; show_on_edit_form?: boolean; show_on_new_form?: boolean; type: string; }': is_enum, required

It doesn't seem to do this nested explanation thing for shapes.

ktomk commented 4 years ago

From the wording I would favor expected ... , got ... (instead of having the the provided at the very end as it is now).

I often find such a notation more readable and more informative, but I can imagine this is subjective.

CAPS in shell, no way unless something like a table header which is not the case here. This is less subjective, CAPS is considered screaming. But still, your mileage may vary, my suggestion though is w/o caps and it adds more space between the expected and the provided parts already.

More space could be added with the tab character which is also often used as field separator. Other control characters might come to mind as well, e.g. unit separator from the C0 control characters.

As error messages are part of the binary interface, I would consider this a breaking change.

mholubowski commented 3 years ago

Hey! Could anyone share how they're currently deciphering type error messages when the types in question are large/complex arrays shapes?

Is there some tool you use or copy-paste the messages into?

As an example: here's a returned error message which is effectively 'one of the fields is missing or of the wrong type', but it's tricky to know which! @TysonAndre could you recommend some way to get a "diff"? Even if it means copy-pasting error messages manually into some 3rd party tool.

Argument 1 of SolidAffiliate\Lib\VO\SchemaEntry::__construct expects array{auto_increment?: bool, custom_form_input_attributes?: array<string, string>, default?: mixed, display_name: string, enum_options?: array<array-key, mixed>, form_field_description?: string, form_input_description?: string, form_input_placeholder?: string, form_input_type_override?: "bigint"|"bool"|"datetime"|"float"|"multi_checkbox"|"varchar"|"wp_editor"|"wp_page", is_enum?: bool, is_password?: bool, key?: bool, length?: int, primary_key?: bool, required?: bool, sanitize_callback?: callable(mixed):mixed, settings_group?: string, settings_tab?: string, show_list_table_column?: bool, show_on_edit_form?: "disabled"|"hidden"|"hidden_and_disabled"|bool, show_on_new_form?: "disabled"|"hidden"|"hidden_and_disabled"|bool, show_on_preview_form?: "disabled"|"hidden"|"hidden_and_disabled"|bool, type: "bigint"|"bool"|"datetime"|"float"|"multi_checkbox"|"varchar"|"wp_editor"|"wp_page", unique?: bool, user_default?: mixed, validate_callback?: callable(mixed):bool}, array{auto_increment: true, display_name: "ID", length: "20", primary_key: true, required: true, sanitize_callback: pure-Closure(mixed):int, show_list_table_column: true, type: "bigint", validate_callback: pure-Closure(mixed):bool} providedPsalmhttps://psalm.dev/004
TysonAndre commented 3 years ago

@TysonAndre could you recommend some way to get a "diff"

No, there's currently no tool I'm using - I'm currently looking at it manually property name by property name in a text editor - that was a feature request/suggestion

frodeborli commented 3 years ago

I would like to contribute to this with the error message I'm reading. A couple of new-lines would go a long way in making errors more readable. Even better would be if you could simply remove the array keys that are NOT invalid.

The inferred type 'array{$meta: array{auto_increment: false, plural: "Users", plural_description: "User accounts that are authorized to access the system", singular: "User", singular_description: "A user account", table: "users"}, properties: array{created: array{$extends: array{$meta: array{mysql_type: "JSON", type: "json"}, $path: "/components/schemas/Charm-Types-Json", description: "Updated object", example: array{account_id: "1234", description: "Description", extraInfo: array{additional: "context"}, remote_addr: "174.123.20.9", timestamp: "2021-06-27T23:48:34+00:00", user_id: "1234"}, readOnly: true, type: "object"}}, email: array{$extends: array{$meta: array{mysql_type: "VARCHAR", type: "string"}, $path: "/components/schemas/Charm-Types-String", example: "A string", type: "string"}, format: "email", uniqueItems: true}, is_admin: array{$extends: array{$meta: array{mysql_type: "TINYINT", type: "bool"}, $path: "/components/schemas/Charm-Types-Boolean", example: true, type: "boolean"}, default: false, nullable: false, readOnly: true}, last_login: array{$extends: array{$meta: array{mysql_type: "DATETIME", type: "datetime"}, $path: "/components/schemas/Charm-Types-DateTime", example: "2018-11-13T20:20:39+00:00", format: "date-time", type: "string"}, nullable: true, readOnly: true}, modified: array{$extends: array{$meta: array{mysql_type: "JSON", type: "json"}, $path: "/components/schemas/Charm-Types-Json", description: "Modified object", example: array{account_id: "1234", description: "Description", extraInfo: array{additional: "context"}, remote_addr: "174.123.20.9", timestamp: "2021-06-27T23:48:34+00:00", user_id: "1234"}, readOnly: true, type: "object"}}, name: array{$extends: array{$meta: array{mysql_type: "VARCHAR", type: "string"}, $path: "/components/schemas/Charm-Types-String", example: "A string", type: "string"}, maxLength: 60, minLength: 1}, password: array{$extends: array{$meta: array{mysql_type: "VARCHAR", type: "string"}, $path: "/components/schemas/Charm-Types-String", example: "A string", type: "string"}, writeOnly: true}, phone: array{$extends: array{$meta: array{mysql_type: "VARCHAR", type: "string"}, $path: "/components/schemas/Charm-Types-String", example: "A string", type: "string"}}, username: array{$extends: array{$meta: array{mysql_type: "VARCHAR", type: "string"}, $path: "/components/schemas/Charm-Types-String", example: "A string", type: "string"}, maxLength: 60, minLength: 3, uniqueItems: true}}, required: array{"name", "username", "password"}}' does not match the declared return type 'array{$meta: array{auto_increment?: bool, plural: string, plural_description: string, singular: string, singular_description: string, table: string}, $path: string, properties: array<string, array{$extends?: array<array-key, mixed>, $meta?: array<array-key, mixed>, $path: string, default?: mixed, enum?: array<array-key, mixed>, exclusiveMaximum?: bool, exclusiveMinimum?: bool, format?: string, maxLength?: int, maximum?: int, minLength?: int, minimum?: int, multipleOf?: float, nullable?: bool, pattern?: string, readOnly?: bool, type?: string, uniqueItems?: bool, writeOnly?: bool}>, required?: array<array-key, string>}' for Models\User::orm

adamoceallaigh commented 1 year ago

Is #8894 the current status of this issue to print error message in a more readable manner ?

andyg0808 commented 10 months ago

I built a little tool to parse and display Psalm array error messages in what's hopefully a helpful way: https://ifixit.github.io/psalm-error-parser/

It's hosted on GitHub pages and written entirely in JS.

Here's an example: copy-pasting the first error from https://psalm.dev/r/876f473d2c results in this view: image instead of this: INFO: LessSpecificReturnStatement - 7:12 - The type 'array{config: array{bread: 42, chalk: 'some'}, greeting: 'hello'}' is more general than the declared return type 'array{config: array{bread: int, cheese: int}, greeting: string}' for takesAnInt

psalm-github-bot[bot] commented 10 months ago

I found these snippets:

https://psalm.dev/r/876f473d2c ```php ["bread" => 42, "chalk" => "some"], "greeting" => "hello"]; } $data = ["some text", 5]; takesAnInt($data[0]); $condition = rand(0, 5); if ($condition) { } elseif ($condition) {} ``` ``` Psalm output (using commit a75d26a): INFO: LessSpecificReturnStatement - 7:12 - The type 'array{config: array{bread: 42, chalk: 'some'}, greeting: 'hello'}' is more general than the declared return type 'array{config: array{bread: int, cheese: int}, greeting: string}' for takesAnInt INFO: UnusedParam - 6:28 - Param i is never referenced in this method INFO: MoreSpecificReturnType - 4:12 - The declared return type 'array{config: array{bread: int, cheese: int}, greeting: string}' for takesAnInt is more specific than the inferred return type 'array{config: array{bread: 42, chalk: 'some'}, greeting: 'hello'}' ERROR: TypeDoesNotContainType - 15:11 - Operand of type 0 is always falsy ERROR: TypeDoesNotContainType - 15:11 - Type 0 for $condition is always !falsy ```