Create `UninferableBase` #1741

DanielNoord · 2022-08-15T18:02:09Z

Steps

For new features or bug fixes, add a ChangeLog entry describing what your PR does.
Write a good description on what the PR does.

Description

Closes #1680.

Type of Changes

	Type
✓	✨ New feature
✓	🔨 Refactoring

coveralls · 2022-08-15T18:05:07Z

Pull Request Test Coverage Report for Build 3503985614

17 of 17 (100.0%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.004%) to 92.292%

Totals
Change from base Build 3503977238:	0.004%
Covered Lines:	9878
Relevant Lines:	10703

💛 - Coveralls

Pierre-Sassoulas

I think this is a neat change, but I don't really like the name. I don't have particularly good proposal either, I'm thinking about UninferableType or TypeOfUninferable. Maybe astroid.typing.Uninferable and there is two Uninferablewith a different namespace ?

DanielNoord · 2022-08-15T20:01:30Z

Yeah the issue I had with that is that that doesn't really make sense a year from now. UninferableBase is essentially just a normal class which gets instantiated. Similar to NodeNGType vs. NodeNG. I thought it would be better to just use a "typeless" name, but I'm fine with still adding it.

Pierre-Sassoulas · 2022-08-15T20:05:21Z

Similar to NodeNGType vs. NodeNG

We have something similar already ? Maybe we could use the same scheme for consistency ?

We could maybe bother Marc. This is a little naming brain teaser that will affect a looooot of typing later and should not take him too long to give his opinion on.

DanielNoord · 2022-08-15T20:06:52Z

We have something similar already ? Maybe we could use the same scheme for consistency ?

No 🤣 I meant, normal classes also don't have Type at the end of their name even though they could be considered the "type" of all their instances 😄

We could maybe bother Marc. This is a little naming brain teaser that will affect a looooot of typing later and should not take him too long to give his opinion on.

I tagged him in the original issue. I was hesitant to retag here.

Pierre-Sassoulas · 2022-08-15T20:15:05Z

I meant, normal classes also don't have Type at the end of their name even though they could be considered the "type" of all their instances

Ha, all right. Yeah that's not very satisfying (actually it's the reason why I don't like the name, but the issue is not the name itself it's the way it's implemented that make the typing not work like every other typing in python).

The astroid.typing.Uninferable vs astroid.util.Uninferable might be a little hacky but the result would be named almost as if the class was working "normally", what do you htink ?

DanielNoord · 2022-08-15T20:18:41Z

The astroid.typing.Uninferable vs astroid.util.Uninferable might be a little hacky but the result would be named almost as if the class was working "normally", what do you htink ?

That's bound to cause issues somewhere down the line. I'm not even sure you can do:

from typing import Uninferable
Uninferable = Uninferable()

That seems like a naming clash.

Anyway, this has a similar issue as UninferableType has: typing.Uninferable doesn't make a lot of sense in a year time. It's not a type or TypeVar, it's class which gets instantiated into a "global constant". Perhaps InstantiateMePleaseUninferable? 😅

Pierre-Sassoulas · 2022-08-15T20:27:06Z

Yeah, that would be:

from typing import Uninferable as InstantiateMePleaseUninferable
Uninferable = InstantiateMePleaseUninferable()

Then in code:

import astroid
from astroid.utils import Uninferable

def myfunc(node: astroid.typing.Uninferable | nodes.NodeNG) -> None:
    if node is Uninferable:
        print(...)

DanielNoord · 2022-08-15T20:30:45Z

I don't think we should use the same name for different things (which is what the current issue basically boils down to) and I don't think it makes sense to put the class definition in the typing module. This is not necessarily a typing issue. You could also see it as a refactoring to make the class of Uninferable available to those who want to create more than a singleton 😄

Anyway, imo in a years time I think it makes the most sense to have a name for the base class that shows that it is a base class rather than a name that shows where the refactoring effort originated from.

Pierre-Sassoulas · 2022-08-15T20:45:24Z

Yeah I don't have a good solution, maybe there isn't one;

cdce8p · 2022-08-19T18:18:19Z

Came across something similar recently, in the cpython/dataclasses.py module.

