Add type checking for ident expressions

[jafaircl]

This PR adds type checking for ident expressions

[srikrsna-buf]

I think we should always resolve candidate names because there is a well defined order for them

Suggested change

	const ident = this.env.variables.find(name);
	if (ident) {
	return ident;
	}

[jafaircl]

I think when we get to macros it will be an issue. The macro vars can shadow global namespaced vars

[jafaircl]

Revisiting the go code this is based on, they make it clearer that this early return should be a local variable and explicitly not a global. I've updated this method and VariableScope to make the different flavors of variable resolution more explicit.

[srikrsna-buf]

From the Name Resolution section of the spec:

Note: Comprehensions (.exists, .all, etc) introduce new scopes that add variables as simple identifiers. When resolving a name within a comprehension body, the name is first compared against the variables declared by the comprehension. If there is a match, the name resolves to that variable, taking precedence over the package-based resolution rules above. If not, resolution proceeds checking for variable matches in parent comprehension scopes recursively. Finally the name follows the package resolution rules above against declarations in the environment. A name with a leading '.' always resolves in the root scope, bypassing local scopes from comprehensions.
For example, in [1].exists(x, x == 1), x is a local variable which shadows any identifier named 'x' in ancestor scopes or the package namespace. In [1].exists(x, .x == 1), x is the global variable x.

My understanding of this is to have scope chain for comprehensions and first look in them if the variable name doesn't start with .. If not found in the comprehension scopes or if name starts with ., follow the package resolution rules and look for them in the root scope.

I think we can simplify this by separating the VariableScopes for comprehensions and simply using the root scope as a backup. This gives a clear direction of how it is being used. The root variables need not be in a scope either. They can simply be a readonly map and the scope type can be internal.

[jafaircl]

My understanding of this is to have scope chain for comprehensions and first look in them if the variable name doesn't start with .. If not found in the comprehension scopes or if name starts with ., follow the package resolution rules and look for them in the root scope.

This explains some of my confusion about the cel-go code regarding when requiresDisambiguation would be necessary. We can implement in the same way if that sounds good. A private attributeResolution type extends CelType and the dot prefix can be added/removed as necessary to resolve variables while retaining the original intention from the source expression.

I think we can simplify this by separating the VariableScopes for comprehensions and simply using the root scope as a backup. This gives a clear direction of how it is being used. The root variables need not be in a scope either. They can simply be a readonly map and the scope type can be internal.

A VariableScope is basically a readonly map already. It seems like a distinction without a difference that will increase complexity to make the root scope use a different data structure than any other scope. I don't see the benefit in deviating significantly from the reference implementation in regards to this.

[srikrsna-buf]

I am not sure about the cel-go implementation but we can keep the VariableScope for now and use two separate chains, one an immutable root that expects the results of resolveCandidateNames and other a mutable chain which resets to an empty scope at the beginning of the type checking just like the two maps.

[jafaircl]

Ok just pushed some changes. I also found why I hadn't noticed that section of the spec before: it was just added last month

[srikrsna-buf]

We are always creating the scope so this need not be undefined. Instead we can create a singleton with no variables and use that as a reset.

[srikrsna-buf]

Inverting the check and throwing the error early will reduce nesting for the happy path

[srikrsna-buf]

We can slightly rewrite the function to avoid the boolean and return an optimized name when needed:

Suggested change

	private resolveSimpleVariableType(name: string): {
	type: CelType | undefined;
	requiresDisambiguation: boolean;
	} {
	const local = this.scope?.find(name);
	// Local variables that do not start with a "." shadow global variables
	// and do not require disambiguation.
	if (local !== undefined && !name.startsWith(".")) {
	return {
	type: local,
	requiresDisambiguation: false,
	};
	}
	for (const candidate of resolveCandidateNames(this.env.namespace, name)) {
	const type = this.env.variables.find(candidate);
	if (type) {
	return {
	type,
	requiresDisambiguation: local !== undefined,
	};
	}
	}
	return {
	type: undefined,
	requiresDisambiguation: false,
	};
	}
	}
	private resolveSimpleVariableType(name: string): {
	type: CelType;
	name: string;
	} | undefined {
	if (name.startsWith(".")) {
	const local = this.scope.find(name);
	// Local variables that do not start with a "." shadow global variables
	// and do not require disambiguation.
	if (local !== undefined) {
	return {
	type: local,
	name,
	};
	}
	}
	for (const candidate of resolveCandidateNames(this.env.namespace, name)) {
	const type = this.env.variables.find(candidate);
	if (type) {
	return {
	type,
	name: "." + candidate,
	};
	}
	}
	return undefined;
	}

And in the ident check we can simply use the name that we get back

[srikrsna-buf]

This seems wrong, we should not do the check if there is a .. See the function update comment above.