Save WhereOperand node instead of performing a scan

[jonmeow]

Follow-up to #4762

This changes to pushing ParenExprStart to make it easy for expressions to tell that they're nested (by breaking up sequencing on the node stack).

[jonmeow]

Note, this is based on #4772 but with the paren expr fix for the test.

Also a few small style things (FIXME -> TODO since I think it doesn't need to be addressed here, NodeId -> WhereOperandId because I think we prefer that (?), CARBON_CHECK that where is only found once because that's part of how I diagnosed -- prior code was "finding" it twice)

[josh11b]

In open discussion, we talked about checking the last node kind to make sure it is a WhereExpr.

Suggested change

	// Stop at the `where` node, if present, to exclude it from syntactic match.
	auto name = PopImplIntroducerAndParamsAsNameComponent(
	context, name_context.where_operand_node_id.is_valid()
	? Parse::NodeId(name_context.where_operand_node_id)
	: node_id);
	// Find the end node of the declaration for syntactic match.
	Parse::Tree::PostorderIterator last_param_iter(node_id);
	auto node_kind = context.parse_tree().node_kind(node_id);
	CARBON_CHECK(node_kind == Parse::NodeKind::ImplDefinitionStart ||
	node_kind == Parse::NodeKind::ImplDecl);
	// Subtracting 1 since we don't want to include the final `{`, `;`.
	--last_param_iter;
	// Stop at the `where` node, if present, to exclude it from syntactic match.
	if (context.parse_tree().node_kind(*last_param_iter) ==
	Parse::NodeKind::WhereExpr &&
	name_context.where_operand_node_id.is_valid()) {
	last_param_iter = name_context.where_operand_node_id;
	--last_param_iter;
	}
	// Pop the `impl` introducer and any `forall` parameters as a "name".
	auto name = PopImplIntroducerAndParamsAsNameComponent(
	context, *last_param_iter);

[jonmeow]

I think the discussed check was something like:

  CARBON_CHECK(name_context.where_operand_node_id.is_valid() ==
                   (context.parse_tree().node_kind(constraint_node) ==
                    Parse::NodeKind::WhereExpr),
               "where_operand_node_id: {0}; constraint node kind: {1}",
               name_context.where_operand_node_id,
               context.parse_tree().node_kind(constraint_node));

Note, one difference with your suggestion is that it validates the WhereExpr when where_operand_node_id is set, essentially a if-and-only-if. But I've gone a slightly different approach now, because I was realizing a similar thing could be achieved through the node stack at negligible storage cost (no cost for non-impls).

[jonmeow]

Was looking at this a little further, I've at least moved out the param iter logic as requested.

Given the type of node_id is AnyImplDeclId, is the CARBON_CHECK still needed? I was thinking on edit that it could be removed because the validation comes from the type already.

[josh11b]

Suggested change

// Pop the `impl` introducer and any `forall` parameters as a "name".

[jonmeow]

Sorry, I'm confused by this, can you clarify why this no longer applies?

[josh11b]

With the suggested change to toolchain/check/handle_impl.cpp, I think these changes should no longer be needed.

[jonmeow]

If this is removed, how does WhereExpr handling know when or when not to mark the WhereOperand? It cannot do that in normal expression handling, and this was added to distinguish between a nested expression and the impl form.

If you have an approach that works without this, maybe it'd make sense to update #4772 since this PR was mainly adding the paren expr tracking on top of your work?

[jonmeow]

Note, I'm suggesting a slightly different approach now, keeping the WhereOperand on the node_stack instead of popping it. This comes from talking about the side-channeling and how we couldn't just add an arg, I was thinking why pop the node off in the first place if we want to use it again? It also takes advantage of Pop checks to enforce the parity if we have an accident in tracking.