...
class _MISSING_TYPE:
    pass
MISSING = _MISSING_TYPE()
...

This is how they define sentinel values. That doesn't solve the typing issue though which got me thinking.
Instead of writing UninferableBase or _Uninferable, it should work with type[Uninferable]. Which is the annotation we use already.

I.e. the change would just be to remove the @object.__new__ magic, rename Uninferable to _Uninferable and create a sentinel object Uninferable = _Uninferable() in the same file. Haven't tested it yet, but I think it should work.

DanielNoord · 2022-08-19T18:51:50Z

Yeah that should work but I was just thinking about two three years from now.
What's the most logical name? Is that something with an underscore? Or something with type or base in it. I don't care too much but I don't want to make a choice that seems logical now but makes no sense in two years time

cdce8p · 2022-08-19T18:58:15Z

What's the most logical name? Is that something with an underscore? Or something with type or base in it. I don't care too much but I don't want to make a choice that seems logical now but makes no sense in two years time

The underscore has the advantage that it's (somewhat) private and we can change it if we want.
Besides that, I think the beauty is that users will only ever deal with one -> Uninferable. So there isn't any confusions. Either you need the sentinel value or wrap it in type for typing.

DanielNoord · 2022-08-21T10:25:27Z

@cdce8p Note that both pyright and mypy still don't really like using type and the is Uninferable type guarding.
See both:

from __future__ import annotations


class _MyClass:
    ...


MyClass = _MyClass()


def f(x: _MyClass | str) -> None:
    if x is MyClass:
        reveal_type(x)
    else:
        reveal_type(x)

❯ mypy test.py --strict
test.py:13: note: Revealed type is "Union[test._MyClass, builtins.str]"
test.py:15: note: Revealed type is "Union[test._MyClass, builtins.str]"
Success: no issues found in 1 source file

And:

from __future__ import annotations


class _MyClass:
    ...


MyClass = _MyClass()


def f(x: type[MyClass] | str) -> None:
    if x is MyClass:
        reveal_type(x)
    else:
        reveal_type(x)

❯ mypy test.py --strict
test.py:11: error: Variable "test.MyClass" is not valid as a type  [valid-type]
test.py:11: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
test.py:12: error: Non-overlapping identity check (left operand type: "Union[Type[MyClass?], str]", right operand type: "_MyClass")  [comparison-overlap]
test.py:13: note: Revealed type is "Union[Type[MyClass?], builtins.str]"
test.py:15: note: Revealed type is "Union[Type[MyClass?], builtins.str]"
Found 2 errors in 1 file (checked 1 source file)

In the last example pyright also complains about "Illegal type annotation: variable not allowed unless it is a type alias".

I might be missing something here but I think this resembles the potential situation with _Uninferable and Uninferable.

jacobtylerwalls · 2022-09-04T11:46:57Z

astroid/util.py


-    def __repr__(self):
+    This is meant to be used as a singleton. Use util.Uninferable to access it.


Would we rather point to astroid.Uninferable, which is where pylint usually accesses it?

DanielNoord · 2023-02-05T12:50:10Z

I don't think there is a real good solution here, but it would be really nice to get this merged and start working on the migration to UninferableBase in pylint as well.

I'm open to other preferences as I don't really have a preference myself but I do think we should bite the bullet and choose one solution.

jacobtylerwalls

I think this is the best approach. It's unfortunate that the sentinel is title-cased (instead of UNINFERABLE), but it's not worth changing this point.

One small suggestion to avoid exporting UninferableBase from the module.

jacobtylerwalls · 2023-02-05T17:36:11Z

astroid/__init__.py

@@ -180,7 +180,7 @@

 # isort: on

-from astroid.util import Uninferable
+from astroid.util import Uninferable, UninferableBase


I would probably skip adding this here.

This is "necessary" for pylint I think, but perhaps also a good question to ask now.
What do we want to do in pylint use node: UninferableBase or node: type[Uninferable]? Some goes for checking node is Uninferable or isinstance(node, UninferableBase).

Definitely UninferableBase. You pointed out above the trouble with type[Uninferable]. (The pyright dev said it was straight wrong.)

