Auto correct appears to run twice

[davidsandilands]

Since pdk 2.5 it appears that the autocorrect runs twice for example running a pdk validate -a on the following code:

class woof {
  Service { 'httpd':
    ensure => running,
    enable => true,
  }
  Package { 'test':
    ensure  => installed,
    options => 'banter'
  }
}

Produces the following debugging showing it corrected twice and two commas appear on the line with the issue.

pdk (DEBUG): Execution of '/mnt/c/Users/David Sandilands/woof/bin/puppet-lint --json --relative --fix manifests/init.pp spec/fixtures/manifests/site.pp' complete (duration: 0.9346774s; exit code: 0)
pdk (DEBUG): STDOUT:
[
[
  {
    "message": "missing trailing comma after last element",
    "line": 14,
    "column": 24,
    "token": "#<PuppetLint::Lexer::Token:0x000000000299d098>",
    "kind": "fixed",
    "check": "trailing_comma",
    "fullpath": "/mnt/c/Users/David Sandilands/woof/manifests/init.pp",
    "path": "manifests/init.pp",
    "filename": "init.pp",
    "KIND": "FIXED"
  },
  {
    "message": "missing trailing comma after last element",
    "line": 14,
    "column": 24,
    "token": "#<PuppetLint::Lexer::Token:0x000000000299d098>",
    "kind": "fixed",
    "check": "trailing_comma",
    "fullpath": "/mnt/c/Users/David Sandilands/woof/manifests/init.pp",
    "path": "manifests/init.pp",
    "filename": "init.pp",
    "KIND": "FIXED"
  }
]

[ekohl]

I'm pretty sure I saw some other report of this and that was actually an issue deeper in puppet-lint / pdk, but I can't find the other bug report.

[davidsandilands]

We discovered this is when a resource default is used instead of resource declaration (that capital is hidden in plain sight), so it ends up falling into both default and resource loops just now.

Great find by @chelnak

[chelnak]

To add to the comment above:

It appears that the resource_indexes method cannot differentiate between a resource declaration and a defaults declaration.

If we compare the two, you can see that a defaults declaration type starts with a capital letter whereas a resource declaration does not.

defaults declaration

Service { 'foo':
  ensure => running,
}

resource declaration

service { 'foo':
  ensure => running,
}

When the resource_indexes method runs, it initially selects tokens by the presence of a colon then works to the right to decide if there are any violations.

That means we are actually only starting to collect information about the declaration after the name.. so we never know the actual type of the declaration.

Service { 'foo':
#              ↑ (here)
  ensure => running,
}

https://github.com/puppetlabs/puppet-lint/blob/main/lib/puppet-lint/data.rb#L167

In contrast, defaults_indexes will skip any tokens that do not have a type of :CLASSREF because we know that a defaults declaration will always start with a capital letter (See puppet-lint.com for more information on token types).

Potential fixes

There are a couple of ways we could approach a fix:

1. I think that the resource_indexes method would benefit from a refactor.. however I don't think this is what we should do to address this particular issue
2. We could insert the following guard clause that will skip the current iteration if a :CLASSREF is detected
   next if colon_token.prev_code_token.prev_code_token.prev_code_token.type == :CLASSREF
3. The same as above but we iterate back until we find what we are looking for (or not)

    def is_classref?(token)
      current_token = token
      while true
        current_token = current_token.prev_code_token

        return true if current_token.type == :CLASSREF
        return false if current_token.type == :NAME
      end
    end

(there are probably more elegant ways to achieve the above)

then

next if is_classref?(colon_token)

I think thats all of the info I have on this.. but will update if I get anything else interesting.

Edit: The more I think about it, i'm pretty sure we can confidently say that the "NAME" is going to be three code tokens back no matter what the resource is so I wonder if #2 is actually fine?

Edit2: Ran through some scenarios with option two and it (obviously) has some short comings.. which means a version of #3 is probably needed.

[ekohl]

I took the liberty to edit it to get syntax highlighting.

I think one problem here is that puppet-lint is a lexer, not a parser. You actually want to operate on a higher level than tokens, but then apply the fix still based on tokens.

Your solution of backtracking is probably the right one, but please do add some safety and don't assume prev_code_token is non-nil.

Edit: The more I think about it, i'm pretty sure we can confidently say that the "NAME" is going to be three code tokens back no matter what the resource is so I wonder if #2 is actually fine?

I'd check with these test cases:

service {
  'foo':
    ensure => running,
}

service {
  'bar':
    ensure => running;
  'foobar':
    ensure => stopped;
}

service { ['first', 'second']:
  ensure => running,
}

[chelnak]

@ekohl yes based on edit2 of my comment backtracking is the right option. Also the PR i'm about to push has a guard clause in to catch nils.. the above was just and example.

[chelnak]

Here is a PR with a potential fix: puppetlabs/puppet-lint#93

Note that this wont actually fix this plugin because it currently overrides many of the now exposed methods from puppet-lint.

For backwards compatibility do you think we should patch this plugin too @ekohl @bastelfreak ?