Ensure export= symbol from JavaScript always has a valueDeclaration

[andrewbranch]

I encountered this bug while chasing #34481, but I think it’s ultimately unrelated (though might be @Jack-Works’ bug in that thread?)

There’s a preexisting assumption that the export= symbol always has a valueDeclaration (see #26973), but exports of aliases in JS weren’t setting valueDeclarations. I’m not sure if doing this without the SymbolFlags.Value flag is correct—I’m not 100% clear on what the criteria for setting valueDeclaration is supposed to be.

[weswigham]

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?

[sheetalkamat]

Doesn't this get fixed by #34543

[sandersn]

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

[sandersn]

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

[sandersn]

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.

[andrewbranch]

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)

[andrewbranch]

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?

[sandersn]

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.

[andrewbranch]

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?

[sandersn]

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

[weswigham]

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.