Some goes for checking node is Uninferable or isinstance(node, UninferableBase)?

Either. The latter is safer against copying/unpickling.

This is "necessary" for pylint I think

I think I'm missing this part; do you mind spelling it out? It could be imported from astroid.util instead of astroid.

Ah yeah, but I thought we put all things that are public as classes directly in astroid. But I do prefer importing from util myself so I'll remove this.

Ah yeah, but I thought we put all things that are public as classes directly in astroid.

I think it's a totally random and difficult to maintain snapshot of shortcuts. I'd like to stop adding to it, if possible:

>>> import astroid >>> astroid.PY310_PLUS False >>> astroid.PY311_PLUS Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: module 'astroid' has no attribute 'PY311_PLUS'

importing from astroid directly has the advantage that we can refactor everything the way we want without breaking the API. And astroid has some really nasty construct and import cycle internally that makes this valuable.

DanielNoord · 2023-02-05T20:45:34Z

I did the clean up of the code base in this PR as well.
I must say, I saw a lot of places that could be made cleaner now that we use isinstance checks but I'll leave that for another time. For now the additional intellisense that pyright gives is a big plus!

Pierre-Sassoulas

I have no strong opinion about this so I trust the decision taken without re-reading the whole discussion.

jacobtylerwalls · 2023-02-05T20:51:53Z

I think Däniel's solution is the same as Marc's suggested here, just not using the underscore. (Which makes sense, it has to be public at least to pylint to let pylint use it as a type.)

There was some more discussion about typing that got us sidetracked, but what's being implemented in this PR is the easy to understand, classic way to type.

codecov · 2023-02-05T20:53:51Z

Codecov Report

Merging #1741 (f022df8) into main (bcaecce) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1741      +/-   ##
==========================================
+ Coverage   92.74%   92.76%   +0.01%     
==========================================
  Files          94       94              
  Lines       10962    10956       -6     
==========================================
- Hits        10167    10163       -4     
+ Misses        795      793       -2

