Support for nested expressions - 1st stage

[radeksimko]

Motivations

One of the common constraints in use is the following:

Attributes: map[string]*schema.AttributeSchema{
	"attr": {
		Expr: schema.ExprConstraints{
			schema.TraversalExpr{OfType: cty.String},
			schema.LiteralTypeExpr{Type: cty.String},
		},
	},
}

which expresses either a reference of string type (attr = var.mystring), or literal string (attr = "foo"). This has a few challenges:

1. It's very verbose for such a common constraint
2. It doesn't account for other expression types (beyond references and literals)
3. It makes it hard to provide reliable completion where there are >= 2 similar constraints. See hashicorp/hcl-lang#170 for more information.

To address the mentioned challenges, it is proposed to refactor expression constraints to eventually enable the same constraint to be expressed this way:

Attributes: map[string]*schema.AttributeSchema{
	"attr": {
		Constraint: schema.AnyExpression{OfType: cty.String},
	},
}

Name Change: Expr -> Constraint

The new name reflects that constraints in HCL will generally always apply to expressions (certainly in the context of an attribute), and should better convey the purpose of the field and make it easier to "say out loud" while typing.

Singular Constraint

It is proposed to optimize for singular constraints as that is the common case, and allow multiple constraints via a special constraint type. Hence the top level type (Expr / Constraint) will no longer be a slice, but Constraint interface.

New Constraints

Two new constraints are proposed for addition, of which only the first one is part of this PR:

• OneOf - equivalent to the existing default ExprConstraints, to be also used as a temporary replacement until AnyExpression is implemented
• AnyExpression{OfType: cty.Type} - to represent any expression which is expected to evaluate to a given type

Convenience Arguments

By using different name, we also gain the benefit of being able to work on this piece by piece, and not having to implement all expressions at the same time and face compilation errors. Adding Constraint is effectively a feature toggle.

Implementation Notes

This is the first step towards implementing hashicorp/terraform-ls#496

The main aim here is to agree on two [new] interfaces, schema.Constraint and decoder.Expression:

type Constraint interface {
	isConstraintImpl() constraintSigil
	FriendlyName() string
	Copy() Constraint
	// EmptyCompletionData provides completion data (to be used in text edits),
	// with snippet placeholder identifiers such as ${4} (if any) starting
	// from given nextPlaceholder.
	// 1 is the most appropriate placeholder, if none were yet assigned prior
	// to rendering completion data for this constraint (e.g. map key).
	EmptyCompletionData(nextPlaceholder int) CompletionData
}

type Expression interface {
	CompletionAtPos(ctx context.Context, pos hcl.Pos) []lang.Candidate
	HoverAtPos(ctx context.Context, pos hcl.Pos) *lang.HoverData
	SemanticTokens(ctx context.Context) []lang.SemanticToken
	ReferenceOrigins(ctx context.Context, allowSelfRefs bool) reference.Origins
	ReferenceTargets(ctx context.Context, attrAddr *schema.AttributeAddrSchema) reference.Targets
}

The PR intentionally leaves out most of the implementation of both interfaces for all expression/constraint types, as the diff is already quite large. It does however implement the simple methods like Copy() and FriendlyName() for schema.Constraints as these were mostly copy-pasted from the existing implementations.

There is a WIP staging ground for some implementations in https://github.com/hashicorp/hcl-lang/compare/f-expressions-staging-ground

The idea is that we can then raise separate PRs, where each focuses on each individual constraint/expression, or even different aspect of it (completion / hover / semantic tokens / reference origins / reference targets).

This PR is also leaving out addition of the planned new constraint, schema.AnyExpressionOfType or similar, which would effectively enable support for nested expressions. It should make adding such a constraint much easier/cleaner however.

Open Questions

• Should decoder.Expression.ReferenceOrigins() accept context as the first argument, like the other methods?
  • Should we also use that context to pass through "allowSelfRefs" somehow?
  • Should decoder.PathDecoder.CollectReferenceOrigins() also accept context, like other similar methods?
• Should decoder.Expression.ReferenceTargets() accept context as the first argument, like the other methods?
  • Should decoder.PathDecoder.CollectReferenceTargets() also accept context, like other similar methods?
• (non-blocking) Should Conform() be implemented for constraints other than LiteralType and OneOf?
  • What would this mean for Object? How should optional attributes be treated?
  • What are the possible use cases for that method? Will that remain only completion hooks?
  • Should we (ab)use this method also in Reflect convertible types in type comparison terraform-ls#583 ?
• Should we be updating tests, or copying them - i.e. should the old logic remain covered by tests in the meantime?

[dbanck]

Great work! I believe the changes will lead to a much more readable and easier extendable codebase. I've just found two minor things.

Regarding your questions:

Should decoder.Expression.ReferenceOrigins() accept context as the first argument, like the other methods?

I'm in favor as it will unify the interface, and we might need it anyway.

Should we also use that context to pass through "allowSelfRefs" somehow?

Maybe. But I would leave this open until we get to the implementation.

Should decoder.PathDecoder.CollectReferenceOrigins() also accept context, like other similar methods?

I'm against it for now as I couldn't find a good use-case yet.

Should decoder.Expression.ReferenceTargets() accept context as the first argument, like the other methods?

I'm in favor as it will unify the interface, and we might need it anyway.

Should decoder.PathDecoder.CollectReferenceTargets() also accept context, like other similar methods?

I'm against it for now as I couldn't find a good use-case yet.

Should we be updating tests, or copying them - i.e. should the old logic remain covered by tests in the meantime?

I'm in favor of copying tests and removing the old tests together with the old implementation. That should ensure we keep everything unbroken.

[radeksimko]

I believe I have addressed all concerns + I also decided to change the Conformable interface to Comparable, with the assumption that we will reuse it eventually for hashicorp/terraform-ls#583 - I can not see why hooks shouldn't take advantage of that potentially more loose interpretation of type compatibility.

PTAL

[dbanck]

Thanks! LGTM 👍