-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Fix issue with ambient property declaration following property assignment in JS #52323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Oleksandr T <oleksandr.tarasiuk@outlook.com>
Semantic Diagnostics for file '/tests/cases/fourslash/quickInfoAndSemanticsOnJsPropertyWithAmbientDeclaration.ts': | ||
|
||
|
||
==== /test.js (0 errors) ==== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait. This is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that the errors don't come up consistently, and that baselining doesn't give the same error results, is very suspicious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. All tests I've had for this have not been compiler tests.
And now I have FOMO... |
I am sure you can find something that looks broken with our JavaScript support. |
Looks like @a-tarasyuk's PR makes a fix in the same place. |
Everyone is trying to fix #51521, so I guess I am too.
This PR tries to fix #51521. It does so by fixing
setValueDeclaration
to defer to property declarations over "assignment declarations" - even in the case of ambients. In the case of code like in the following.js
file:We'll encounter the
this.foo = 10
assignment first, then encounterdeclare foo: number;
.Even though
declare foo: number
is marked as ambient, it is arguably a better value declaration than the JavaScript one.