Defaults for case statements and selectors should not be needed when checking an Enum

[alexjfisher]

From https://puppet.com/docs/puppet/7/style_guide.html#style_guide_conditionals-case-selector-defaults

Case statements must have default cases.

IMO, if the control expression is a class/type parameter with type Enum then a default case should not be included if cases for all the valid values have been.

class example(
  Enum['foo','bar'] $param,
) {
  case $param {
    'foo': { include example::foo }
    'bar': { include example::bar }
    default: { fail('why are we mandating this unreachable block?') }
  }
}

[bastelfreak]

I run into this a few times as well (and puppet-lint complaining about the missing default). I agree with you and think that there doesn't need to be a default case.

[ekohl]

For your specific example, I think you can also write include "example::${param}" instead of a case statement, but I agree this comes up regularly.

Today puppet-lint enforces the style, but to properly enforce your new suggestion I think puppet-lint needs to track the data type and all branches of the case statement to see if there's complete coverage. If not, it should fail telling the user there is an undefined branch.

Or put into concrete terms (coming up with test cases here), let's say we have:

class example (
  Enum['foo','bar'] $param,
) {
  case $param {
    'foo': { include example::foo }
    'bar': { include example::bar }
  }
}

This is a good case: all branches are covered and should pass because there is no undefined behavior.

Now imagine it's changed:

class example (
  Enum['foo','bar', 'boo'] $param,
) {
  case $param {
    'foo': { include example::foo }
    'bar': { include example::bar }
  }
}

This should fail: it either needs to define 'boo' or default.

Until now it's all simple because everything is in the same file, but let's imagine we have type aliases:

class example (
  Example::CustomType $param,
) {
  case $param {
    'foo': { include example::foo }
    'bar': { include example::bar }
  }
}

Today the linter is mostly a simple lexer and you work on tokens while building the logic on top of it. To properly enforce this, the linter needs to become a real parser. And I certainly have seen cases where working on an AST is much better.

So while I think in theory you have a good point, in practice there are some challenges in enforcing the style via puppet-lint. On the other hand: we already have a real parser: Puppet itself.

Today we have puppet parser as a subcommand. This implements validate and dump commands. I can imagine that an analyze command would do a deeper analysis. While a tool could parse the output of dump, it says it's the internal parse tree so probably not a guaranteed stable API.

[alexjfisher]

For your specific example, I think you can also write include "example::${param}"

Yeah, sorry about that noddy example. Should have just stuck a comment in those blocks instead.

How or if you could even implement this in puppet-lint is maybe separate from the question of what the styleguide should say?

Implementation is unlikely to be perfect. As you point out, the parameter might use a custom type that is actually an Enum, but also the matches aren't always going to be single strings. You can use regexes, data types and type aliases there too. In the case that the parameter is a type alias, perhaps enforcing a default is actually desirable. If the type is in a different file, the default is a good guard against new cases being added without case/selectors being updated.