Z317 incorrect multi-line parameters in nested data types

[nndii]

Bug report

What's wrong

Z317 enforces me to split nested tuples in multiple lines.

Code sample that causes that issue

from pyramid.security import Allow

class Blog(object):

    def __acl__(self):
        return [
            (Allow, 'group:editors', ('edit', 'create')),
        ]

How is that should be

There is "special case" in docs but its not actually clear whether I should follow it strictly or nested data types in one line is also allowed.

flake8 information

{
  "dependencies": [
    {
      "dependency": "setuptools",
      "version": "39.0.1"
    }
  ],
  "platform": {
    "python_implementation": "CPython",
    "python_version": "3.7.1",
    "system": "Linux"
  },
  "plugins": [
    {
      "is_local": false,
      "plugin": "flake8-bandit",
      "version": "v1.0.2"
    },
    {
      "is_local": false,
      "plugin": "flake8-broken-line",
      "version": "0.1.0"
    },
    {
      "is_local": false,
      "plugin": "flake8-bugbear",
      "version": "18.8.0"
    },
    {
      "is_local": false,
      "plugin": "flake8-comprehensions",
      "version": "1.4.1"
    },
    {
      "is_local": false,
      "plugin": "flake8-debugger",
      "version": "3.1.0"
    },
    {
      "is_local": false,
      "plugin": "flake8-docstrings",
      "version": "1.3.0, pydocstyle: 3.0.0"
    },
    {
      "is_local": false,
      "plugin": "flake8-eradicate",
      "version": "0.1.1"
    },
    {
      "is_local": false,
      "plugin": "flake8-mypy",
      "version": "17.8.0"
    },
    {
      "is_local": false,
      "plugin": "flake8-print",
      "version": "3.1.0"
    },
    {
      "is_local": false,
      "plugin": "flake8-string-format",
      "version": "0.2.3"
    },
    {
      "is_local": false,
      "plugin": "flake8-type-annotations",
      "version": "0.1.0"
    },
    {
      "is_local": false,
      "plugin": "flake8_builtins",
      "version": "1.4.1"
    },
    {
      "is_local": false,
      "plugin": "flake8_coding",
      "version": "1.3.1"
    },
    {
      "is_local": false,
      "plugin": "flake8_commas",
      "version": "2.0.0"
    },
    {
      "is_local": false,
      "plugin": "flake8_pep3101",
      "version": "1.2.1"
    },
    {
      "is_local": false,
      "plugin": "flake8_quotes",
      "version": "1.0.0"
    },
    {
      "is_local": false,
      "plugin": "logging-format",
      "version": "0.5.0"
    },
    {
      "is_local": false,
      "plugin": "mccabe",
      "version": "0.6.1"
    },
    {
      "is_local": false,
      "plugin": "naming",
      "version": "0.7.0"
    },
    {
      "is_local": false,
      "plugin": "per-file-ignores",
      "version": "0.6"
    },
    {
      "is_local": false,
      "plugin": "pycodestyle",
      "version": "2.4.0"
    },
    {
      "is_local": false,
      "plugin": "pyflakes",
      "version": "2.0.0"
    },
    {
      "is_local": false,
      "plugin": "wemake-python-styleguide",
      "version": "0.6.2"
    }
  ],
  "version": "3.6.0"
}

[sobolevn]

Needs investigation.

[sobolevn]

@nndii yes, that a bug indeed. And a very complex one. Oh man, I hate this rule. It is my personal nightmare.

[sobolevn]

The problem is with ast parsing of tuple in python:

print((  # should start from here
    1, 2, 3,  # actually starts from here
))

and

print([
    ('Allow', 'group:editors', ('edit', 'create')),  # tuple starts from here
])

ast incorrectly detects lineno. But, I am almost completely lost. How should it work?!

[sobolevn]

Maybe we can treat it as a single line? And just force all elements of these tuples to be on line with print?

Like astor does:

[('Allow', 'group:editors', ('edit', 'create'))]
('Allow', 'group:editors', ('edit', 'create'))

[sobolevn]

@nndii ok, here's what's wrong. ast parsing of tuples is broken. And it can not be fixed right now. You will have to live with the current implementation for some time.

Later, I will rewrite this check from ast to tokenize (I have spent almost a whole working week the last time I have tried).

Any help is highly appreciated. If you can handle this task - it would be the most significant contribution to the project ever.

[kxepal]

@sobolevn Could you tell a bit how it's broken and why it's hard to fix?

[sobolevn]

@kxepal here's what's wrong:

1. ast is abstract syntax tree. It means that some syntax is changed or dropped
2. In this case we are dealing with different line number in tuples
3. When tuples are defined their line numbers are not preserved, see some examples above
4. I have tried to fix it https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/transformations/ast/bugfixes.py#L37-L61
5. I have failed to fix it, since this bug exist
6. I have tried to fix it with this idea: https://github.com/wemake-services/wemake-python-styleguide/issues/454#issuecomment-456496474
7. I have failed again, it have to be rewritten to tokenize which preserves line numbers

Why is it hard?

1. Because you have to work inside ordered list of tokens that can be nested into multiple levels, eg: [[1, [2],]]
2. You have to maintain the state between them, since the rule requires to know about previous parameters
3. Algorithm is quite tricky, there are different cases that needs to be covered

[ffedoroff]

Another false positive example:

    class Meta:
        model = User
        fields = (
            'id', 'username', 'first_name', 'last_name', 'email', 'created', 'modified', 'version',
            'address', 'city', 'state', 'zip', 'groups',
        )

[sobolevn]

@ffedoroff this is not false positive. Correct one should be:

class Meta(object):
        model = User
        fields = (
            'id', 
           'username', 
           'first_name', 
           'last_name', 
           ...
        )

[sobolevn]

Related: https://github.com/jsfehler/flake8-multiline-containers/issues/9#issuecomment-519936744

I guess, that we can switch to flake8-multiline-containers when this feature will be ready.