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

Support Negative Int Literal #7878

Merged
merged 7 commits into from Nov 6, 2019

Conversation

@TH3CHARLie
Copy link
Contributor

TH3CHARLie commented Nov 5, 2019

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 line

Commit 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.

Copy link
Collaborator

ilevkivskyi left a comment

Thanks, looks like a god start, I have some action items for you.

test-data/unit/check-literal.test Show resolved Hide resolved
test-data/unit/check-literal.test Show resolved Hide resolved
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):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 5, 2019

Collaborator
Suggested change
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:
value = ctx.type.last_known_value.value
fallback = ctx.type.last_known_value.fallback
if isinstance(value, int):
return LiteralType(-value, fallback)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 5, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@TH3CHARLie

TH3CHARLie Nov 5, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 5, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@TH3CHARLie

TH3CHARLie Nov 5, 2019

Author Contributor

I summary the message as below:

  • add type_context to CheckerPluginInterface
  • fill the missing logic so that TypeChecker correctly has a type_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

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 5, 2019

Collaborator

Yes.

(about the last bullet, copy the logic to your plugin)

This comment has been minimized.

Copy link
@TH3CHARLie

TH3CHARLie Nov 5, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 5, 2019

Collaborator

Out of the three steps above which one you can't make?

This comment has been minimized.

Copy link
@TH3CHARLie

TH3CHARLie Nov 6, 2019

Author Contributor

It's my misunderstanding. Now I think it's going to work, please check 32a99a2

mypy/plugins/default.py Show resolved Hide resolved
@TH3CHARLie TH3CHARLie requested a review from ilevkivskyi Nov 5, 2019
Copy link
Collaborator

ilevkivskyi left a comment

Thanks for updates, this looks almost ready! I have few minor comments.

@@ -219,6 +219,7 @@ def __init__(self, errors: Errors, modules: Dict[str, MypyFile], options: Option
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

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 6, 2019

Collaborator

Could you please make this a property, to make it clear it is not writeable.

@@ -215,6 +215,8 @@ class CheckerPluginInterface:
msg = None # type: MessageBuilder
options = None # type: Options
path = None # type: str
# Type context for type inference
type_context = None # type: List[Optional[Type]]

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 6, 2019

Collaborator

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)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 6, 2019

Collaborator

Could you please also add this test:

ONE: Final = 1
TWO: Final = 2

err_code = -ONE
if bool():
    err_code = -TWO

This comment has been minimized.

Copy link
@TH3CHARLie

TH3CHARLie Nov 6, 2019

Author Contributor

Should I add it as a new test or append to testNegativeIntLiteralWithFinal in check-literal.test @ilevkivskyi

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 6, 2019

Collaborator

I think appending to the existing test is better.

Copy link
Collaborator

ilevkivskyi left a comment

Great, thanks!

@ilevkivskyi ilevkivskyi merged commit 8010414 into python:master Nov 6, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@TH3CHARLie

This comment has been minimized.

Copy link
Contributor Author

TH3CHARLie commented Nov 6, 2019

Thanks for the guidance! It’s a great education

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.