(PUP-11315) Allow resource attributes named "plan" and "apply"

[nicklewis]

Previously, using either of these keywords as resource attribute names
within an apply() block in a plan would fail. That happened because they
weren't included in the keyword rule. This was only a problem when
running in a plan, because those keywords only exist when the tasks
setting is set. In normal Puppet operation, they aren't lexed as special
tokens at all, so they didn't need to be in the keyword rule to function
properly.

We now add them to the keyword rule regardless of whether they are
actually lexed as keywords, which works fine in both cases.

[CLAassistant]

All committers have signed the CLA.

[nicklewis]

This came up because of https://github.com/hercules-team/augeasproviders_sysctl which defines a parameter called apply.

[nicklewis]

These tests actually pass without the change, because :tasks is false.

[lucywyman]

Does this need to target 6.x?

[lucywyman]

Confirmed this addresses the original issue of apply in a Puppet resource erroring inside Bolt plans 🙌🏼

[donoghuc]

I do think this needs to land in 6.x so we can use it in PE.

[nicklewis]

Rebased and retargeted at 6.x

[joshcooper]

Lemme try closing and reopening to rekick the checks

[joshcooper]

Ignore the appyeyor issue, I think it's just confused from the rebase.

I'm not super familiar with the lexer/parser, so to make sure I understand, PUP-8977 added the apply token for plans. It was added as a TOKEN to the grammar, but not to the list of keywords So when running a plan that used apply as a resource parameter, it resulted in a syntax error. And by adding it to the keywords hash, it'll get lexed as a keyword?

Is the general rule that all TOKENs like IF, ELSE, CASE, etc that are not operators, literals, etc need to also be a keyword?

I also noticed the grammar defines a reserved_word production, should apply and plan be added to that instead of keyword? For example, the puppet-specification groups private, attr along with plan and apply in https://github.com/puppetlabs/puppet-specifications/blob/master/language/lexical_structure.md#keywords

[nicklewis]

This case is particularly confusing because the grammar is static but the lexer varies based on whether Puppet[:tasks] is set.

If Puppet[:tasks] is false, the string apply will just be lexed as a WORD. That means that any rules that refer to the APPLY token will never be matched, since that token doesn't appear in the input. That's what causes apply_expression not to be produced when Puppet[:tasks] is false. Since apply is just a WORD, apply() { ... } is parsed no differently from foobar() { ... } (which is to say, it's a syntax error). That also means that resource_type { resource_title: apply => "value" } is also parsed no differently from resource_type { resource_title: foobar => "value" }.

When Puppet[:tasks] is true, the string apply is lexed as an APPLY token. That means if it appears in the input, the grammar will try to match an apply_expression rule (the only place that token appears). If it's not in an apply_expression, it will be a syntax error.

This change adds that token to keyword, meaning that in addition to appearing in the apply_expression rule, APPLY can also appear anywhere a keyword is allowed. The only place keyword is used is in attribute_name, where we say an attribute name can be either a NAME or a keyword. So when the parser encounters an APPLY token and it's not in an apply_expression, it knows that it could be in an attribute_name expression instead.

You're right that the general rule is that all keywords need to be in keyword, so that they can be used as an attribute name. This means you can also have resource attributes named IF, AND, etc.

I'm not sure exactly what the reserved_word rule is about. It basically serves to make it an error to use those words, which I guess is because we don't know how they'll be used in the future, so we want to forbid them entirely (even as attribute names), so we don't wind up in a bind if our future usage of them turns out to be incompatible with using them as attribute names. I don't think we want to reserve apply or plan, because they're not longer "for future use"; we actually know what they're for.

Because the use of apply and plan as attribute names is not incompatible with their main function (it doesn't create a
conflict in the parser), a better implementation of the Puppet[:tasks]-specific behavior might be to always lex those tokens and simply restrict their usage in the validator (which is how we restrict tasks-incompatible behavior when Puppet[:tasks] is enabled). But that's a more substantial change...

[joshcooper]

Ahh, thanks @nicklewis for a great explanation! I missed that the attribute_name rule produced a NAME or keyword. But yeah makes sense.