arrow_alignment --fix doesn't indent keys when introducing line breaks; erroneously reports success

[stig]

I installed 2.0.0 and ran bundle exec puppet-lint --only-checks arrow_alignment --fix on our puppet repo. After running again without the --fix flag four instances were still failing. Running a second time with the with the --fix flag resulted in all four of those being reported fixed, but two of them still fail on subsequent checks.

The two pieces of code that continue to fail had newlines introduced by the --fix flag, and I believe the alignment fails because the initial indentation of the keys do not match. Here's the patches:

    @@ -28,12 +28,13 @@ class laterpay::core::puppetdb_setup (
             class { 'puppetdb':
    -            database                => 'embedded',
    +            database             => 'embedded',
                 #database                => 'postgres',
                 #postgres_version        => '9.3',
    -            java_args               => { '-Xmx' => '512m', '-Xms' => '256m' },
    -            listen_address          => $::ipaddress_eth0,
    -            listen_port             => 4998,
    -            ssl_listen_address      => $::ipaddress_eth0,
    -            ssl_listen_port         => 4999,
    -            open_listen_port        => false,
    -            open_ssl_listen_port    => false;
    +            java_args            => { '-Xmx'            => '512m',
    + '-Xms'            => '256m' },
    +            listen_address       => $::ipaddress_eth0,
    +            listen_port          => 4998,
    +            ssl_listen_address   => $::ipaddress_eth0,
    +            ssl_listen_port      => 4999,
    +            open_listen_port     => false,
    +            open_ssl_listen_port => false;
             }

And here is the other:

    @@ -100,12 +100,14 @@ class example_program::setup (
           audit_log_parts => 'ABJDEFHZ'
         }
         apache::vhost { 'example_program':
    -        vhost_name  => '*',
    -        ip          => $application_ip,
    -        port        => $application_port,
    -        docroot     => '/var/www',
    -        proxy_pass  => [
    -            { 'path' => '/', 'url' => "http://127.0.0.1:${application_port}/", 'reverse_urls' => "http://127.0.0.1:${application_port}/" },
    +        vhost_name         => '*',
    +        ip                 => $application_ip,
    +        port               => $application_port,
    +        docroot            => '/var/www',
    +        proxy_pass         => [
    +            { 'path'                    => '/',
    + 'url'                     => "http://127.0.0.1:${application_port}/",
    + 'reverse_urls'            => "http://127.0.0.1:${application_port}/" },
             ],
         }

I tried to additionally activate the 2sp_soft_tabs check, but although it complains about the newly introduced lines, it does not succeed in fixing them. However, when I manually indented the keys so they start in the same column then subsequent --fix runs work as expected.

[arrdem]

So I'm not entirely sure what the fix to this is, but I can show that it's frustratingly nontrivial as there are several issues in flight here.

scratch.pp

#
class example (
  $external_ip_base,
) {

  bar { 'xxxxxxxxxxx':
    inputs => {
      'ny' => {
        "${external_ip_base}.16:443 ${a} ${b} ${c}" => 'foo',
        'veryveryverylongstring8:443'=>'foo',
        'simple'=>'foo',
        '3'=> :foo,
        :baz=> :qux,
        3=> 3,
      },
    },
  }
}

Using the patch

diff --git a/lib/puppet-lint/plugins/check_whitespace.rb b/lib/puppet-lint/plugins/check_whitespace.rb
index ec27831..ba09c34 100644
--- a/lib/puppet-lint/plugins/check_whitespace.rb
+++ b/lib/puppet-lint/plugins/check_whitespace.rb
@@ -131,9 +131,8 @@ PuppetLint.new_check(:arrow_alignment) do
       resource_tokens.each_with_index do |token, idx|
         if token.type == :FARROW
           (level_tokens[indent_depth_idx] ||= []) << token
-          prev_indent_token = resource_tokens[0..idx].rindex { |t| t.type == :INDENT }
-          indent_token_length = prev_indent_token.nil? ? 0 : resource_tokens[prev_indent_token].to_manifest.length
-          indent_length = indent_token_length + token.prev_code_token.to_manifest.length + 2
+          whitespace_padding = token.prev_token.type == :INDENT ? token.prev_token.value.length : 0
+          indent_length = token.column + 1 - whitespace_padding

           if indent_depth[indent_depth_idx] < indent_length
             indent_depth[indent_depth_idx] = indent_length

Running

$ bundle exec puppet-lint ./scratch.pp --only-checks arrow_alignment --fix; cat scratch.pp
FIXED: indentation of => is not properly aligned (expected in column 42, but found it in column 41) on line 9
FIXED: indentation of => is not properly aligned (expected in column 42, but found it in column 38) on line 10
FIXED: indentation of => is not properly aligned (expected in column 42, but found it in column 17) on line 11
FIXED: indentation of => is not properly aligned (expected in column 42, but found it in column 12) on line 12
FIXED: indentation of => is not properly aligned (expected in column 42, but found it in column 13) on line 13
FIXED: indentation of => is not properly aligned (expected in column 42, but found it in column 10) on line 14
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 13) on line 13
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 10) on line 14
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 13) on line 13
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 10) on line 14
#
class example (
  $external_ip_base,
) {

  bar { 'xxxxxxxxxxx':
    inputs => {
      'ny' => {
        "${external_ip_base}.16:443 ${a} ${b} ${c}"                                       => 'foo',
        'veryveryverylongstring8:443'    =>'foo',
        'simple'                         =>'foo',
        '3'                              => :foo,
        :baz                                                       => :qux,
        3                                        => 3,
      },
    },
  }
}

