mesonbuild / meson

The Meson Build System
http://mesonbuild.com
Apache License 2.0
5.57k stars 1.62k forks source link

meson format: mangles with `if` statements with indented conditionals #13570

Open dcbaker opened 2 months ago

dcbaker commented 2 months ago

Describe the bug It's fairly common on projects using 2 space indent to use 4 space indent for multi line conditionals, so that the condition and the body of the block are visually distinct:

if (cc.links('''
    void do_func(int foo) { return INT_MAX - foo; }
    '''))
  deps += dependency('foo')
endif

meson format mangles this into something resembling the nonsense that black produces:

if (cc.links('''
    bool func(uint foo) { return foo < MAX_INT; }
    '''))
  cargs += ['-DHAS_MAX_INT']
endif

meson format turns this reasonable and readable code into:

if (cc.links(
    '''
    bool func(uint foo) { return foo < MAX_INT; }
    ''',
))
    cargs += ['-DHAS_MAX_INT']
endif

Expected behavior Does not mangle the if so that the closing parans are on the same indent level as the if/endif

system parameters Meson 1.5 and master

annacrombie commented 2 months ago

@dcbaker So you would prefer this?

if (cc.links(
    '''
    bool func(uint foo) { return foo < MAX_INT; }
    ''',
    ))
    cargs += ['-DHAS_MAX_INT']
endif

What is the actual output you want? (asking for a friend)

dcbaker commented 2 months ago

Well, since I use the canonical two space indent (:D), I'd expect this:

if (cc.links(
    '''
    bool func(uint foo) { return foo < MAX_INT; }
    ''',
    ))
  cargs += ['-DHAS_MAX_INT']
endif

But if you used four space indent, I'd expect what python that doesn't use black (like Meson itself) does:

if (cc.links(
        '''
        bool func(uint foo) { return foo < MAX_INT; }
        ''',
        ))
    cargs += ['-DHAS_MAX_INT']
endif
annacrombie commented 2 months ago

I see, so you wouldn't like it to just be what it originally was (with the trailing parens stuck to the line)?

Also, I disagree with your second example because changing the indentation inside ''' is actually modifying the string itself which a formatter should never do imo.

dcbaker commented 2 months ago

My principle problem is that whenever I see:

if (
  ...
)
  ...

This is super confusing, because ^\) It makes me think "This if block is done, we are now outside of the if", it's visually confusing. Indenting a second level (whether the )) stays attached or moves to its own line), makes it clear to me visual "I am still inside the if block". This is similar to the C mistake of:

if (condition)
  "something in the if block"
  "oops! looks like we're in the if block, but were not!"

And I get that the first ''' is changing, it just isn't relevant to my example so I was being lose with it.

bruchar1 commented 2 months ago

Do we want this as the only way to format, or do we want this as an option?

dcbaker commented 2 months ago

I suspect we'll want an option for this, because from my experience in python people either really love the:

if (
   condition
)
  body
endif

style, or they really hate it (and vice versa)

annacrombie commented 2 months ago

I think the option could be called sticky_parens, and it controls wether parenthesis stick to the wrapping expression, e.g.

if (cc.links('''
    bool func(uint foo) { return foo < MAX_INT; }
    '''))
  cargs += ['-DHAS_MAX_INT']
endif

It also causes extra indentation in conditionals when enabled to make the conditional part clear, e.g:

if (not cc.compiles('...') 
    or cc.compiles('...') 
    or cc.compiles('...'))
  do
  stuff
endif

With sticky_parens off:

if (
  not cc.compiles('...') 
  or cc.compiles('...') 
  or cc.compiles('...')
)
  do
  stuff
endif