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

Implement inference for JoinedStr and FormattedValue #2459

Conversation

ericvergnaud
Copy link
Contributor

Type of Changes

Type
🐛 Bug fix

Description

node.inferred() would return Uninferable if inference involved a JoinedStr.
JoinedStr involves FormattedValue so this is addressed for both classes in the same PR

@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.3.0 milestone Jul 8, 2024
@Pierre-Sassoulas Pierre-Sassoulas added Needs review 🔍 Needs to be reviewed by one or multiple more persons Work in progress and removed Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Jul 8, 2024
astroid/nodes/node_classes.py Show resolved Hide resolved
Comment on lines 4775 to 4776
if node is util.Uninferable:
result += "{Uninferable}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like we should exit early here and not continue to product "half" matching values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's philosophical, depending on the strategy for dealing with partially inferred collections of values

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help to make a decision about this if we could see a test case. At the moment, I do not think we should manufacture a constant that would never exist at runtime with the substring "Uninferable" in it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's "coincidental" coverage of this line in test_typing_extensions_types based on the current implementation of typing, but it would be better to have explicit coverage.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Jacob. I think Uninferable makes sense to those that have worked on astroid and pylint, but this will also be shown to users who might have no idea what it means. I would be in favour of exiting early and not creating partially correct values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

@ericvergnaud ericvergnaud Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is useful to know which fragment(s) of a JoinedStr cause inference failure, but I agree that half-baked strings can be puzzling.

@jacobtylerwalls jacobtylerwalls changed the title infer values for JoinedStr and FormattedValue Implement inference for JoinedStr and FormattedValue Jul 28, 2024
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing this feature!

astroid/nodes/node_classes.py Outdated Show resolved Hide resolved
astroid/nodes/node_classes.py Show resolved Hide resolved
Comment on lines 4775 to 4776
if node is util.Uninferable:
result += "{Uninferable}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help to make a decision about this if we could see a test case. At the moment, I do not think we should manufacture a constant that would never exist at runtime with the substring "Uninferable" in it.

astroid/nodes/node_classes.py Outdated Show resolved Hide resolved
tests/test_inference.py Show resolved Hide resolved
Comment on lines 4775 to 4776
if node is util.Uninferable:
result += "{Uninferable}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's "coincidental" coverage of this line in test_typing_extensions_types based on the current implementation of typing, but it would be better to have explicit coverage.

ChangeLog Outdated Show resolved Hide resolved
Eric Vergnaud added 3 commits August 1, 2024 20:31
…ue' of github.com:ericvergnaud/astroid into support-value-inference-for-joinedstr-and-formatted-value

# Conflicts:
#	ChangeLog
jacobtylerwalls
jacobtylerwalls previously approved these changes Aug 2, 2024
ChangeLog Outdated Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member

Great work !

@DanielNoord
Copy link
Collaborator

Oh, I just saw the failing tests. I do think we need to address it.

@Pierre-Sassoulas
Copy link
Member

It's failing on main too, we've merged multiple MR with 3.13 failing in the last week. (Due to a New beta of python 3.13). I think we could (should?) fix it before astroid 3.3.0 release.

@ericvergnaud
Copy link
Contributor Author

Oh, I just saw the failing tests. I do think we need to address it.

TBH I'm 100% sure that the failure is unrelated to this PR. The test simply calls as_string which is unchanged. Smells like an unrelated bug with 3.13-dev.

@jacobtylerwalls
Copy link
Member

It's fixed in the 3.13rc1, which may not be on GH yet.

@jacobtylerwalls
Copy link
Member

See #2478

@jacobtylerwalls jacobtylerwalls merged commit 3dbf139 into pylint-dev:main Aug 2, 2024
13 of 14 checks passed
@ericvergnaud ericvergnaud deleted the support-value-inference-for-joinedstr-and-formatted-value branch August 6, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants