Check for require when binding commonjs

[sandersn]

Previously, the binder didn't check for a declared function named 'require' when binding require in JS. It unconditionally treated 'require' as the commonjs function, even when a local function named 'require' existed and even in ES modules. Now, the binder checks for a local function named require and avoids binding in that case.

Both before and after, actually importing the module causes the checker to issue an error saying that top-level functions named 'require' are illegal. But this error doesn't show up until you've imported from the module, so you won't notice it in the editor, where JS errors are most useful.

Within-binder checks for declared functions are dependent on binding order, but function binding simulates hoisting by binding all function declarations first, so it should be pretty reliable.

Fixes #48726

[sandersn]

Note: The "'require' is reserved by the compiler" error has to do with emit, so it shouldn't be issued on any .js file. That's a separate issue though.

Edit: Filed #50132 for this
Edit edit: Nope, I'm wrong, the error is correct in this case.

[ProKashif]

we should move this in enum as we are doing in below

[sandersn]

Are you talking about an enum like ModifierFlags? require isn't a modifier like public et al, and I think if the binder gets this right, then the checker won't have to look up require again. Even if it does, it'll only happen once more. I don't think it's worth caching.

[jakebailey]

Will this end up breaking code that relied on the fact that we treat a function named "require" specially? I know that yarn plugins are passed a require function as an argument (so you can require things lazily / get the same dep as the executing yarn version), and having require recognized as the require function is helpful there.

[andrewbranch]

Similar question to Jake’s: does this check affect ambient declarations? Both @types/node and @types/webpack-env declare require.

[sandersn]

@andrewbranch No: lookupForSymbolName doesn't look in the global scope, so it won't affect files that reference those types. However, a declare function require() in the same file would block require use there, even though that's not legal JS syntax.

@jakebailey Yes: based on the test noCrashOnParameterNamedRequire. While technically correct, the assumption that require behaves like commonjs require is really useful for understanding code most of the time.

ES modules would be less likely to have a require passed to them, I think. Do you think this change makes sense if restricted to ES modules? I went ahead and did that, but if you can access require in an ES module (and if people commonly do), then even that probably doesn't make sense.

I looked at a couple of yarn plugins but they were written in TS, and I didn't see any require passed around.

Two more things:

1. This is a common pattern in bundled code too, but the references are to other regions in the same bundled file, so we've never been able to resolve them unless the bundle is in the same directory as the source.
2. I can't figure out how to trigger the "please don't name your function require" error in the editor — even in TS. Improving the usability of that seems to be a better alternative fix at this point.

[jakebailey]

I looked at a couple of yarn plugins but they were written in TS, and I didn't see any require passed around.

My example is in a private repo so I can't link it here, but here is the example from the yarn docs: https://yarnpkg.com/advanced/plugin-tutorial/#writing-our-first-plugin

[sandersn]

Closing this after various discussions. I think a warning against writing a function named require is better.