Add some inference annotations #1887

nickdrozd · 2022-11-27T15:22:54Z

Type of Changes

	Type
✓	🔨 Refactoring

for more information, see https://pre-commit.ci

coveralls · 2022-11-27T15:26:23Z

Pull Request Test Coverage Report for Build 3700007049

27 of 27 (100.0%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.285%

Totals
Change from base Build 3678470532:	0.0%
Covered Lines:	9905
Relevant Lines:	10733

💛 - Coveralls

DanielNoord

Initial pass!

DanielNoord · 2022-11-27T15:28:52Z

astroid/decorators.py

@@ -89,15 +89,17 @@ def __get__(self, inst, objtype=None):
        return val


-def path_wrapper(func):
+def path_wrapper(func: Callable) -> Callable:


This should probably be a Typevar or even a ParamSpec.

Could you explain more? I haven't used those.

I think the PEP introducing ParamSpec is quite clear:
https://peps.python.org/pep-0612/

It explains its uses and those can be applied here.

That sounds out of scope for this PR. I can undo this change if you want.

Yeah, let's revert then

astroid/helpers.py

DanielNoord · 2022-11-27T15:30:36Z

astroid/inference.py

-def _infer_sequence_helper(node, context: InferenceContext | None = None):
+def _infer_sequence_helper(
+    node: _BaseContainerT, context: InferenceContext | None = None
+) -> list[nodes.NodeNG]:


Is node.elts correctly typed?

BaseContainer has self.elts: list[NodeNG] = []

Hmm that sounds weird. I need to investigate this but I'm fairly certain that it could be Instance as well.

I checked and this is likely incorrect. In the construction of Lists in DictModel.attr_values we use the keys of a Dict.items. Those are SuccessfulInferenceResults.
So elts is indeed incorrectly typed.

Should this change be undone?

Can't you change it to SuccessfulInferenceResult?

astroid/inference.py

DanielNoord · 2022-11-27T15:33:12Z

astroid/inference.py

-def _populate_context_lookup(call, context):
+def _populate_context_lookup(
+    call: nodes.Call, context: InferenceContext | None
+) -> dict[nodes.NodeNG, InferenceContext]:


This feels like it should be InferenceResult or something.

Which part?

The nodes.NodeNG

astroid/interpreter/dunder_lookup.py

astroid/nodes/_base_nodes.py

astroid/protocols.py

for more information, see https://pre-commit.ci

DanielNoord

Did another pass! Thanks for taking the time to do this, I know how difficult it is to get these right and PRs like this are really helpful to at least get the discussion and project going 😄

DanielNoord · 2022-11-29T22:28:43Z

astroid/decorators.py

-    def wrapped(node, context: InferenceContext | None = None, _func=func, **kwargs):
+    def wrapped(
+        node, context: InferenceContext | None = None, _func=func, **kwargs
+    ) -> Iterator:


Let's use Generator, since this function has return statements.

DanielNoord · 2022-11-29T22:30:23Z

astroid/helpers.py

@@ -219,7 +219,7 @@ def is_supertype(type1, type2) -> bool:
    return _type_check(type1, type2)


-def class_instance_as_index(node):
+def class_instance_as_index(node) -> nodes.Const | None:


Suggested change

def class_instance_as_index(node) -> nodes.Const | None:

def class_instance_as_index(node: bases.Instance) -> nodes.Const | None:

I did a quick check and this should work 😄

DanielNoord · 2022-11-29T22:36:42Z

astroid/inference.py

-def _infer_sequence_helper(node, context: InferenceContext | None = None):
+def _infer_sequence_helper(
+    node: _BaseContainerT, context: InferenceContext | None = None
+) -> list[nodes.NodeNG]:


I checked and this is likely incorrect. In the construction of Lists in DictModel.attr_values we use the keys of a Dict.items. Those are SuccessfulInferenceResults.
So elts is indeed incorrectly typed.

DanielNoord · 2022-11-29T22:37:28Z

astroid/inference.py

@@ -216,7 +218,7 @@ def _higher_function_scope(node: nodes.NodeNG) -> nodes.FunctionDef | None:
    while current.parent and not isinstance(current.parent, nodes.FunctionDef):
        current = current.parent
    if current and current.parent:
-        return current.parent  # type: ignore[return-value]
+        return current.parent


This causes an additional message.

DanielNoord · 2022-11-29T22:38:08Z

astroid/inference.py

@@ -743,7 +745,9 @@ def _bin_op(
    )


-def _get_binop_contexts(context, left, right):
+def _get_binop_contexts(


Let's do this in my BinOp PRs. I'll prepare those somewhere this week.

DanielNoord · 2022-11-29T22:39:14Z

astroid/inference.py

@@ -759,7 +763,7 @@ def _get_binop_contexts(context, left, right):
        yield new_context


-def _same_type(type1, type2):
+def _same_type(type1, type2) -> bool:


This is interesting as adding annotations to type1 will cause a type error... Hmm, we should probably investigate this.

This function is used twice. In both cases, the inputs can be InferenceResult | None, but this function assumes that it will be InferenceResult.

Proxy is involved too. Messy!

DanielNoord · 2022-11-29T22:40:54Z

astroid/protocols.py

@@ -349,7 +349,9 @@ def assend_assigned_stmts(
 nodes.AssignAttr.assigned_stmts = assend_assigned_stmts


-def _arguments_infer_argname(self, name, context):
+def _arguments_infer_argname(
+    self, name: str, context: InferenceContext


name can be None, see code down below.

pyproject.toml

DanielNoord · 2022-11-30T20:03:26Z

astroid/decorators.py

@@ -89,15 +89,17 @@ def __get__(self, inst, objtype=None):
        return val


-def path_wrapper(func):
+def path_wrapper(func: Callable) -> Callable:


Yeah, let's revert then

DanielNoord · 2022-11-30T20:04:20Z

astroid/inference.py

-def _infer_sequence_helper(node, context: InferenceContext | None = None):
+def _infer_sequence_helper(
+    node: _BaseContainerT, context: InferenceContext | None = None
+) -> list[nodes.NodeNG]:


Can't you change it to SuccessfulInferenceResult?

DanielNoord · 2022-11-30T20:04:46Z

astroid/inference.py

@@ -759,7 +763,7 @@ def _get_binop_contexts(context, left, right):
        yield new_context


-def _same_type(type1, type2):
+def _same_type(type1, type2) -> bool:


DanielNoord · 2022-11-30T20:06:06Z

astroid/inference.py

-def _populate_context_lookup(call, context):
+def _populate_context_lookup(
+    call: nodes.Call, context: InferenceContext | None
+) -> dict[InferenceResult, InferenceContext]:


Have you actually checked that this is InferenceResult and not SuccesfulInferenceResult?

Pierre-Sassoulas

Looks pretty good, there are a lot of fixes. I would be in favor of merging. We definitely need to activate mypy to be able to tell if something regressed without having to check locally by comparing the output before and after (I didn't do that so the approve is based on trust and the fact that actually verifying would take a lot of time.)

DanielNoord

I did verify this last time and all comments have been resolved! Thanks!

nickdrozd and others added 3 commits November 27, 2022 08:30

Cut unused type ignores

0bf9511

Add some type annotations in and around inference.py

99fdf0d

[pre-commit.ci] auto fixes from pre-commit.com hooks

cfc9a7b

for more information, see https://pre-commit.ci

DanielNoord requested changes Nov 27, 2022

View reviewed changes

Pierre-Sassoulas added the Maintenance Discussion or action around maintaining astroid or the dev workflow label Nov 28, 2022

nickdrozd and others added 5 commits November 28, 2022 08:49

Add str, fix typo

650c6e7

Use generator instead of iterator

a865372

Use Const

33ebfc6

Undo bin op infer types

c7b36a5

[pre-commit.ci] auto fixes from pre-commit.com hooks

e1a1218

for more information, see https://pre-commit.ci

DanielNoord self-requested a review November 29, 2022 17:03

DanielNoord requested changes Nov 29, 2022

View reviewed changes

Pierre-Sassoulas and others added 7 commits November 30, 2022 13:26

Merge branch 'main' into inference-annotations

d116e88

Ignore different warning

5aa7fac

Undo _get_binop_contexts

5d2204a

InferenceResult instead of NodeNG

2e719c4

MAke name optional

59e0921

Undo config changes

618cce5

Use Generator

ee4834f

DanielNoord requested changes Nov 30, 2022

View reviewed changes

nickdrozd added 3 commits December 14, 2022 18:45

Undo some changes

a007168

Use SuccessfulInferenceResult

70471a3

Merge branch 'main' into inference-annotations

48cbc18

Pierre-Sassoulas approved these changes Dec 15, 2022

View reviewed changes

DanielNoord approved these changes Dec 15, 2022

View reviewed changes

DanielNoord merged commit 583a279 into pylint-dev:main Dec 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some inference annotations #1887

Add some inference annotations #1887

nickdrozd commented Nov 27, 2022

coveralls commented Nov 27, 2022 •

edited

Loading

DanielNoord left a comment

DanielNoord Nov 27, 2022

nickdrozd Nov 28, 2022

DanielNoord Nov 29, 2022

nickdrozd Nov 30, 2022

DanielNoord Nov 30, 2022

DanielNoord Nov 27, 2022

nickdrozd Nov 28, 2022

DanielNoord Nov 29, 2022

DanielNoord Nov 29, 2022

nickdrozd Nov 30, 2022

DanielNoord Nov 30, 2022

DanielNoord Nov 27, 2022

nickdrozd Nov 28, 2022

DanielNoord Nov 29, 2022

DanielNoord left a comment

DanielNoord Nov 29, 2022

DanielNoord Nov 29, 2022

DanielNoord Nov 29, 2022

DanielNoord Nov 29, 2022

DanielNoord Nov 29, 2022

DanielNoord Nov 29, 2022

nickdrozd Nov 30, 2022

nickdrozd Nov 30, 2022

DanielNoord Nov 30, 2022

DanielNoord Nov 29, 2022

DanielNoord Nov 30, 2022

DanielNoord Nov 30, 2022

DanielNoord Nov 30, 2022

DanielNoord Nov 30, 2022

Pierre-Sassoulas left a comment

DanielNoord left a comment

	def class_instance_as_index(node) -> nodes.Const \| None:
	def class_instance_as_index(node: bases.Instance) -> nodes.Const \| None:

Add some inference annotations #1887

Add some inference annotations #1887

Conversation

nickdrozd commented Nov 27, 2022

Type of Changes

coveralls commented Nov 27, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3700007049

💛 - Coveralls

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

coveralls commented Nov 27, 2022 •

edited

Loading