gh-96461: clarify the meaning of the oparg for CACHE and COPY opcode #96462

MatthieuDartiailh · 2022-08-31T11:24:35Z

This could be backported to Python 3.11

Issue: CACHE and SWAP documentation is vague with the meaning of the oparg #96461

Doc/library/dis.rst

iritkatriel · 2022-09-01T13:20:48Z

Doc/library/dis.rst

@@ -398,13 +401,17 @@ The Python compiler currently generates the following bytecode instructions.
   Push the *i*-th item to the top of the stack. The item is not removed from its
   original location.

+   The stack is indexed from 1, so COPY 1 will copy TOS.


Maybe instead of adding this, we could define TOS(i) at the top along with TOS1, TOS2 etc, and then reword the definition here to use those.

I tried something along those lines in my last commit let me know if you like it better this way.

iritkatriel · 2022-09-01T14:18:06Z

Doc/library/dis.rst

@@ -382,6 +382,10 @@ The Python compiler currently generates the following bytecode instructions.

 **General instructions**

+In the following, TOS(i) refer to the *i*-th item on the stack which is index from 1.


Suggested change

In the following, TOS(i) refer to the *i*-th item on the stack which is index from 1.

In the following, TOS(i) refers to the *i*-th item on the stack which is indexed from 1.

iritkatriel · 2022-09-01T14:22:18Z

Doc/library/dis.rst

@@ -382,6 +382,10 @@ The Python compiler currently generates the following bytecode instructions.

 **General instructions**

+In the following, TOS(i) refer to the *i*-th item on the stack which is index from 1.
+We also use as shorthand TOS = TOS(1) which is the top-of-stack.
+TOS1, TOS2, TOS3 are the second, third and fourth items on the stack, respectively.


It's a bit unfortunate that TOS1 = TOS(2), TOS2 = TOS(3), etc.

@markshannon - would you like to change this?

This was another reason for my previous formulation that avoided ro shed too much light on this.

Generally we count from one for stack items, so I would like to change it. Ideally drop TOS1 etc, all together.
The current explanation is not explicit about what is popped or not, so the shorthand is not helpful.

markshannon · 2022-10-03T19:25:29Z

Doc/library/dis.rst

@@ -395,15 +399,15 @@ The Python compiler currently generates the following bytecode instructions.

 .. opcode:: COPY (i)

-   Push the *i*-th item to the top of the stack. The item is not removed from its
+   Push TOS(i) to the top of the stack. The item is not removed from its


Maybe "Push a copy of ith item (counting from 1) to the top of the stack ..."

markshannon · 2022-10-03T19:33:59Z

Doc/library/dis.rst

   original location.

   .. versionadded:: 3.11


 .. opcode:: SWAP (i)

-   Swap TOS with the item at position *i*.
+   Swap TOS with TOS(i).


Maybe "Swap the first and ith item on the stack (counting from 1).

MatthieuDartiailh · 2022-10-05T07:31:51Z

I can rephrase everything to not use TOS(i) and repeat that the stack is indexed from 1 in each relevant place however they are quite a few occurrences (many more than what this PR currently does).

@iritkatriel do you agree with @markshannon on this ?

iritkatriel · 2022-10-05T07:52:09Z

I can rephrase everything to not use TOS(i) and repeat that the stack is indexed from 1 in each relevant place however they are quite a few occurrences (many more than what this PR currently does).

@iritkatriel do you agree with @markshannon on this ?

I agree that we don't need to have both TOS(i) and TOSi, I think removing the TOSis is the smaller change, amirite?

@markshannon - were you suggesting to add "(counting from 1)" on each place where TOS(i) is used? Or just once where it's defined or used for the first time?

MatthieuDartiailh · 2022-10-05T08:33:12Z

Removing the TOSi should be a smaller change yes and would not entail any large duplication.

MatthieuDartiailh · 2022-10-06T19:43:03Z

I went ahead and replaced TOS by TOS(1)

I am not sure this a clear win for the reader TBH. Let me know what you think.

sweeneyde · 2022-10-07T00:25:54Z

I don't know if this has already been considered, but one approach might be to eliminate "TOS" terminology altogether and instead refer to stack[-1], stack[-2], stack.pop(), etc., borrowing the well-understood semantics of lists.

POP_TOP becomes "Remove stack[-1], i.e., do stack.pop()"
COPY becomes stack.append(stack[-i]).
SWAP becomes stack[-1], stack[-i] = stack[-i], stack[-1]
UNARY_POSITIVE becomes stack[-1] = +stack[-1]
BINARY_SUBSCR becomes right = stack.pop(); left = stack.pop(); stack.append(left[right]), or similar.

What do you think?

MatthieuDartiailh · 2022-10-07T07:15:06Z

