Skip to content

Conversation

@souradeep-das
Copy link
Contributor

@souradeep-das souradeep-das commented Jun 16, 2025

What was wrong?

#9

How was it fixed?

  • As the cst nodes are traversed, assign the statements index position as the child offset
    Since, the child_offset for the cst node is incremented both during enter_class_def and after visiting the node at exit(), incrementing on every iteration sometimes incorrectly assigns different child offset indexes (skipped by 2)

  • while traversing cst, do not revisit docstring lines as statements (since they are attached to their parent attribute)

  • For the class body, the cst children starts with a Index 0: TrailingWhitespace(), while for modules (outside classes) index starts with constants - selectively handle that when determining the offset of the docstring

Testing

Verified against different class structures and conditions

Sample Class structures and results
bytes.py (in src_docc).pdf

@souradeep-das souradeep-das marked this pull request as ready for review June 16, 2025 22:33
self.old_stack[-1].child_offset += 1
for idx, cst_statement in enumerate(cst_node.body.body):
self.old_stack[-1].child_offset = idx
if (
Copy link
Owner

Choose a reason for hiding this comment

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

If I'm reading this correctly, this detects any string alone on a line? I suspect you tested it, but this works for multiline strings, and for strings that aren't the first child? Like:

def foo():
    a = 1
    "String"
    return a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so for functions the functionality is unchanged. For class definitions it would still take the very next docstring line after the attribute (also multilines). Dangling strings not attached to class attributes or class descriptions wont show up.

I have attached some test outputs here (also with functions inside classes), which covers these cases: #18 (comment)

do lmk if something is off, or needs to be handled.

for cst_statement in cst_node.body.body:
self.old_stack[-1].child_offset += 1
for idx, cst_statement in enumerate(cst_node.body.body):
self.old_stack[-1].child_offset = idx
Copy link
Owner

Choose a reason for hiding this comment

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

Is this a stylistic change, or is this important for fixing the issue? If it's the latter, can you briefly explain what went wrong here? When would += 1 be different from idx?

Even if it's just stylistic, it's definitely an improvement.

Copy link
Contributor Author

@souradeep-das souradeep-das Jun 17, 2025

Choose a reason for hiding this comment

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

I noticed that the child_offset was being incremented also at exit() L843, which resulted in statements being assigned offsets in double increments (multiples of 2). so used idx to assign it to its true position

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, interesting. Again, there's a good chance I'm wrong here, but it looks like enter_class_def returns SkipChildren, so do the children of the class definition actually get their exit called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the exit() for simpleStatementLine is still called since we do a statement.visit(self) on L573

I can see offsets being incremented after visiting every child

..
child_offset=8: SimpleStatementLine(
    body=[
        AnnAssign(
            target=Name(
                value='parent3_hash',
...
child_offset incremented to 9 after exiting CstNode(SimpleStatementLine(...), start=12:4, end=12:24)
  Source line: parent3_hash: Hash32
  
child_offset=10: SimpleStatementLine(
    body=[
        Expr(
            value=SimpleString(
                value='"""Number of bytes3 in each instance of this class."""',
..

@SamWilsn SamWilsn force-pushed the souradeep/fix_class_attr branch from e94f5eb to c060f6f Compare June 17, 2025 17:25
@SamWilsn SamWilsn merged commit 62194ee into SamWilsn:master Jun 17, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants