[analyzer] NodeReplacer throws when replacing the block of a catch clause

[shyndman]

Hi there,

I found a small issue in the implementation of NodeReplacer.visitCatchClause, which prevents catch cause replacement.

sdk/pkg/analyzer/lib/src/dart/ast/utilities.dart

Lines 1728 to 1740 in cb33212

	bool visitCatchClause(covariant CatchClauseImpl node) {
	if (identical(node.exceptionType, _oldNode)) {
	node.exceptionType = _newNode as TypeAnnotation;
	return true;
	} else if (identical(node.exceptionParameter, _oldNode)) {
	node.exceptionParameter = _newNode as SimpleIdentifier;
	return true;
	} else if (identical(node.stackTraceParameter, _oldNode)) {
	node.stackTraceParameter = _newNode as SimpleIdentifier;
	return true;
	}
	return visitNode(node);
	}

You'll notice that there is no handling for the catch's body block, and as a result the code hits visitNode, and visitNode throws.

It's an easy fix. Happy to send a PR your way. For now I've gotten around it with a subclass.

Thanks!

[bwilkerson]

Glad you found a workaround. A PR would be much appreciated.

[shyndman]

So I put together a test case, ran it, and oddly, it didn't fail.

Turns out some code was added somewhat recently that invalidates the testing approach — the first if statement in this snippet: (line 3012–3014)

sdk/pkg/analyzer/lib/src/dart/ast/utilities.dart

Lines 3011 to 3022 in cb33212

	static bool replace(AstNode oldNode, AstNode newNode, {AstNode? parent}) {
	if (identical(oldNode, newNode)) {
	return true;
	}
	parent ??= oldNode.parent;
	if (parent == null) {
	throw ArgumentError("The old node is not a child of another node");
	}
	NodeReplacer replacer = NodeReplacer(oldNode, newNode);
	return parent.accept(replacer)!;
	}
	}

The NodeReplacer tests work by replacing a node with itself, then verifying that the tree isn't broken. With that early exit, the node replacement code isn't running.

BUT, on the upside, I commented out the early exit and tests are all passing (except for the one I added, yay!),.

Fixing this is a bit beyond the scope of what I'm wanting to do. Would you like me to send a PR for my catch body change, along with a test that will work when this other issue is resolved?

[bwilkerson]

@scheglov @srawlins

[shyndman]

So...still send the PR? I can at least fix one of the bugs.

[scheglov]

I will fix it, there is some work to be done in better testing.

[shyndman]

sg. Thanks.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/220806