That may lead to clearer description indeed. I must say I first tried to make the changes minimal but I can undertake a larger changes if it is generallyvthough to be useful.

MatthieuDartiailh · 2022-11-08T08:55:25Z

I went ahead with @sweeneyde suggestion and also fixed a bunch of small issues in separate commits (let me know if they should be split into their own PR). I will wait to get some feedback before dealing with the merge conflict.

Doc/library/dis.rst

iritkatriel · 2022-12-22T14:54:35Z

I think @sweeneyde 's suggestion is good.

MatthieuDartiailh · 2022-12-22T14:56:24Z

Thanks @iritkatriel. I will incorporate your changes and work on fixing the conflicts.

ghost · 2022-12-22T15:05:22Z

All commit authors signed the Contributor License Agreement.

gvanrossum

I'm honestly not sure why you hate the term "TOS" so much. It's short, and doesn't go into the (irrelevant) issue whether the stack grows up or down.

gvanrossum · 2022-12-22T15:47:02Z

Doc/library/dis.rst

+   Push the i-th item to the top of the stack without removing it from its original
+   location.::
+
+      STACK.append(STACK[-i])


Add that i should not be zero.

MatthieuDartiailh · 2022-12-22T19:23:48Z

I have no issue with the term TOS. I am simply looking for the best way to document opcode operating on the i-th element of the stack. Ideally I would like whatever is picked to be easy to remember and ideally consistent across the documentation. I like STACK[-i] because to me at least it makes it very obvious that the indexing starts at 1. But I would happily make whichever changes are believed to be best.

MatthieuDartiailh · 2022-12-23T14:47:06Z

I rebased this and forced pushed.

There are places where using TOS instead of STACK[-1] may be more readable and make this change less disruptive but it would hurt uniformity. @iritkatriel @gvanrossum let me know which version you prefer and I will make the relevant changes

markshannon

I really like describing the stack as a Python list, it is unambiguous and clear to any Python developer.

However, you need to be careful to accounts for pops, pushes and sequencing.

E.g. LIST_APPEND is
list.append(STACK[-i-1], STACK.pop())
not
list.append(STACK[-i], STACK[-1])

markshannon · 2022-12-23T14:56:57Z

Doc/library/dis.rst



 .. opcode:: BINARY_OP (op)

   Implements the binary and in-place operators (depending on the value of
   *op*).
-   ``TOS = TOS1 op TOS``.
+   ``STACK[-1] =  op STACK[-1]``.


You are missing the left operand.
STACK[-1] = STACK[-2] op STACK.pop()
or

rhs = STACK.pop() lhs = STACK.pop() STACK.push(lhs op rhs)

markshannon · 2022-12-23T14:59:34Z

Doc/library/dis.rst


   .. versionadded:: 3.11


 .. opcode:: BINARY_SUBSCR

-   Implements ``TOS = TOS1[TOS]``.
+   Implements ``STACK[-1] = STACK[-2][STACK[-1]]``.


This is incorrect, although no more than the old version.
See my comments on BINARY_OP

…list

… an argument

Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>

MatthieuDartiailh · 2023-01-12T15:08:55Z

I rebased to address the conflicts and updated the description of CALL_INSTRINSIC_1

ping @markshannon

markshannon

Excellent, thanks for this.

I've one comment on BUILD_TUPLE
And there is still @gvanrossum's comment on COPY to address.

markshannon · 2023-01-12T15:29:44Z

Doc/library/dis.rst

-   resulting tuple onto the stack.
+   resulting tuple onto the stack.::
+
+      STACK.append(tuple(STACK[-count:]))