So the first problem here, addressed by the pasted patch, is that the existing implementation assumes the code token preceding is the full width of the line preceding the =>. This assumption breaks down in the case of double quoted strings containing variable substitutions, because of the way that the lexer tokenizes double quoted strings. Where single quoted strings are tokenized as a single entity, double quoted strings are tokenized as a list of tokens conforming to the regex on types [:DQPRE (:VARIABLE | :DQMID)* :DQPOST]. When the existing implementation tries to compute the width of a double quoted string, token.prev_code_token will be a :DQPOST node, which has a .value being the trailing non-variable subset of the string before the ". So for instance in "${foo}bar" the .value will be 'bar'. This means that the indenter as implemented sees that the width of the previous code token is nowhere near the real width of the preceding text.

The patch in this comment fixes this, by realizing that we can infer the total width of all the preceding text on the line by taking the column of the => and subtracting the width of the prev_token if and only if it is whitespace.

But something else is screwy here. Either the tokenizer is doing something wacky with line number information, because the reported arrow line numbers are totally wrong here and suggest that several lines are being re-indented repeatedly.

[rnelson0]

@arrdem Have you had any chance to revisit this since PuppetConf?

[arrdem]

@rnelson0 I haven't. I'll try and make some time for it this week.

[rnelson0]

This is now a little better and a little worse. Starting with the original sample:

$ cat 506.pp
class { 'puppetdb':
    database                => 'embedded',
    #database                => 'postgres',
    #postgres_version        => '9.3',
    java_args               => { '-Xmx' => '512m', '-Xms' => '256m' },
    listen_address          => $::ipaddress_eth0,
    listen_port             => 4998,
    ssl_listen_address      => $::ipaddress_eth0,
    ssl_listen_port         => 4999,
    open_listen_port        => false,
    open_ssl_listen_port    => false;
}
$ be puppet-lint 506.pp --fix
FIXED: indentation of => is not properly aligned (expected in column 41, but found it in column 59) on line 5
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 29) on line 2
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 29) on line 5
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 29) on line 6
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 29) on line 7
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 29) on line 8
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 29) on line 9
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 29) on line 10
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 29) on line 11
$ cat 506.pp
class { 'puppetdb':
    database             => 'embedded',
    #database                => 'postgres',
    #postgres_version        => '9.3',
    java_args            => { '-Xmx' => '512m',
                                 '-Xms' => '256m' },
    listen_address       => $::ipaddress_eth0,
    listen_port          => 4998,
    ssl_listen_address   => $::ipaddress_eth0,
    ssl_listen_port      => 4999,
    open_listen_port     => false,
    open_ssl_listen_port => false;
}

The 2nd line of java_args isn't thrown all the way too the left, but it isn't correct, either. Further fix attempts do not change this.

$ cat 506-1.pp
class example_program::setup (
  audit_log_parts => 'ABJDEFHZ'
}
apache::vhost { 'example_program':
    vhost_name  => '*',
    ip          => $application_ip,
    port        => $application_port,
    docroot     => '/var/www',
    proxy_pass  => [
        { 'path' => '/', 'url' => "http://127.0.0.1:${application_port}/", 'reverse_urls' => "http://127.0.0.1:${application_port}/" },
    ],
}
$ be puppet-lint 506-1.pp --fix
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 18) on line 10
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 32) on line 10
FIXED: indentation of => is not properly aligned (expected in column 26, but found it in column 91) on line 10
FIXED: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 5
FIXED: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 6
FIXED: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 7
FIXED: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 8
FIXED: indentation of => is not properly aligned (expected in column 16, but found it in column 17) on line 9
$ cat 506-1.pp
class example_program::setup (
  audit_log_parts => 'ABJDEFHZ'
}
apache::vhost { 'example_program':
    vhost_name => '*',
    ip         => $application_ip,
    port       => $application_port,
    docroot    => '/var/www',
    proxy_pass => [
        { 'path'         => '/',
          'url'          => "http://127.0.0.1:${application_port}/",
          'reverse_urls' => "http://127.0.0.1:${application_port}/" },
    ],
}

