(#14441) Require instrumentation indirections

Previously, the instrumentation faces were not requiring their
corresponding indirection, so the indirections were not getting
registered,
e.g. Puppet::Indirector::Indirection.instance(:instrumentation_data)
was nil.  This was causing `spec/integration/documentation_spec.rb` to
fail -- it was iterating across each face and trying to generate help,
but that failed because the indirection wasn't registered.  This issue
didn't show up earlier, because due to a separate issue, the tests
were not being executed.

This issue only occurs when using the instrumentation related classes
programmatically, but not when running the applications, e.g. `puppet
instrumentation_data` because the latter calls
`Puppet::Util::Instrumentation.init` early on in the application
lifecycle, and that requires all of the instrumentation code.

This commit changes each instrumentation-related face to require its
instrumentation indirection, e.g. Puppet::Face[:instrumentation_data]
requires `puppet/util/instrumentation/data`.

This commit doesn't address the overall issue with #14441 in that
Puppet::Util::Instrumentation.init` has to be invoked before it can be
used, so don't close the bug when this is merged.
Maint: Actually execute the tests on non-windows platforms

Previously, the tests were using conditional pending statements to
mark tests that don't pass on Windows:

  describe "group" do
    pending("porting to Windows", :if => ...) do
      it "should ..." do
        ...
      end
      it "should ..." do
        ...
      end
    end
  end

But this is not a valid use of conditional pending statements, and it
causes the examples within the pending block to be silently skipped.
Instead, the conditional pending statement needs to be within the
example:

  it "should ..." do
    pending("porting to Windows", :if => ...) do
      ...
    end
  end

In our case, we want to mark a set of examples as pending, so we use
an rspec exclude filter instead:

  describe "something", :unless => Puppet.features.microsoft_windows? do

Some tests now pass on Windows, using :file instead of :mount and the
instrumentation documentation, so I enabled them.
A node always has an environment.

Puppet node objects, now, always have environments, and they are the genuine
Ruby object representing an environment, never just the name.

In consequence of this, the code in the compiler to handle two conditions - no
environment, or an empty string as environment - that presumably could have
been true at some time in the past ... no longer makes sense.

Instead, replace it with a clear and direct failure in the event we don't get
the object we expect, and carry on.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
A scope always has a compiler.

The scope object contained some internal code to substitute an external
environment when the compiler was not supplied.  This was used only in
testing, not in any production capacity.

In light of that, we can eliminate the dynamic decision making inside the
scope and simply demand that a compiler instance (or a fake equivalent) is
always supplied.

This reduces the complexity of the code and makes clearer the object
relationships involved.

(The small cost is a lot of testing that depended on this had to change to use
 a real compiler.  Most of the change derives from that.)

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Drop the hash prefix when passing compiler to scope.

Now that the compiler is always required by the scope we can save a few
keystrokes by making the non-optional argument a regular value, not a name
prefixed option.

(...even if I love Objective-C as much as the next developer, having the same
 text in every call just to name the argument doesn't make much sense in such
 an internal context.)

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Eliminate an unused library from the scope.

Nothing in the scope uses the `strscan` library, so we can just drop it.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Use `Forwardable` rather than hand-rolling it.

Using the standard `forwardable` library makes for a clearer understand of
what is going on where in our code - if we just wrap directly around a method
on a member, we might as well delegate properly.

That way people viewing the code know, with certainty, that it isn't actually
doing anything of significance other than passing that call directly on.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Simplify the code handling the namespace option to the scope.

No functional changes - just a few less method calls required to achieve the
same result, so it becomes easier to see what the meaning is.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Use `alias` to map []{,=} to {lookup,set}var on scope.

Rather than hand-rolling an alias for the hash-like accessors to the named get
and set methods, we can just use the stock alias method.

This eliminates an extra layer of function calls, but also makes clearer to
code analysis and the casual reader what the actual significance of those
different methods are.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Treat `nil` as false, not true in scope and AST evaluation.

We had a central place defined in the scope to determine the truthiness of
objects from an AST / code evaluation point of view.  Since inception this
treated a `nil` value as *true*, in variance to Ruby behaviour.

While odd, this probably doesn't make sense to preserve, so add a test to
validate that `nil` becomes false, and stays that way.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Simplify the number? implementation of scope.

The `number?` method coerces an input into a number in a standard way for the
AST; this rewrites it to a simple, direct case statement rather than the
previous mess of if-then-else decisions.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Simplify the scope `to_hash` implementation.

Minor code cleanups to make it clearer what is going on; no functional changes.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Simplify the scope `emphemeral_include?` method.

The implementation had various complications, such as reverse-walking the
tree, that don't actually make any difference compared to a simple sweep
through in standard order.

This doesn't change overall behaviour, just makes the code more obvious.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Check that function loading worked correctly.

In some odd circumstances - typically, just development errors - you can wind
up in a world where a function correctly loads but isn't available.

This ensures that the method is defined as expected and raises a clear, quick
error if not. The alternative is an unclear stack overflow raised because we
recurse forever through method_missing.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
compiler: use forwardable where applicable.

The replaces the manual forwardings of methods through to a member with the
Forwardable module - it makes clearer the intent of the code.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Use `set_options` rather than open-coding it everywhere.

`Puppet::Util::MethodHelper` has a handful of tools for handling the common
pattern of accepting a hash and setting object attributes from it.  We also
had a bunch of places in the code that open-coded this same pattern.

This just replaces them all with the single, standard implementation.

Really, we should probably even hoist this up to something like Object or
Module so that it is available everywhere to peer with the `attr_*` methods
and all.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
ast: trivial indentation and line wrap fixes.

    nothing out of place
   exquisite imperfection
     each weed a flower

There are no functional changes here, just moving code around to look less
terrible in a few places.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
ast: eliminate dead `settor` attribute of AST nodes.

Once upon a time, AST entries that set something were evaluated before entries
that didn't.  That was eliminated on Fex 28, 2008, in fd1573fd, by Luke.

We might as well take the last, and misleading, vestige of that out of the
tree at this point.

Also, eliminate a couple of comment lines that have lingered from that era
attached to something randomly else.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
ast: native boolean stringification is just fine.

We don't need to manually override to_s on boolean objects to get the strings
true and false out of them - so don't bother with the override.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
ast: correctly quote strings in {Flat,}String nodes.

A string that contained an embedded double-quote wouldn't be correctly escaped
when the AST rendered the node to a flat string - it quoted around the
outside, but didn't escape the content.

This fixes that, and adds testing that embedded double-quotes are correctly
handled on output.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>