Syntactic impl declaration matching updates

[josh11b]

• Implement ignoring the difference between Self as and as, as well as where clauses at the end of an impl declaration when checking whether impl declarations match, from Matching redeclarations #3763.
• Allow impl declarations with different constraint ids to match, as long as the facet type of the constraint has the same interface_id and specific_id.
• Add some TODOs reflecting future facet type resolution.

[zygoloid]

Would it be better to defer doing this until we build the DeclParams objects in MergeImplRedecl? That'd mean we don't need to do the scan here until we're about to compare two token sequences anyway, which would avoid doing this scan entirely in cases where the impl isn't in the same bucket as any other impl (which is probably a common case), and would likely increase locality given that we'd be doing two checks of nearby tokens at around the same time.

[josh11b]

@jonmeow What do you think?

[jonmeow]

To try to offer a few upsides to the current approach:

• When we're doing it here the nodes will have recently been loaded [for checking]. So from a cache locality perspective, these are the nodes we just checked and may still be in cache. If deferred, it may result in overlapping impls being reevaluated (I don't know how expensive that'd be).
• This uses a reverse iteration instead of a forward iteration, whereas MergeImplRedecl uses a forward iteration, so may not cache well if the range turns out "large".
• By doing it here, does that make it easier to save the results so that the scan won't be repeated?

But maybe I'd bring up a more radical option: in WhereExpr handling, if there's an impl on the stack and it hasn't reached a where clause, had you considered trying to store the where location? Maybe that could be annotated as part of decl_introducer_state_stack. Could that give the result you want without the scan of nodes?

[josh11b]

I agree that we could avoid the scan by doing saving something in the WhereExpr handling, but it seems really fragile. There are a lot of places where a where clause can occur in an impl declaration that we don't want to truncate at.

impl forall [T:! I where...] ...
impl DynPtr(Container where .ElementType = i32) as ...
impl ... as J(K where ...)
impl ... as (... where ...) where ...

I think the current code is easier to get right since it directly looks in the one place we want to operate on.

[jonmeow]

In each of the cases you're showing, it seems like there'd be something on the NodeStack indicating the nesting:

• [ at [T:!
• ( at (Container
• ( at (K
• ( at (... where

Maybe it's worth thinking about further?

[jonmeow]

Put differently, is there a characteristic here where 2 nodes before the where must be something like forall or as?

[jonmeow]

To try to offer a few upsides to the current approach:

• When we're doing it here the nodes will have recently been loaded [for checking]. So from a cache locality perspective, these are the nodes we just checked and may still be in cache. If deferred, it may result in overlapping impls being reevaluated (I don't know how expensive that'd be).
• This uses a reverse iteration instead of a forward iteration, whereas MergeImplRedecl uses a forward iteration, so may not cache well if the range turns out "large".
• By doing it here, does that make it easier to save the results so that the scan won't be repeated?

But maybe I'd bring up a more radical option: in WhereExpr handling, if there's an impl on the stack and it hasn't reached a where clause, had you considered trying to store the where location? Maybe that could be annotated as part of decl_introducer_state_stack. Could that give the result you want without the scan of nodes?

[jonmeow]

It looks like you wrap this just to write the CHECKs. Had you considered instead only doing the iterator construction inside the CHECK?

I'm particularly wondering about this because whereas last_param_iter is mutated, this isn't, so having it be an iterator and stored in first_param_node_id took me time to understand the code.

[josh11b]

I've made the change so you can see how it looks. I'm neutral -- this new version is definitely longer/noiser, but if that makes it easier to understand that is an improvement.

[jonmeow]

Another option I was kind of wondering about is if we should just make NodeId inherit from IndexBase, allowing you to do the comparison directly (converting the iterator to a nodeid with just *). Thoughts?

[jonmeow]

What do you think about using a while to avoid the copied CHECK? You could also move in --last_param_iter; like so:

Suggested change

	--last_param_iter;
	CARBON_CHECK(first_param_iter < last_param_iter);
	do {
	node_kind = context.parse_tree().node_kind(*last_param_iter);
	if (node_kind == Parse::NodeKind::WhereExpr) {
	// If we have a nested `where`, we need to see another `WhereOperand`
	// before we find the one that matches our original `WhereExpr` node.
	++where_operands_to_skip;
	} else if (node_kind == Parse::NodeKind::WhereOperand) {
	--where_operands_to_skip;
	}
	--last_param_iter;
	CARBON_CHECK(first_param_iter < last_param_iter);
	} while (where_operands_to_skip > 0);
	while (where_operands_to_skip > 0) {
	--last_param_iter;
	CARBON_CHECK(first_param_iter < last_param_iter);
	node_kind = context.parse_tree().node_kind(*last_param_iter);
	if (node_kind == Parse::NodeKind::WhereExpr) {
	// If we have a nested `where`, we need to see another `WhereOperand`
	// before we find the one that matches our original `WhereExpr` node.
	++where_operands_to_skip;
	} else if (node_kind == Parse::NodeKind::WhereOperand) {
	--where_operands_to_skip;
	}
	}

[josh11b]

The problem is that code isn't quite the same since it doesn't skip the final WhereOperand node. Which we want so that Foo where... compares equal to Foo without a where. The code that doesn't repeat the decrement and CHECK does:

while (true) {
  --last_param_iter;
  CARBON_CHECK(first_param_iter < last_param_iter);
  if (where_operands_to_skip == 0) {
    break;
  }              
  node_kind = context.parse_tree().node_kind(*last_param_iter);
  // ...

which isn't shorter, but maybe the lack of repetition makes the code more robust? I'm not sure. Unfortunately C++ doesn't have a convenient control flow construct for loop-and-a-half

[jonmeow]

FWIW I'd lean towards that, although maybe I'm being biased about do/while. Ultimately I don't have a strong preference (although will Carbon have do/while?)

[jonmeow]

LG -- discussed the as lookup approach offline, approving and merging for parallelism.