Make ``Arguments.defaults`` ``None`` for uninferable signatures
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.
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 | |
|---|---|
| Change from base Build 2442630334: | 0.004% |
| Covered Lines: | 9260 |
| Relevant Lines: | 10077 |
💛 - 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).
@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 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
defaulttoNoneinstead of an empty list? Isn't it enough thatargsis alreadyNone?
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.
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.
Just for me to understand The ultimate goal is to infer the
argswhich areNonecurrently? And subsequently, if theargsare infered, we need to know thatdefaultsneed 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.0release. At the moment, I'm not fully certain it won't break something.
Fine with me!
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.