In this case, proxy_pass entries are properly split by lines and correctly aligned.

$ cat scratch.pp
#
class example (
  $external_ip_base,
) {

  bar { 'xxxxxxxxxxx':
    inputs => {
      'ny' => {
        "${external_ip_base}.16:443 ${a} ${b} ${c}" => 'foo',
        'veryveryverylongstring8:443'=>'foo',
        'simple'=>'foo',
        '3'=> :foo,
        :baz=> :qux,
        3=> 3,
      },
    },
  }
}
$ be puppet-lint scratch.pp --fix
ERROR: example not in autoload module layout on line 2
WARNING: top-scope variable being used without an explicit namespace on line 9
WARNING: top-scope variable being used without an explicit namespace on line 9
WARNING: top-scope variable being used without an explicit namespace on line 9
FIXED: indentation of => is not properly aligned (expected in column 53, but found it in column 38) on line 10
FIXED: indentation of => is not properly aligned (expected in column 53, but found it in column 17) on line 11
FIXED: indentation of => is not properly aligned (expected in column 53, but found it in column 12) on line 12
FIXED: indentation of => is not properly aligned (expected in column 53, but found it in column 13) on line 13
FIXED: indentation of => is not properly aligned (expected in column 53, but found it in column 10) on line 14
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 13) on line 13
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 10) on line 14
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 13) on line 13
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 10) on line 14
$ cat scratch.pp
#
class example (
  $external_ip_base,
) {

  bar { 'xxxxxxxxxxx':
    inputs => {
      'ny' => {
        "${external_ip_base}.16:443 ${a} ${b} ${c}" => 'foo',
        'veryveryverylongstring8:443'               =>'foo',
        'simple'                                    =>'foo',
        '3'                                         => :foo,
        :baz                                           => :qux,
        3                                                 => 3,
      },
    },
  }
}

This one gets interesting if you fix it a few more times:

$ be puppet-lint scratch.pp --fix
ERROR: example not in autoload module layout on line 2
WARNING: top-scope variable being used without an explicit namespace on line 9
WARNING: top-scope variable being used without an explicit namespace on line 9
WARNING: top-scope variable being used without an explicit namespace on line 9
FIXED: indentation of => is not properly aligned (expected in column 53, but found it in column 54) on line 13
FIXED: indentation of => is not properly aligned (expected in column 53, but found it in column 13) on line 14
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 54) on line 13
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 13) on line 14
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 54) on line 13
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 13) on line 14
[rnelson0@build03 puppet-lint:master]$ cat scratch.pp
#
class example (
  $external_ip_base,
) {

  bar { 'xxxxxxxxxxx':
    inputs => {
      'ny' => {
        "${external_ip_base}.16:443 ${a} ${b} ${c}" => 'foo',
        'veryveryverylongstring8:443'               =>'foo',
        'simple'                                    =>'foo',
        '3'                                         => :foo,
        :baz => :qux,
        3   => 3,
      },
    },
  }
}
[rnelson0@build03 puppet-lint:master]$ be puppet-lint scratch.pp --fix
ERROR: example not in autoload module layout on line 2
WARNING: top-scope variable being used without an explicit namespace on line 9
WARNING: top-scope variable being used without an explicit namespace on line 9
WARNING: top-scope variable being used without an explicit namespace on line 9
FIXED: indentation of => is not properly aligned (expected in column 53, but found it in column 14) on line 13
FIXED: indentation of => is not properly aligned (expected in column 53, but found it in column 13) on line 14
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 13) on line 14
FIXED: indentation of => is not properly aligned (expected in column 14, but found it in column 13) on line 14
[rnelson0@build03 puppet-lint:master]$ cat scratch.pp
#
class example (
  $external_ip_base,
) {

  bar { 'xxxxxxxxxxx':
    inputs => {
      'ny' => {
        "${external_ip_base}.16:443 ${a} ${b} ${c}" => 'foo',
        'veryveryverylongstring8:443'               =>'foo',
        'simple'                                    =>'foo',
        '3'                                         => :foo,
        :baz                                         => :qux,
        3   => 3,
      },
    },
  }
}

It rotates between those two results now.

Clearly recent fixes have not resolved all of the issues.

[rodjek]

Ahh arrow alignment. Simple for humans but quite problematic to handle all the possible cases in code. Seeing as I dug into it most recently for a different issue, I'll check this one out on the plane tomorrow.