astroid icon indicating copy to clipboard operation
astroid copied to clipboard

Make ``Arguments.defaults`` ``None`` for uninferable signatures

Open DanielNoord opened this issue 3 years ago • 7 comments

Steps

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

Description

This is a breaking change.

However, Arguments.args is already None for uninferable live objects. So I think there should already be an if node.args is None: continue line in most use-cases of this node.

The ultimate benefit of this is #1593 which allows us to get info about calls to int.__dir__ etc. All C objects that have signatures that inspect can find will now become readable by us.

Type of Changes

Type
:sparkles: New feature

Related Issue

Ref https://github.com/PyCQA/astroid/pull/1593.

DanielNoord avatar Jun 05 '22 10:06 DanielNoord

Pull Request Test Coverage Report for Build 2442772814

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 91.892%

Totals Coverage Status
Change from base Build 2442630334: 0.004%
Covered Lines: 9260
Relevant Lines: 10077

💛 - Coveralls

coveralls avatar Jun 05 '22 10:06 coveralls

@cdce8p I didn't want to bother you as there were more pressing PRs which I liked you to review. But if you have the time I would appreciate your opinion on this change. It's certainly non-trivial, but I'm not sure if there is a better way to resolve this (see OP).

DanielNoord avatar Jun 05 '22 21:06 DanielNoord

@cdce8p I didn't want to bother you as there were more pressing PRs which I liked you to review. But if you have the time I would appreciate your opinion on this change. It's certainly non-trivial, but I'm not sure if there is a better way to resolve this (see OP).

Tbh I haven't followed the discussion around this so far. High level, what is the benefit of setting default to None instead of an empty list? Isn't it enough that args is already None?

cdce8p avatar Jun 05 '22 21:06 cdce8p

@cdce8p I didn't want to bother you as there were more pressing PRs which I liked you to review. But if you have the time I would appreciate your opinion on this change. It's certainly non-trivial, but I'm not sure if there is a better way to resolve this (see OP).

Tbh I haven't followed the discussion around this so far. High level, what is the benefit of setting default to None instead of an empty list? Isn't it enough that args is already None?

Ultimately the objective is to merge https://github.com/PyCQA/astroid/pull/1593. This doesn't only give us information about arguments of C-extensions but also about all live objects in builtins.

However, there are still objects which we can't fully infer. Since we currently don't do any inference of arguments on such live objects and just set args to None this doesn't really matter. However, without these changes we start to miss crucial data for some of our pylint checks: If we don't infer arguments for live objects we won't report missing arguments. If we do infer arguments, but don't infer defaults we will start to raise warnings about missing arguments because we don't see that there are default values.

TLDR: Before merging #1593 we need to prepare how we build live objects to discern between data we can't find and data which is just []. Otherwise the partial inference added in #1593 will break stuff.

Note that #1593 might still break stuff because it seems like pylint currently depends on these live objects not being inferred. That's an issue for that PR though I think.

DanielNoord avatar Jun 05 '22 22:06 DanielNoord

Just for me to understand The ultimate goal is to infer the args which are None currently? And subsequently, if the args are infered, we need to know that defaults need to be inferred too. Otherwise we might emit false-positives if it's just always an empty list.

-- It might be best to postpone this PR until after the 2.12.0 release. At the moment, I'm not fully certain it won't break something.

cdce8p avatar Jun 05 '22 22:06 cdce8p

Just for me to understand The ultimate goal is to infer the args which are None currently? And subsequently, if the args are infered, we need to know that defaults need to be inferred too. Otherwise we might emit false-positives if it's just always an empty list.

Some of them yeah. As long as they have a signature that can be read by inspect. Yes to the second question!

-- It might be best to postpone this PR until after the 2.12.0 release. At the moment, I'm not fully certain it won't break something.

Fine with me!

DanielNoord avatar Jun 05 '22 22:06 DanielNoord

I'm wondering if we could return Uninferable instead of None when the signature and the default value are uninferable ? It seems to be more semantic. Is it because None is already handled elsewhere to avoid breaking changes ?

Yeah we handle None both in astroid and pylint. So changing that would be a breaking change.

DanielNoord avatar Jun 15 '22 06:06 DanielNoord