GeezerVison

Just kindly to ask any further updates needed? I'm not sure whether it's proper to re-request review in CPython PR workflow.

@iritkatriel Hi, I've encountered an issue when trying to remove f-string conversion in astfold_body. For the following function when opt=0 or 1:

def func():
    "not " + "a docstring"

astfold_body prevents the concatenated string (optimized as const expression statement somewhere during AST optimization I guess) being parsed as docstring by converting it into f-string. But if we remove the logic, the symbol table generation cannot distinguish "not a docstring" is originally a real docstring, or optimized concatenated string.

@iritkatriel Hi, I've encountered an issue when trying to remove f-string conversion in astfold_body. For the following function when opt=0 or 1:

def func():
    "not " + "a docstring"

astfold_body prevents the concatenated string (optimized as const expression statement somewhere during AST optimization I guess) being parsed as docstring by converting it into f-string. But if we remove the logic, the symbol table generation cannot distinguish "not a docstring" is originally a real docstring, or optimized concatenated string.

I thought the idea is that it uses ste_has_docstring to determine whether there is a docstring or not, rather than the type of the first expression.

I thought the idea is that it uses ste_has_docstring to determine whether there is a docstring or not, rather than the type of the first expression.

Sorry I'm a little confused now. 😂

If we remove the f-string conversion in the astfold_body function, the _PyAST_GetDocString returns true and set ste_has_docstring incorrectly in symbol table generation stage.

The reason is _PyAST_GetDocString checks whether there is a docstring by inspecting the first expression type. If it's a Constant_kind, then it returns true. If we remove the f-string conversion in AST optimization stage, it cannot distinguish the constant expression is actually a docstring or concatenated string expression. In either cases, it returns true and ste_has_docstring will be set.

If there is something I misunderstood, please correct me, thanks!

I guess the problem is that we perform AST optimization before we build the symtable, so we have to keep doing the f-string conversion to avoid breaking the symtable. Maybe we can instead run the symtable before ast optimization, so we can get rid of the f-string trick in the AST optimizer.

I guess the problem is that we perform AST optimization before we build the symtable

Yeah that's the problem. That's say, we cannot convey the information from AST construction to symtable generation phase.

Maybe we can instead run the symtable before ast optimization, so we can get rid of the f-string trick in the AST optimizer.

Theoratically it's feasible, but I'm afraid that there are side-effects and the scope is beyond this PR.

My proposal (in #126072 (comment)) was to move the docstring removal to the symtable, so it's all in one place.

My proposal (in #126072 (comment)) was to move the docstring removal to the symtable, so it's all in one place.

But I think the problem is still not addressed, we've lost some information after the AST optimization (the concatenated string has been optimized as constant string). Even though the docstring removal is put together with symtable, it still fails to distinguish in the above case.

Revert to the version without swapping AST optimization and symtable generation. The docstring removal in astfold_body can be omitted in cunrrent implementation, but the f-string conversion should be retained. IMHO, removing the f-string conversion will introduce a lot of compromise changes, which even makes the code more complicated.

Specifically, can we disable the constant string concatenation optimization in AST when opt=1?

No, this would mean we optimise less when we should be optimising more.

I think we should leave symtable after ast_opt and try my suggestion to just move the docstring handling from ast_opt to symtable.

I think we should leave symtable after ast_opt and try my suggestion to just move the docstring handling from ast_opt to symtable.

As mentioned above, what blocks us doing so is if we leave the docstring handling to symtable, for the following code:

def with_const_expression():
    "aa" * 4

The optimized AST will be:

Module(
  body=[
    FunctionDef(
      name='with_const_expression',
      args=arguments(),
      body=[
        Expr(
          value=Constant(value='aaaaaaaa'))])])

Then in the symtable stage, it cannot distinguish whether the constant string 'aaaaaaaa' is really a docstring, or the optimized concatenated string. So the compiler will incorrectly set ste_has_docstring.

Ok, I see. I think we should just forget about that for this PR. Can you revert all the changes related to removing the hack, and make this PR just about about module and class change?