Support Negative Int Literal #7878
Conversation
mypy/plugins/default.py
Outdated
| This is mainly used to infer the return type as LiteralType | ||
| if the original underlying object is a LiteralType object | ||
| """ | ||
| if isinstance(ctx.type, Instance) and isinstance(ctx.type.last_known_value, LiteralType): |
There was a problem hiding this comment.
| if isinstance(ctx.type, Instance) and isinstance(ctx.type.last_known_value, LiteralType): | |
| if isinstance(ctx.type, Instance) and ctx.type.last_known_value is not None: |
mypy/plugins/default.py
Outdated
| value = ctx.type.last_known_value.value | ||
| fallback = ctx.type.last_known_value.fallback | ||
| if isinstance(value, int): | ||
| return LiteralType(-value, fallback) |
There was a problem hiding this comment.
You should return an Instance here with the last known value set to this literal, instead of the literal directly. This is what screws type inference and makes many tests fail.
There was a problem hiding this comment.
That sounds very reasonable. I patched this up in 473c93a but it fails at newly added test, showing that rhs of the assignment is still int type.
I also tried modifying ctx.type in place by changing ctx.type.last_known_value.value if Instance and ctx.type.value if LiteralType and then returning ctx.type directly, it fails the same.
There was a problem hiding this comment.
Hm, it looks like in both branches you need to return either instance or literal depending on context, essentially you need to copy logic from ExpressionChecker.infer_literal_expr_type(). For this however you must know type_context. It is not however a part of CheckerPluginInterface. It actually may be a useful addition, so I propose to add type_context there, and consequently make it a property on TypeChecker.
There was a problem hiding this comment.
I summary the message as below:
- add
type_contexttoCheckerPluginInterface - fill the missing logic so that
TypeCheckercorrectly has atype_context - copy logic from
ExpressionChecker.infer_literal_expr_type()to complete this task
If my understanding is correct, I will start working on it ASAP
There was a problem hiding this comment.
Yes.
(about the last bullet, copy the logic to your plugin)
There was a problem hiding this comment.
I took a quick look at checker.py and checkerexpr.py and got a little bit confused. The callback function returned from the plugin and thus has no access to the TypeChecker and the plugin passed in when constructing a TypeChecker directly goes into the ExpressionChecker and therefore I see no ways that will be able to check type_context inside the callback function without modifying its API. Please enlighten me
There was a problem hiding this comment.
Out of the three steps above which one you can't make?
There was a problem hiding this comment.
It's my misunderstanding. Now I think it's going to work, please check 32a99a2
mypy/checker.py
Outdated
| self.msg = MessageBuilder(errors, modules) | ||
| self.plugin = plugin | ||
| self.expr_checker = mypy.checkexpr.ExpressionChecker(self, self.msg, self.plugin) | ||
| self.type_context = self.expr_checker.type_context |
There was a problem hiding this comment.
Could you please make this a property, to make it clear it is not writeable.
mypy/plugin.py
Outdated
| options = None # type: Options | ||
| path = None # type: str | ||
| # Type context for type inference | ||
| type_context = None # type: List[Optional[Type]] |
There was a problem hiding this comment.
Here it should be an abstract property.
| value = ctx.type.value | ||
| fallback = ctx.type.fallback | ||
| if isinstance(value, int): | ||
| return LiteralType(value=-value, fallback=fallback) |
There was a problem hiding this comment.
Could you please also add this test:
ONE: Final = 1
TWO: Final = 2
err_code = -ONE
if bool():
err_code = -TWOThere was a problem hiding this comment.
Should I add it as a new test or append to testNegativeIntLiteralWithFinal in check-literal.test @ilevkivskyi
There was a problem hiding this comment.
I think appending to the existing test is better.
|
Thanks for the guidance! It’s a great education |
2 participants
This PR aims to support negative int Literal, resolves #7844
Currently, the newly added test will broke and show inconsistent output with directly invoking mypy from command lineCommit 278ec69 shows the inconsistent error output between running mypy from command line and running from tests
Commit 7b3d1e8 now behaves ideally for testcase like #7844. But it breaks current codebase and tests.