Avoid crash for import code fixes with dotted require

[DanielRosenwasser]

Fixes #47428.

Okay, here's the summary:

First, we have a type predicate in our services layer that determines whether something is an import statement or a variable initialized with a require call.

TypeScript/src/services/utilities.ts

Line 2032 in 0f1496f

const importKindPredicate: (node: Node) => node is AnyImportOrRequireStatement = decl.kind === SyntaxKind.VariableStatement ? isRequireVariableStatement : isAnyImportSyntax;

That type predicate is wrong because it takes a dependency on isRequireVariableStatement...

TypeScript/src/compiler/utilities.ts

Lines 2123 to 2127 in 0f1496f

	export function isRequireVariableStatement(node: Node): node is RequireVariableStatement {
	return isVariableStatement(node)
	&& node.declarationList.declarations.length > 0
	&& every(node.declarationList.declarations, decl => isRequireVariableDeclaration(decl));
	}

which relies on isRequireVariableDeclaration...

TypeScript/src/compiler/utilities.ts

Lines 2116 to 2121 in 0f1496f

	export function isRequireVariableDeclaration(node: Node): node is RequireVariableDeclaration {
	if (node.kind === SyntaxKind.BindingElement) {
	node = node.parent.parent;
	}
	return isVariableDeclaration(node) && !!node.initializer && isRequireCall(getLeftmostAccessExpression(node.initializer), /*requireStringLiteralLikeArgument*/ true);
	}

which returns true a variable like in const foo = require("yadda").foo.

Because the type predicate is wrong, we eventually end up crashing because we think we have a CallExpression on the initializer and can't access the first argument, resulting in:

TypeError: Cannot read property '0' of undefined

So my solution here is to split up the usages of the function - some for when a consumer can tolerate the property access version (require("yadda").foo), and some for tolerating the bare require("yadda") version. isRequireVariableStatement can safely be changed to rely on the bare version because it's only called by the type predicate function.

This at the very least avoids crashing for code-fixes without impacting go-to-definition, and other functionality in the type-checker itself.

[sandersn]

The fix seems good, I just don't like the new name at all.
I suggested a slightly less bad name, although it's still not good.

[sandersn]

I think I prefer RequireVariableDeclarationOrExpression

[jakebailey]

I'd agree (less easy to miss if you're doing completion and were about to use the old one, too).

[andrewbranch]

It’s not a “declaration or expression” though—it’s always a variable declaration, and the initializer of that variable declaration is always an expression. The thing that changes is whether that expression is a special call expression or a property access expression off that special call expression. I don’t like either of these names and don’t have a better suggestion ready, but I don’t understand what RequireVariableDeclarationOrExpression is supposed to mean.

[DanielRosenwasser]

I think I'm going to go with VariableDeclarationInitializedTo<T> with CallExpression and AccessExpression | CallExpression.

[jakebailey]

I'd agree (less easy to miss if you're doing completion and were about to use the old one, too).

[DanielRosenwasser]

I've renamed both functions uniformly so you can't miss them, though I've left isRequireVariableStatement alone. That can be a follow-up if people feel strongly enough about it.

[jakebailey]

Seems okay to me, but someone else may object to the naming, of course.

[DanielRosenwasser]

@typescript-bot cherry-pick this to release-4.5

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.5 on this PR at bf708bf. You can monitor the build here.

[typescript-bot]

Hey @DanielRosenwasser, I've opened #47515 for you.