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

Regression: error messages for literal expressions in invalid places are worse #5989

Closed
Michael0x2a opened this issue Dec 2, 2018 · 0 comments · Fixed by #6149
Closed

Regression: error messages for literal expressions in invalid places are worse #5989

Michael0x2a opened this issue Dec 2, 2018 · 0 comments · Fixed by #6149

Comments

@Michael0x2a
Copy link
Collaborator

Michael0x2a commented Dec 2, 2018

After #5947 (basic end-to-end support for literal types) lands, we'll start deferring some of the "invalid type" checks to the semantic analysis layer by wrapping them into a 'RawLiteralType'. This can, in some cases have an unintended side-effect of making some error messages worse. For example, writing T = TypeVar('T', bound=1) previously used to report error messages like "TypeVar 'bound' must be a type"; now they report error messages like E: Invalid type: try using Literal[1] instead?.

We ran into a similar case with 'NewType' -- in that case, we added some special-casing to restore the original error message. We could do the same thing here, and for other similar "type-alias-like" constructs such as NamedTuple or TypedDict.

Alternatively, we could explore adding TypeOfAny.invalid_type as Ivan suggests here -- this could also potentially help us resolve #4030 at the same time.

Note to self: if it looks like literal types aren't going to make it into the next mypy release, we should at least fix this error and/or change any existing errors to make sure we never recommend using 'Literal[...]'. It would be confusing if error messages started talking about literal types before they're actually officially released.

@Michael0x2a Michael0x2a self-assigned this Dec 2, 2018
Michael0x2a added a commit to Michael0x2a/mypy that referenced this issue Dec 31, 2018
This diff changes how we track raw literal types in the semantic
analysis phase. It makes the following changes:

1.  Removes the `RawLiteralType` synthetic type.

2.  Adds a new `TypeOfAny`: `TypeOfAny.invalid_type` as suggested
    in python#4030.

3.  Modifies `AnyType` so it can optionally contain a new
    `RawLiteral` class. This class contains information
    about the underlying literal that produced that particular
    `TypeOfAny`.

4.  Adjusts mypy to stop recommending using `Literal[...]` when
    doing `A = NewType('A', 4)` or `T = TypeVar('T', bound=4)`.

    (The former suggestion is a bad one: you can't create a NewType of a
    Literal[...] type. The latter suggestion is a valid but stupid one:
    `T = TypeVar('T', bound=Literal[4])` is basically the same thing as
    `T = Literal[4]`.)

    This resolves python#5989.

The net effect of this diff is that:

1.  RawLiteralTypes no longer leak during fine-grained mode,
    which should partially help unblock
    python#6075.

2.  The way mypy handles literal expressions in types is "inverted".

    Previously, we by default assumed literal expressions would belong
    inside `Literal[...]` and tacked on some logic to make them convert
    into error `AnyTypes`. Now, we do the reverse: we start with an
    error `AnyType` and convert those into `Literal[...]`s as needed.

    This more closely mirrors the way mypy *used* to work before we
    started work on Literal types. It should also hopefully help reduce
    some of the cognitive burden of working on other parts of the
    semantic analysis code, since we no longer need to worry about the
    `RawLiteralType` synthetic type.

3.  We now have more flexibility in how we choose to handle invalid
    types: since they're just `Anys`, we have more opportunities to
    intercept and customize the exact way in which we handle errors.

    Also see python#4030 for additional
    context. (This diff lays out some of the foundation work for that
    diff).
Michael0x2a added a commit to Michael0x2a/mypy that referenced this issue Jan 6, 2019
This commit modifies mypy so that the caller of TypeAnalyzer can
optionally suppress "Invalid type" related error messages.

This resolves python#5989: mypy will
now stop recommending using Literal[...] when doing
`A = NewType('A', 4)` or `T = TypeVar('T', bound=4)`.

(The former suggestion is a bad one: you can't create a NewType of a
Literal[...] type. The latter suggestion is a valid but stupid one:
`T = TypeVar('T', bound=Literal[4])` is basically the same thing as
doing just `T = Literal[4].
Michael0x2a added a commit that referenced this issue Jan 7, 2019
This pull request improves how we handle error messages with Literal types in
three different ways:

1. When the user tries constructing types like `Literal[3 + a]`, we now report
   an error message that says 'Invalid type: Literal[...] cannot contain
   arbitrary expressions'.

2. We no longer recommend using Literal[...] when doing `A = NewType('A', 4)`
   or `T = TypeVar('T', bound=4)`. This resolves 
   #5989.

   (The former suggestion is a bad one: you can't create a NewType of a
   Literal[...] type. The latter suggestion is a valid but stupid one: 
   `T = TypeVar('T', bound=Literal[4])` is basically the same thing as 
   `T = Literal[4]`.)

3. When the user tries using complex numbers inside Literals (e.g. 
  `Literal[3j]`), we now report an error message that says 'Parameter 1 
   of Literal[...] cannot be of type "complex"'. This is the same kind of 
   error message we previously used to report when the user tried using 
   floats inside of literals.

In order to accomplish bullet point 1, moved the "invalid type comment or
annotation" checks from the parsing layer to the semantic analysis layer.
This lets us customize which error message we report depending on whether
or not the invalid type appears in the context of a Literal[...] type.

In order to accomplish this, I repurposed RawLiteralType so it can represent
any arbitrary expression that does not convert directly into a type (and
renamed 'RawLiteralType' to 'RawExpressionType' to better reflect this new
usage). I also added an optional "note" field to that class: this lets the
parsing layer attach some extra context that would be difficult to obtain up
in the semantic analysis layer.

In order to accomplish bullet point 2, I modified the type analyzer so that
the caller can optionally suppress the error messages that would otherwise be
generated when a RawExpressionType appears outside of a Literal context.

Bullet point 3 only required a minor tweak to the parsing and error handling
code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant