Update to work with puppet-lint >= 2.4

puppet-lint 2.4 introduced what was supposed to be a non-breaking change
that fixed how interpolated expressions were tokenised, but this
unfortunately broke the check in this plugin (I'm going to have to
rethink how plugins factor into my change classifications for semver
purposes).

Anyway, this PR should fix up the issues with this plugin and
puppet-lint 2.4.x by extending the existing logic to handle cases where
variables like `$facts['osfamily']` are represented internally as
a sequence of tokens rather than a single token.

lib/puppet-lint/plugins/legacy_facts.rb

@@ -96,6 +96,14 @@
     'xendomains'                  => "facts['xen']['domains']",
     'zonename'                    => "facts['solaris_zones']['current']",
   }
+
+  # A list of valid hash key token types
+  HASH_KEY_TYPES = Set[
+    :STRING,  # Double quoted string
+    :SSTRING, # Single quoted string
+    :NAME,    # Unquoted single word
+  ].freeze
+
   def check
     tokens.select { |x| LEGACY_FACTS_VAR_TYPES.include?(x.type) }.each do |token|
       fact_name = ''
@@ -108,36 +116,50 @@ def check
       # This matches using legacy facts in a the new structured fact. For
       # example this would match 'uuid' in $facts['uuid'] so it can be converted
       # to facts['dmi']['product']['uuid']"
-      elsif token.value.start_with?("facts['") then
+      elsif token.value == 'facts' then
+        fact_name = hash_key_for(token)
+
+      elsif token.value.start_with?("facts['")
         fact_name = token.value.match(/facts\['(.*)'\]/)[1]
       end
 
       if EASY_FACTS.include?(fact_name) or UNCONVERTIBLE_FACTS.include?(fact_name) or fact_name.match(Regexp.union(REGEX_FACTS)) then
         notify :warning, {
-          :message => 'legacy fact',
-          :line    => token.line,
-          :column  => token.column,
-          :token   => token,
+          :message   => 'legacy fact',
+          :line      => token.line,
+          :column    => token.column,
+          :token     => token,
+          :fact_name => fact_name,
         }
       end
     end
   end
 
-  def fix(problem)
-    # This probably should never occur, but if it does then bail out:
-    raise PuppetLint::NoFix if problem[:token].raw and problem[:token].value != problem[:token].raw
+  # If the variable is using the $facts hash represented internally by multiple
+  # tokens, this helper simplifies accessing the hash key.
+  def hash_key_for(token)
+    lbrack_token = token.next_code_token
+    return '' unless lbrack_token && lbrack_token.type == :LBRACK
+
+    key_token = lbrack_token.next_code_token
+    return '' unless key_token && HASH_KEY_TYPES.include?(key_token.type)
 
-    # Get rid of the top scope before we do our work. We don't need to
-    # preserve it because it won't work with the new structured facts.
-    if problem[:token].value.start_with?('::') then
-      fact_name = problem[:token].value.sub(/^::/, '')
+    key_token.value
+  end
 
-    # This matches using legacy facts in a the new structured fact. For
-    # example this would match 'uuid' in $facts['uuid'] so it can be converted
-    # to facts['dmi']['product']['uuid']"
-    elsif problem[:token].value.start_with?("facts['") then
-      fact_name = problem[:token].value.match(/facts\['(.*)'\]/)[1]
+  def fix(problem)
+    fact_name = problem[:fact_name]
+
+    # Check if the variable is using the $facts hash represented internally by
+    # multiple tokens and remove the tokens for the old legacy key if so.
+    if problem[:token].value == 'facts'
+      loop do
+        t = problem[:token].next_token
+        remove_token(t)
+        break if t.type == :RBRACK
+      end
     end
+
     if EASY_FACTS.include?(fact_name)
       problem[:token].value = EASY_FACTS[fact_name]
     elsif fact_name.match(Regexp.union(REGEX_FACTS))
@@ -157,7 +179,5 @@ def fix(problem)
         problem[:token].value = "facts['solaris_zones']['zones']['" << m['name'] << "']['" << m['attribute'] << "']"
       end
     end
-
-    problem[:token].raw = problem[:token].value unless problem[:token].raw.nil?
   end
 end

puppet-lint-legacy_facts-check.gemspec

@@ -19,7 +19,7 @@ Gem::Specification.new do |spec|
   `$facts['os']['name']` instead
   EOF
 
-  spec.add_dependency             'puppet-lint', '2.3.6'
+  spec.add_dependency             'puppet-lint', '~> 2.4'
   spec.add_development_dependency 'rspec', '~> 3.0'
   spec.add_development_dependency 'rspec-its', '~> 1.0'
   spec.add_development_dependency 'rspec-json_expectations'

spec/puppet-lint/plugins/legacy_facts_spec.rb

@@ -106,6 +106,14 @@
         expect(problems).to have(1).problem
       end
     end
+
+    context 'fact variable using legacy facts hash variable in interpolation' do
+      let(:code) { %("${facts['osfamily']}") }
+
+      it 'detects a single problem' do
+        expect(problems).to have(1).problem
+      end
+    end
   end
 
 
@@ -391,5 +399,16 @@
         expect(manifest).to eq("\"$facts['os']['distro']['release']['specification']\"")
       end
     end
+    context "fact variable using facts hash in double quotes \"$facts['lsbrelease']\"" do
+      let(:code) { "\"${facts['lsbrelease']}\"" }
+
+      it 'should only detect a single problem' do
+        expect(problems).to have(1).problem
+      end
+
+      it 'should use the facts hash' do
+        expect(manifest).to eq("\"${facts['os']['distro']['release']['specification']}\"")
+      end
+    end
   end
 end