Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Code looks good to me.
cc @markshannon @JelleZijlstra

This fix looks correct, but I doubt about the need of two always equal values in this case.

@efimov-mikhail Are you referring to the output of dis.dis here? Would that now ideally be changed as part of this PR?

All commit authors signed the Contributor License Agreement.

Maybe we could add something like this if instr.argrepr and instr.argrepr != str(instr.arg)
to Formatter.print_instruction_line. But I want to hear another opinions.

CC @iritkatriel @picnixz

I do not think that it is useful if we are just repeating the same value.

Maybe we could add something like this if instr.argrepr and instr.argrepr != str(instr.arg)

In this case, it could be confusing if the argstr looks almost the same. I am also unsure whether it is possible to have that difference except with quoting differences.

Of course, you're right, @picnixz.
I think a little more, and IMO it will be better to just change argrepr for LOAD_SMALL_INT and change Formatter.print_instruction_line only for this instruction.

@davep, Could you please add this extra condition for LOAD_SMALL_INT and change the tests properly?

Wait. How dou you suggest changing argrepr? because for me even this format is fine actually. I am not even sure that the condition "argrepr != str(self.arg)" would be useful. When does it hold?

Or do you mean to store argrepr but not rendering it? because in this case, I would prefer a generic check saying whether the instruction should have its repr rendered if the value is identical.

I am on mobile now so t is hard to check but the check should not be restricted to LOADSMALLINT. What about other instructions that load constants? do they render both their values and args as well? do they have a filled argrepr? (is there another instruction in the first place ?)

@picnixz We discussed this with @efimov-mikhail, and we proposed making this change only for LOAD_SMALL_INT in this PR. And do the generic check in a follow-up one.

This way, we can backport the PR more easily.

I still don't think this is a relevant change. LOAD_SMALL_INT is self expressive. It loads a small int and the value being that integer. When do we need to actually show its repr? What would be the rendered output? or do you mean to just populate argrepr in the structure without showing it?

At best, I agree that argrepr could be populated, but not necessarily rendered. However, my questions are still unanswered:

• Are there other instructions that have empty argrepr (whether they render both value and repr or not)?
• For instructions with argrepr and argvalue set, do they show them both when it's unclear?

do you mean to just populate argrepr in the structure without showing it?

Yes, I meant this.

However, my questions are still unanswered:

Yes, I propose doing it in follow-up issues and PRs.
First, perform an analysis to check if there are any instructions with an empty argrepr.
Second, if so, create a PR and fix them.
Third, check if we have instructions where the argvalue and the argrepr are the same, and do not render the argrepr in the output, in a separate PR.

These are small steps, but I think we can take them one by one.

I prefer we first answer those questions before doing any change in LOAD_SMALL_INT. I don't like specializing for this specific instruction if other instructions that also have missing argrepr behave differently. So, please, check first which instructions have empty argrepr and let's decide afterwards which ones should be rendered.

Then we can fill those that need to be filled and hide them in the output in one PR. Because deciding whether to hide the output may also be a generic function that could later be used to determine whether to show those with argvalue == argrepr.

So there's a lot of conversation here but I'm not clear on the conclusion, if there is one. Could someone possibly clearly say what I can do to improve this fix, if anything?

Actually, after reading the code (now that I'm on my laptop), I see that _opcode.has_const(_opcode_metadata.opmap["LOAD_SMALL_INT"]) returns False, hence the "bug" and only LOAD_SMALL_INT has some special handling.

However, _get_const_info calls _get_const_value which itself checks whether the op has constants or if it's LOAD_SMALL_INT. So, I think we should indeed set it because argrepr is not necessarily set on the (raw) instruction itself (the instruction doesn't have a constant as its argument but it can be deduced).

We have other places where we do op in hasconst or op == LOAD_SMALL_INT (e.g., in find_imports or in the assert of _get_const_value. So I would suggest the following:

• Keep what you did. Even if we duplicate the output for now.
• Add a helper (private) function that checks if it has constants, e.g., _hasconst_like() which checks whether op is in hasconst or op == LOAD_SMALL_INT and replace the checks with that method. If we decide to change the meaning of "having a constant" later this would help a lot.
• I'll decide then whether we can modify print_instruction_line to remove the duplicated output. An alternative would be to have some field on instr that says "hide duplicated fields" and then we can use this in the formatter.

For instance:

class Instruction(_Instruction):
   
   ...
   

   @property
   def _simplify_repr(self):
       return self.baseopcode == LOAD_SMALL_INT

and then in the formatter, we would only print the argrepr if _simplify_repr is False.

Thanks @picnixz, that's exactly the clarity/summary I was looking for. Very much appreciated.

@davep Is there anything left to do?

@sergey-miryanov I don't believe so. As best as I can tell I took on board the feedback and made the relevant changes. Unless anyone else has any input I think it's good to go from my point of view.

@picnixz Could you please take a look when you have time?