-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Remove RawLiteralType synthetic type #6121
Conversation
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).
One meta-note: I think my "fix" of just removing the assert from the astmerge visitor in #6075 might have actually been fine: after analyzing the fine-grained logic, it seemed to me that encountering RawLiteralTypes during that phase of the update logic might have actually been expected behavior. However, I decided that it would just be easier to rip out RawLiteralType altogether instead of sitting down and formalizing this hunch. I do feel a little bad that this PR tacks on some additional cruft to AnyType, but we're already tracking a lot of provenance-related info in that class, so I felt this wasn't too extreme of a change. |
This seems too hacky to me. The new responsibility of Adding |
I am actually with Jukka here. Also, if we add # TODO: Would it be better to always return Any instead of UnboundType
# in case of an error? On one hand, UnboundType has a name so error messages
# are more detailed, on the other hand, some of them may be bogus.
return t |
Also, it would make sense to use else: # sym is None
if self.third_pass:
self.fail('Invalid type "{}"'.format(t.name), t)
return AnyType(TypeOfAny.from_error) # <- here |
The main problem with this approach is that I think adding just And if we do add some context to In any case, I'll look into just plugging the leak as well -- but I still think it's worth adding in some variation of this PR if we want to add |
Ok, after doing more poking around, I'm pretty confident that we're not actually leaking anything and that removing the assert is the from astmerge's In short, we're invoking this class within NodeReplaceVisitor.fixup_type. One of the places this function is called is inside NodeReplaceVisitor.process_base_func, which looks like this: def process_base_func(self, node: FuncBase) -> None:
self.fixup_type(node.type)
node.info = self.fixup(node.info)
if node.unanalyzed_type:
# Unanalyzed types can have AST node references
self.fixup_type(node.unanalyzed_type) It looks like |
@Michael0x2a as I mentioned in #6075, I am fine to merge it as is. So you can keep only the parts necessary to fix #5989 in this PR. Optionally, you can also fix the TODO item I mentioned above. |
(Also I would keep |
I think I'll just close this PR and submit a new one with the error message fixes. |
This diff changes how we track raw literal types in the semantic analysis phase. It makes the following changes:
Removes the
RawLiteralType
synthetic type.Adds a new
TypeOfAny
:TypeOfAny.invalid_type
as suggested in Better error message for "Invalid type" #4030.Modifies
AnyType
so it can optionally contain a newRawLiteral
class. This class contains information about the underlying literal that produced that particularTypeOfAny
.Adjusts mypy to stop recommending using
Literal[...]
when doingA = NewType('A', 4)
orT = 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 asT = Literal[4]
.)This resolves Regression: error messages for literal expressions in invalid places are worse #5989.
The net effect of this diff is that:
RawLiteralTypes no longer leak during fine-grained mode, which should partially help unblock Add tests for Literal types with incremental and fine-grained mode #6075.
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 errorAnyTypes
. Now, we do the reverse: we start with an errorAnyType
and convert those intoLiteral[...]
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.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 Better error message for "Invalid type" #4030 for additional context. (This diff lays out some of the foundation work for that diff).