Wrong inference for the str(const)#2995
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2995 +/- ##
==========================================
+ Coverage 93.27% 93.40% +0.12%
==========================================
Files 92 92
Lines 11348 11323 -25
==========================================
- Hits 10585 10576 -9
+ Misses 763 747 -16
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
|
||
| if call.positional_arguments: | ||
| try: | ||
| first_value = next(call.positional_arguments[0].infer(context=context)) |
There was a problem hiding this comment.
Why only use the first value? If we infer multiple values I don't think we should default to the first one.
There was a problem hiding this comment.
Other functions in builtin brain seems to do the same.
Do you mean infer_str should return an interator, e.g. inference result of str(unknown if unknown else "a") would be [Const(""), Const("a")] ?
There was a problem hiding this comment.
Other functions in builtin brain seems to do the same.
Unfortunately not all existing code is of the same quality 😅
As for the solution, I'm not sure to be honest. I think it would probably be best to only return a Const with a value if we can be 100% sure it is that value, and otherwise keep falling back.
Something like if all(v == first_value for v in node.infer(): value = first_value. If you get what I mean 😅
There was a problem hiding this comment.
Good solution. Added just that in the last commit.
| return nodes.Const("") | ||
|
|
||
| if isinstance(first_value, nodes.Const): | ||
| return nodes.Const(str(first_value.value)) |
There was a problem hiding this comment.
Does this face similar issues as fixed in #2971? 7**10000 to str will raise an exception
There was a problem hiding this comment.
Good point, I missed the str() may fail
DanielNoord
left a comment
DanielNoord
left a comment
There was a problem hiding this comment.
@Pierre-Sassoulas @jacobtylerwalls You want to review this? It LGTM!
jacobtylerwalls
left a comment
jacobtylerwalls
left a comment
There was a problem hiding this comment.
Thanks for the PR 👍
Can you add a changelog?
| if len(candidates) == 1: | ||
| return nodes.Const(candidates.pop()) |
There was a problem hiding this comment.
This seems like an unnecessary source of indeterminism.
| return fallback | ||
|
|
||
| try: | ||
| candidates.add(str(inferred.value)) |
There was a problem hiding this comment.
Should we only run str() if the value is a builtin? I wouldn't want a more involved str() implementation from user code to run (we should keep pylint static).
| str(2 + 2) #@ | ||
| str() #@ | ||
| str(int) #@ | ||
| str(2 if unknown() else 3) #@ |
There was a problem hiding this comment.
I think we should add a test ensuring user __str__ doesn't run:
class StrWillFail:
def __str__(self):
raise RuntimeError
str(StrWillFail()) #@3 participants
This PR fixes the wrong inference for the
str(a), then theais inferred to beConst.Closes #2994
I preserved returning
Const("")in other cases. Maybe it should raiseUseInferenceDefaultlike theinfer_intdoes ?