microsoft / pylance-release

Documentation and issues for Pylance
Creative Commons Attribution 4.0 International
1.72k stars 765 forks source link

Explain types and how they work better #3892

Closed rchiodo closed 11 months ago

rchiodo commented 1 year ago

Linked items:

See the discussion here: https://github.com/microsoft/pyright/discussions/3953#discussioncomment-4733920

And the numerable other cases where how types should be used was confusing.

A lot of the ones in this category might be avoided if we could explain things more clearly: https://github.com/microsoft/pyright/issues?q=is%3Aissue+is%3Aclosed+label%3A%22as+designed%22

This issue is to provide a way to better explain python typing to Pylance users.

rchiodo commented 1 year ago

Potential idea: Every pylance error should explain why it happened and give examples on how to use the typing construct.

rchiodo commented 1 year ago

Another idea: Train ChatGPT on Eric's responses.

erictraut commented 1 year ago

Most of pyright's errors do attempt to explain the cause of an error, but there are some (think "syntax error") where it should be self-explanatory. In other cases (e.g. "TypeVar appears only once in signature" or "expression is not used") where the diagnostic is indicative of some fundamental misunderstanding on the part of a user, but it's difficult to explain succinctly what the source of that confusion is or how they might remedy the error.

Other investment ideas:

rchiodo commented 1 year ago

This is what ChatGPT3 said about the issue mentioned:

image

erictraut commented 1 year ago

That's a pretty good explanation, but the recommended fix is likely wrong. (I say "likely wrong" beacuse it's difficult to say what the user actually intended in this case.) A better recommendation in this case is "change the annotation for parameter xynum to XY[int, int]". Or "Change the definition of XYNUM to a type alias: XYNUM: TypeAlias = XY[int, int]".

rchiodo commented 1 year ago

Well, this is why it needs to be trained only on your responses :)

rchiodo commented 1 year ago

Another idea:

Here's an example:

            this._addDiagnostic(
                this._fileInfo.diagnosticRuleSet.reportGeneralTypeIssues,
                DiagnosticRule.reportGeneralTypeIssues,
                Localizer.Diagnostic.annotationNotSupported(),
                node.chainedTypeAnnotationComment
            );

That would instead put up a message like so:

Type annotation not supported. [More Info](https://github.com/microsoft/pyright/docs/errors/typeannotationnotsupported.md#chained)

That would jump to

rchiodo commented 1 year ago

@Heejaechang had another idea, give each diagnostic a unique ID so that users can actually search for these errors.

That would change the above to be something like:

Type annotation not supported. [More Info](https://github.com/microsoft/pyright/docs/errors/1110.md#chained)

erictraut commented 1 year ago

Creating a unique ID and documentation for every diagnostic message would be a very significant undertaking and would come with significant maintenance cost. Last time I counted, there were over 600 diagnostic messages. For those reasons, I would vote against that approach. I think some of the other ideas above are much more feasible.

rchiodo commented 1 year ago

Had a meeting with the typescript team. Here's a summary of what was discussed:

rchiodo commented 1 year ago

C# team had similar feedback: