I'm conflicted on this - I'm not sure it's right to set a value declaration on something which lacks the flags to indicate that it's a value (so I question #26973, too). How do we fall into getTypeOfFuncClassEnumModule? It's because we have a bad merge, right, a case where an export assignment (which is an alias) is merged with an exported thing, and that merge is adding a value-meaning symbol flag, but neglecting to set a value declaration?

Doesn't this get fixed by #34543

No, still fails. I just tested it. =(

#34493 seems like it is exactly the same bug, but this change doesn't fix it. Not sure why yet.

What is the crash like here? I think this might be OK for a fix, but I would like to understand better why it was crashing.

How do we fall into getTypeOfFuncClassEnumModule? It's because we have a bad merge, right, a case where an export assignment (which is an alias) is merged with an exported thing, and that merge is adding a value-meaning symbol flag, but neglecting to set a value declaration?

Uhm, I don’t think so? getTypeOfFuncClassEnumModule isn’t in this stack, and the export= symbol lacking the valueDeclaration is not merged a merged symbol. If you replace this JS with the equivalent TypeScript, it works exactly the same, except the binder unconditionally sets a valueDeclaration on the ExportAssignment node, even though it points to an alias and lacks the SymbolFlags.Value flag. The stack is

     TypeError: Cannot read property 'kind' of undefined
      at getErrorSpanForNode (built/local/run.js:10632:22)
      at createDiagnosticForNodeInSourceFile (built/local/run.js:10592:20)
      at Object.createDiagnosticForNode (built/local/run.js:10583:16)
      at getTargetOfImportClause (built/local/run.js:34913:51)
      at getTargetOfAliasDeclaration (built/local/run.js:35082:28)
      at resolveAlias (built/local/run.js:35125:30)
      at checkAliasSymbol (built/local/run.js:62468:26)
      at checkImportBinding (built/local/run.js:62499:13)
      at checkImportDeclaration (built/local/run.js:62513:25)
      at checkSourceElementWorker (built/local/run.js:62888:28)
      at checkSourceElement (built/local/run.js:62728:17)
      at Object.forEach (built/local/run.js:325:30)
      at checkSourceFileWorker (built/local/run.js:63046:20)
      at checkSourceFile (built/local/run.js:63018:13)
      at getDiagnosticsWorker (built/local/run.js:63093:17)
      at Object.getDiagnostics (built/local/run.js:63079:24)
      at /Users/anbranc/Developer/microsoft/TypeScript/built/local/run.js:95685:85
      at runWithCancellationToken (built/local/run.js:95650:24)
      at getSemanticDiagnosticsForFileNoCache (built/local/run.js:95673:20)

The most surface-level solution would be to fall back to symbol.declarations[0] when valueDeclaration is undefined, but by reading past assertions that export= should always have a valueDeclaration, I thought that was too surface level, and might let future crashes through that make the same assumption. But it sounds like the codebase contains conflicting assertions:

• export= must always have a valueDeclaration
• a symbol that’s just an alias must not have a valueDeclaration

@weswigham is the latter an accurate representation of what you’re saying?

Given the stack, this sounds like the right fix to me -- this fails because a commonjs "alias" isn't a real alias, but we have to report errors on it correctly in the middle of module resolution.

this sounds like the right fix to me

By “this” do you mean the one I described in the previous comment, or the one currently in this PR?

The one in the PR, not using declarations[0].

If you replace this JS with the equivalent TypeScript, it works exactly the same, except the binder unconditionally sets a valueDeclaration on the ExportAssignment node,

Oh, well, I guess we should do the same. Seems odd to me, though - it means that you can have a .valueDeclaration for a thing that... Isn't a value.