This isn't quite correct. The colon should be before the -count and the values need to be popped from the stack.
Also state that count should not be zero (it can be, but the compiler emits LOAD_CONST in that case, so we might change that it the future.

assert(count > 0) STACK, values = STACK[-count:], STACK[:-count] STACK.append(tuple(values))

I am confused. BUILD_TUPLE consume the items on top of the stack so if count is 2 that would be STACK[-2],STACK[-1] == STACK[-2:] so shouldn't we write:

assert count > 0 STACK, values = STACK[:-count], STACK[-count:] STACK.append(tuple(values))

Yes, you're right.

bedevere-bot · 2023-01-12T15:33:33Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

MatthieuDartiailh · 2023-01-12T15:48:50Z

I updated BUILD_TUPLE based on my response to your comment which I believe is correct. Correct if I somehow got it wrong still.

I have made the requested changes; please review again

bedevere-bot · 2023-01-12T15:48:55Z

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

markshannon · 2023-01-12T18:23:58Z

Thanks

bedevere-bot added docs Documentation in the Doc dir skip news awaiting review labels Aug 31, 2022

kumaraditya303 requested a review from iritkatriel August 31, 2022 12:18

iritkatriel reviewed Aug 31, 2022

View reviewed changes

Doc/library/dis.rst Show resolved Hide resolved

iritkatriel requested a review from markshannon September 1, 2022 13:18

iritkatriel reviewed Sep 1, 2022

View reviewed changes

markshannon reviewed Oct 3, 2022

View reviewed changes

bedevere-bot mentioned this pull request Nov 8, 2022

CACHE and SWAP documentation is vague with the meaning of the oparg #96461

Closed

iritkatriel reviewed Dec 22, 2022

View reviewed changes

Doc/library/dis.rst Outdated Show resolved Hide resolved

iritkatriel reviewed Dec 22, 2022

View reviewed changes

Doc/library/dis.rst Outdated Show resolved Hide resolved

gvanrossum reviewed Dec 22, 2022

View reviewed changes

MatthieuDartiailh force-pushed the dis-docs branch from 8209c15 to 84176ea Compare December 23, 2022 14:45

markshannon requested changes Dec 23, 2022

View reviewed changes

bedevere-bot removed the awaiting review label Dec 23, 2022

MatthieuDartiailh and others added 13 commits January 12, 2023 16:05

Doc: define TOS(i) and rewrite CACHE and SWAP description to use it

5219126

typo

8cc0163

Doc.dis: remove use of TOSi and only use TOS(i)

21f6716

docs: use STACK to describe stack operation in analogy with a Python …

717f2e5

…list

docs: remove (delta) from BEFORE_WITH since BEFORE_WITH does not take…

2ebf26c

… an argument

docs: clarify UNPACK_EX documentation

62f007c

docs: fix typo in KW_NAMES documentation (i -> consti)

f5542af

docs: clarify the use of RESUME and RETURN_GENERATOR opcodes

7c11ec0

docs: simplify SWAP documentation

8d95852

Apply suggestions from code review

e37fd63

Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>

docs: remove trailing whitespaces

5564a05

docs: fix the description of the stack impact of multiple opcodes

f0d7f17

dis.rst: use STACK in the CALL_INSTRINSIC_1 description

b84a775

MatthieuDartiailh force-pushed the dis-docs branch from aaa0ba3 to b84a775 Compare January 12, 2023 15:08

markshannon requested changes Jan 12, 2023

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jan 12, 2023

dis.rst: address review comments

e8ec505

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 12, 2023

bedevere-bot requested a review from markshannon January 12, 2023 15:48

markshannon merged commit eecd422 into python:main Jan 12, 2023

bedevere-bot removed the awaiting change review label Jan 12, 2023

MatthieuDartiailh deleted the dis-docs branch January 13, 2023 14:15

Eclips4 mentioned this pull request Jul 4, 2024

dis: BUILD_LIST docs incorrectly say count must be > 0 #121367

Closed

		@@ -382,6 +382,10 @@ The Python compiler currently generates the following bytecode instructions.

		General instructions

		In the following, TOS(i) refer to the i-th item on the stack which is index from 1.

	In the following, TOS(i) refer to the i-th item on the stack which is index from 1.
	In the following, TOS(i) refers to the i-th item on the stack which is indexed from 1.

Uh oh!

gh-96461: clarify the meaning of the oparg for CACHE and COPY opcode #96462

gh-96461: clarify the meaning of the oparg for CACHE and COPY opcode #96462

Uh oh!

Conversation

MatthieuDartiailh commented Aug 31, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iritkatriel Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatthieuDartiailh commented Oct 5, 2022

Uh oh!

iritkatriel commented Oct 5, 2022

Uh oh!

MatthieuDartiailh commented Oct 5, 2022

Uh oh!

MatthieuDartiailh commented Oct 6, 2022

Uh oh!

sweeneyde commented Oct 7, 2022

Uh oh!

MatthieuDartiailh commented Oct 7, 2022

Uh oh!

MatthieuDartiailh commented Nov 8, 2022

Uh oh!

Uh oh!

Uh oh!

iritkatriel commented Dec 22, 2022

Uh oh!

MatthieuDartiailh commented Dec 22, 2022

Uh oh!

ghost commented Dec 22, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatthieuDartiailh commented Dec 22, 2022

Uh oh!

MatthieuDartiailh commented Dec 23, 2022

Uh oh!

markshannon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatthieuDartiailh commented Jan 12, 2023

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 12, 2023

Uh oh!

MatthieuDartiailh commented Aug 31, 2022 •

edited by bedevere-bot

Loading

iritkatriel Sep 1, 2022 •

edited

Loading

ghost commented Dec 22, 2022 •

edited by ghost

Loading

markshannon left a comment •

edited

Loading