fix(parser): handle empty maybeAlias

[antongolub]

closes #160

[tommy-mitchell]

Do you have a test case for reproduction? Or could you give a small example? I'm not sure which test it failed on in the zx repo.

[antongolub]

google/zx@2dbf8e8
https://github.com/google/zx/tree/dev/test-d

Seems it fails on any attempt to load a test. Any test. So it's hard to say what exaclty is broken.

[tommy-mitchell]

Weird. And does this fixing maybeAlias work? The CLI also reads a flags property:

https://github.com/SamVerschueren/tsd/blob/42a2b2bdaae70beb1cec9f2c01257dc67a09f37f/source/cli.ts#L45-L49

Though maybeAlias.flags being undefined is more likely.

[antongolub]

I've checked this point too. Then found this:

            // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
            const maybeAlias = checker.getSymbolAtLocation(expression);
            console.log('!!!', maybeAlias)

    members: Map(4) {
      '__new' => [SymbolObject],
      '__call' => [SymbolObject],
      'isArray' => [Circular *1],
      'prototype' => [SymbolObject]
    },
    parent: undefined,
    mergeId: 7
  },
  id: 48790
}
!!! undefined
Cannot read properties of undefined (reading 'flags')

[tommy-mitchell]

Right then, guess it is that. @sindresorhus I assumed here that since we have a valid node, checker.getSymbolAtLocation(expression) would always be defined:

https://github.com/SamVerschueren/tsd/blob/42a2b2bdaae70beb1cec9f2c01257dc67a09f37f/source/lib/parser.ts#L20-L34

I'm not sure how to reproduce when the Symbol is undefined, but seems that right now we should not assume it is. I think a simple if is clearer than null chaining:

const maybeAlias = checker.getSymbolAtLocation(expression)

if (maybeAlias) {
    // ...
}

[antongolub]

My first thought was to add fail-fast. But it's not clear to me: should it be just return or forEachChild(node, walkNodes) is needed anyway.

if (!maybeAlias) {
    // ...
}

[tommy-mitchell]

I suppose a return would be fine since that means the current node doesn't have a Symbol, but I'm not keen on tempting fate again lol. Simplest to just move the logic into the if and change the name of maybeAlias to be more descriptive:

const maybeSymbol = checker.getSymbolAtLocation(expression);
			
if (maybeSymbol) {
	const symbol = maybeSymbol.flags & SymbolFlags.Alias ?
		checker.getAliasedSymbol(maybeSymbol) :
		maybeSymbol;

	// ...
}

[tommy-mitchell]

Suggested change

	// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
	const maybeAlias = checker.getSymbolAtLocation(expression)!;
	const maybeAlias = checker.getSymbolAtLocation(expression);

[tommy-mitchell]

Looks good to me. Leaving a suggestion here for future refactoring:

function handleNode(node: CallExpression) {
    // ...

    const maybeSymbol = checker.getSymbolAtLocation(expression);

    if (!maybeSymbol) {
        return;
    }

    // ...
}

function walkNodes(node: Node) {
    if (isCallExpression(node)) {
        handleNode(node);
    }

    forEachChild(node, walkNodes);
}