python / cpython

The Python programming language
https://www.python.org
Other
62.39k stars 29.96k forks source link

`del (a, [b])` is currently accepted syntax by the python interpreter #113587

Open 15r10nk opened 8 months ago

15r10nk commented 8 months ago

Bug report

Bug description:

I found the following while working on pysource-codegen, which generates random python code. I don't know any real world python code which does this.

echo "del (a,[b])" | python3.12 -m dis
  0           0 RESUME                   0
  1           2 DELETE_NAME              0 (a)
              4 DELETE_NAME              1 (b)
              6 RETURN_CONST             0 (None)

echo "del (a,[b])" | python3.12 -m ast
Module(
   body=[
      Delete(
         targets=[
            Tuple(
               elts=[
                  Name(id='a', ctx=Del()),
                  List(
                     elts=[
                        Name(id='b', ctx=Del())],
                     ctx=Del())],
               ctx=Del())])],
   type_ignores=[])

I think that this is most likely an bug in the parser.

Another issue that I found while working on this is that del (a,b) and del a,b generate different python ast.

echo "del (a,b)" | python3.12 -m ast
Module(
   body=[
      Delete(
         targets=[
            Tuple(
               elts=[
                  Name(id='a', ctx=Del()),
                  Name(id='b', ctx=Del())],
               ctx=Del())])],
   type_ignores=[])
echo "del a,b" | python3.12 -m ast
Module(
   body=[
      Delete(
         targets=[
            Name(id='a', ctx=Del()),
            Name(id='b', ctx=Del())])],
   type_ignores=[])

The ast documentation says:

class ast.Delete(targets) Represents a del statement. targets is a list of nodes, such as Name, Attribute or Subscript nodes

I don't know what the best solution to this problem should be, but I would like to have a correct documentation. Especially the case that ast.Tuple could be a del target is missing and was confusing for me. I had expected that del (a,b) and del a,b would produce the same ast and that del (a,(b,c)) is invalid syntax.

del [a] looks like a bug to me and I hope it can be fixed.

CPython versions tested on:

3.12

Operating systems tested on:

Linux

Linked PRs

ronaldoussoren commented 8 months ago

The language reference: has this grammar rule for the del statement:

del_stmt ::= "del" target_list

Target_list is also used for the LHS of an assignment statement.

All examples are valid python code. I'm not enough of a parser expert to have an opinion on the generated AST.

grigoriev-semyon commented 8 months ago

target can be "[" target_list "]" so that's not a bug in the current version

indeed, the ast turns out different, but this is supported by the python grammar

15r10nk commented 8 months ago

so that's not a bug in the current version

This maybe depends on the definition of bug, but I don't think that the behavior was intended. target_list is only used for del and assignments. It looks like it was original build for assignments and later reused for del.

There are explicit tests which check if you delete anything which would be allowed by the target_list syntax but can not be deleted like star-expressions. I think that [] was just forgotten.

https://github.com/python/cpython/blob/50b093f5c7060c0b44c264808411346cee7becf0/Lib/test/test_syntax.py#L2091-L2094C39

However, a clear documentation would be nice if ast.Tuple or maybe even ast.List should be valid childs for the ast.Del. Otherwise anyone who tries to analyse code which works with ast.Del will be surprised by its possible values.

15r10nk commented 8 months ago

I take it back. I found tests for this syntax:

   del a, [b, c], (d, [e, f])

https://github.com/python/cpython/blob/50b093f5c7060c0b44c264808411346cee7becf0/Lib/test/test_grammar.py#L893C35-L893C35

I think this should be added to the documentation of ast.Del.

sunmy2019 commented 8 months ago

So, now it's a doc issue?

davidmerwin commented 8 months ago

I take it back. I found tests for this syntax: ...

@15r10nk agree

Sent with GitHawk

15r10nk commented 8 months ago

So, now it's a doc issue?

I think so. I'm still curious as to why it has to be that way, but I can live with it if there is intent behind it (the test is that case)

grigoriev-semyon commented 8 months ago

Is backport to 3.8-3.12 needed?

Eclips4 commented 8 months ago

Is backport to 3.8-3.12 needed?

Only to 3.11 and 3.12. 3.8-3.10 accepts only security fixes.