Flag	Coverage Δ
linux	`92.52% <100.00%> (+0.01%)`	⬆️
pypy	`88.46% <100.00%> (+0.01%)`	⬆️
windows	`92.36% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
astroid/arguments.py	`95.89% <100.00%> (ø)`
astroid/bases.py	`88.53% <100.00%> (ø)`
astroid/brain/brain_builtin_inference.py	`91.54% <100.00%> (ø)`
astroid/brain/brain_dataclasses.py	`94.28% <100.00%> (ø)`
astroid/brain/brain_functools.py	`98.66% <100.00%> (ø)`
astroid/brain/brain_namedtuple_enum.py	`92.98% <100.00%> (ø)`
astroid/brain/brain_typing.py	`88.43% <100.00%> (-0.08%)`	⬇️
astroid/builder.py	`94.11% <100.00%> (ø)`
astroid/constraint.py	`100.00% <100.00%> (ø)`
astroid/helpers.py	`93.71% <100.00%> (ø)`
... and 9 more

DanielNoord added Bug 🪳 Maintenance Discussion or action around maintaining astroid or the dev workflow labels Aug 15, 2022

DanielNoord added this to the 2.13.0 milestone Aug 15, 2022

Pierre-Sassoulas reviewed Aug 15, 2022

View reviewed changes

Pierre-Sassoulas approved these changes Aug 15, 2022

View reviewed changes

jacobtylerwalls reviewed Sep 4, 2022

View reviewed changes

DanielNoord mentioned this pull request Sep 29, 2022

Create ContextManagerModel and let GeneratorModel inherit #1810

Merged

2 tasks

Pierre-Sassoulas modified the milestones: 2.13.0, 2.14.0 Nov 19, 2022

cdce8p modified the milestones: 2.14.0, 2.15.0 Jan 30, 2023

DanielNoord requested a review from jacobtylerwalls February 5, 2023 12:49

jacobtylerwalls approved these changes Feb 5, 2023

View reviewed changes

Create UninferableBase

f022df8

DanielNoord force-pushed the uninferable branch from 1b758c6 to f022df8 Compare February 5, 2023 20:44

DanielNoord requested a review from jacobtylerwalls February 5, 2023 20:44

DanielNoord requested a review from Pierre-Sassoulas February 5, 2023 20:44

jacobtylerwalls approved these changes Feb 5, 2023

View reviewed changes

Pierre-Sassoulas approved these changes Feb 5, 2023

View reviewed changes

DanielNoord merged commit eb711d2 into pylint-dev:main Feb 5, 2023

DanielNoord deleted the uninferable branch February 5, 2023 20:54

DanielNoord mentioned this pull request Mar 5, 2023

Use UninferableBase instead of Uninferable pylint-dev/pylint#8384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `UninferableBase` #1741

Create `UninferableBase` #1741

DanielNoord commented Aug 15, 2022

coveralls commented Aug 15, 2022 •

edited

Loading

Pierre-Sassoulas left a comment •

edited

Loading

DanielNoord commented Aug 15, 2022

Pierre-Sassoulas commented Aug 15, 2022

DanielNoord commented Aug 15, 2022

Pierre-Sassoulas commented Aug 15, 2022

DanielNoord commented Aug 15, 2022

Pierre-Sassoulas commented Aug 15, 2022

DanielNoord commented Aug 15, 2022

Pierre-Sassoulas commented Aug 15, 2022

cdce8p commented Aug 19, 2022

DanielNoord commented Aug 19, 2022

cdce8p commented Aug 19, 2022

DanielNoord commented Aug 21, 2022 •

edited

Loading

jacobtylerwalls Sep 4, 2022

DanielNoord commented Feb 5, 2023

jacobtylerwalls left a comment

jacobtylerwalls Feb 5, 2023

DanielNoord Feb 5, 2023

jacobtylerwalls Feb 5, 2023

DanielNoord Feb 5, 2023

jacobtylerwalls Feb 5, 2023

Pierre-Sassoulas Feb 5, 2023

DanielNoord commented Feb 5, 2023

Pierre-Sassoulas left a comment

jacobtylerwalls commented Feb 5, 2023

codecov bot commented Feb 5, 2023


		def __repr__(self):
		This is meant to be used as a singleton. Use util.Uninferable to access it.

Create UninferableBase #1741

Create UninferableBase #1741

Conversation

DanielNoord commented Aug 15, 2022

Steps

Description

Type of Changes

coveralls commented Aug 15, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3503985614

💛 - Coveralls

Pierre-Sassoulas left a comment • edited Loading

Choose a reason for hiding this comment

DanielNoord commented Aug 15, 2022

Pierre-Sassoulas commented Aug 15, 2022

DanielNoord commented Aug 15, 2022

Pierre-Sassoulas commented Aug 15, 2022

DanielNoord commented Aug 15, 2022

Pierre-Sassoulas commented Aug 15, 2022

DanielNoord commented Aug 15, 2022

Pierre-Sassoulas commented Aug 15, 2022

cdce8p commented Aug 19, 2022

DanielNoord commented Aug 19, 2022

cdce8p commented Aug 19, 2022

DanielNoord commented Aug 21, 2022 • edited Loading

jacobtylerwalls Sep 4, 2022

Choose a reason for hiding this comment

DanielNoord commented Feb 5, 2023

jacobtylerwalls left a comment

Choose a reason for hiding this comment

jacobtylerwalls Feb 5, 2023

Choose a reason for hiding this comment

DanielNoord Feb 5, 2023

Choose a reason for hiding this comment

jacobtylerwalls Feb 5, 2023

Choose a reason for hiding this comment

DanielNoord Feb 5, 2023

Choose a reason for hiding this comment

jacobtylerwalls Feb 5, 2023

Choose a reason for hiding this comment

Pierre-Sassoulas Feb 5, 2023

Choose a reason for hiding this comment

DanielNoord commented Feb 5, 2023

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

jacobtylerwalls commented Feb 5, 2023

codecov bot commented Feb 5, 2023

Codecov Report

Create `UninferableBase` #1741

Create `UninferableBase` #1741

coveralls commented Aug 15, 2022 •

edited

Loading

Pierre-Sassoulas left a comment •

edited

Loading

DanielNoord commented Aug 21, 2022 •

edited

Loading