In source files and blocks, bind function declarations before other statements#22766
Conversation
(Inference of class members from this-assignments inside a prototype-assigned function.)
|
|
||
| Point.origin = new Point(0, 0); | ||
| ~~~~~~ | ||
| !!! error TS2339: Property 'origin' does not exist on type '(x: any, y: any) => any'. |
There was a problem hiding this comment.
This part of this test should probably be moved into a .js file in the test, to preserve original intent? (Which was that the ambient class declaration above provides contextual types for these functions)
There was a problem hiding this comment.
While I agree that moving this part into a .js file is the right thing, I don't understand why the ambient class should provide contextual types for the functions. At best they should merge, which is what they do now.
There was a problem hiding this comment.
Eh? Ok. I don't really have a strong opinion on it, since it feels like the kinda thing that maybe just feel out from other more intentioned choices before.
There was a problem hiding this comment.
Hm. Well, @DanielRosenwasser convinced me that contextual typing makes sense for the prototype method assignment, but not for the function declaration. I removed the conflicting function instead.
|
|
||
|
|
||
| ==== tests/cases/conformance/salsa/use.js (1 errors) ==== | ||
| var mini = require('./minimatch') |
There was a problem hiding this comment.
maybe add a .d.ts into the test that declares require and module, just so there's no unnecessary error baseline?
There was a problem hiding this comment.
I tried that already, and, because of the uses of require/exports, these files aren't in the global namespace any more. I forgot that I could use triple-slash references to work around that.
I still couldn't get rid of the error on assignment to exports.
weswigham
left a comment
There was a problem hiding this comment.
Seems OK, just small nits about tests.
Functions should be bound before other statements to match the hoisting behaviour of the Javascript runtime. In contrast,
vardeclarations should not be bound before other statements because property access isn't legal until after initialisation, and initialisation isn't hoisted the way it is with functions.This change isn't observable in correct Typescript code since we don't allow assignment of properties on the function. But this is common in Javascript, and some code bases use it frequently (webpack, npm, semver and minimist, in the survey I recently did).
Errors from the binder are somewhat order dependent, especially for complex merges. That means there is some error message churn.
Fixes #22725
Fixes #22438