Don’t recognize non-top-level module.exports as special assignments

[andrewbranch]

Fixes #33765 the rest of the way

[sandersn]

One comment with four questions.

[sandersn]

previously, isTopLevel calls occurred outside getAssignmentDeclarationKind, mostly [1] so that we could call that once in the binder and not have to walk up the tree too much elsewhere -- at least at one point, getAssignmentDeclarationKind checked fairly local syntax.

So, three questions:

1. Does this make a difference in practise? We don't have any JS projects in the performance suite so have no way to measure this.
2. Is getAssignmentDeclarationKind actually called a lot in the checker?
3. Is getAssignmentDeclarationKind actually still pretty local?
4. Four. Four questions: Is this change worth it even if those answers are all "yes"? Probably, but I wanted to have you think about them too.

[1] it was partly chance. But lots of things are partly chance.

[andrewbranch]

1. You mean perf difference? I think it shouldn’t, since we check a lot of other things before resorting to looking up the tree. This only happens when we see an assignment to something related to exports. But you’re right, we don’t have metrics on this.
2. Yes, and a lot in services too. My initial instinct was to do this top-down from the binder, but I realized it would produce inconsistent results everywhere else :/
3. I don’t understand the question?
4. I’m actually not sure, honestly. It feels fairly unimportant since literally nobody has ever asked for this; it just came up as a correctness suggestion in Conditional assignments to exports should not mark a js file as a commonjs module #33765, which we only noticed because there was a regression related to element access assignments. The heuristics around what should be global feel really fuzzy to me in scenarios like this, which likely only appear in bundled or otherwise module-faking browser code. I’m not attached to this PR at all, and was actually hoping that putting it up would solicit enlightening feedback about why/whether this issue matters.

[sandersn]

3. Do the existing parts of getAssignmentDeclarationKind examine parents or children up and down the tree much? Or is this the first time it has started walking up the tree more than, say, one or two parents deep?

[andrewbranch]

Ah, yeah, this is the first time we look way up the tree.

[sandersn]

Any idea what the test failures are about?

[andrewbranch]

Any idea what the test failures are about?

Most of them are some kind of assertion that module.exports nested in various kinds of blocks actually works, e.g.

if (true) {
    exports.b = true;
} else {
    exports.n = 3;
}
function fn() {
    exports.s = 'foo';
}

So here we mark b and n as export property assignments, but not s. This seems bad. I’m going to close this, because, again, nobody is asking for it.