Extend addPropertyToElementList to differentiate get/set accessors from properties
#54935
Conversation
typescript-bot
added
the
For Milestone Bug
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at faf9843. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at faf9843. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at faf9843. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at faf9843. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at faf9843. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at faf9843. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
CompilerComparison Report - main..54935
System
Hosts
Scenarios
TSServerComparison Report - main..54935
System
Hosts
Scenarios
StartupComparison Report - main..54935
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
The rxjs-src errors occur in the latest nightly, too ( |
|
You can ignore the rxjs thing; the bot is apparently not smart enough to handle paths in error elaborations. |
src/compiler/checker.ts
Outdated
| propertyTypeNode | ||
| )], | ||
| /*body*/ undefined); | ||
| typeElements.push(preserveCommentsOn(setAccessorSignature)); |
There was a problem hiding this comment.
So, as-is, by using preserveCommentsOn in both the get and set branches, this'll duplicate comments from the valueDeclaration of the symbol onto both the emitted get and set accessors. You probably wanna do something a bit more bespoke that lookups up the get/set accessor declarations and copies the comments from that specific matching declaration kind. setCommentRange(setAccessorSignature, setAccessorDecl) should do.
There was a problem hiding this comment.
Thanks @weswigham. I've made the change you suggested, and also added a test for it.
Because I don't think any of them are actually valid. Also remove a debugger statement that snuck in accidentally.
|
I also made another change to this PR, just FYI. Previously it would add all the modifiers from the source properties, except for a list I knew to be problematic. But then I ran into at least one more problematic one ( Not sure if typescript-bot will respond to me, but wanted to try: |
|
I can repack it if you want; the bot only responds to team members given it'd not be so great for arbitrary PRs to get published to npm without human interaction. @typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 616c614. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
src/compiler/checker.ts
Outdated
| /*dotDotDotToken*/ undefined, | ||
| parameterName, | ||
| /*questionToken*/ undefined, | ||
| propertyTypeNode |
There was a problem hiding this comment.
Rather than reusing the propertyTypeNode, we'd need to at least (deep) clone it - potentially putting the same node in the tree at two places can have funky results sometimes. But, better yet, would be fixing the differing get/set type bug while we're here! You can get the seter type by using getWriteTypeOfSymbol on the propertySymbol, which you can then feed through serializeTypeForDeclaration, same as is done for the reading type. You might need to duplicate the getNonMissingTypeOfSymbol into a getNonMissingWriteTypeOfSymbol to handle the missing type (the special undefined-like type we represent non-existent properties with in some compiler settings) correctly, though.
There was a problem hiding this comment.
Makes sense, @weswigham. I should be able to implement it this weekend.
There was a problem hiding this comment.
This is done, and I think the updated baselines demonstrate it's working. I wasn't sure if any of the createElidedInformationPlaceholder or propertyIsReverseMapped stuff applies to the setter type. If the answer is "yes" (or even "maybe"), let me know and I'll dig deeper. Thanks for the ongoing review!
|
@sandersn just a bump on this because it's in the "Waiting on Author" state, but I don't think it's actually waiting on anything from me anymore. Sorry for the noise if I just need to be more patient! |
|
Woops, I misread the comment history. I moved it back to Waiting on Reviewers. |
|
@sandersn / @weswigham any update on this? Seems like it's been waiting for review for a while and I just ran in to the same issue it's trying to solve. Thanks for your help! |
|
This seems to change too much for what it's doing -- there's no need to modify all object type emit here when the only problem has to do with |
7 participants
Fixes #54879
Please don't be alarmed by the large number of changed files here. The code change is only to the
addPropertyToElementListfunction inchecker.ts. The rest of the changes update the baseline and fourslash files for the large number of cases where the compiler was previously incorrectly (AFAICT) emitting regular properties where it should have emitted get or set accessors.I've looked at all the baseline changes in detail and I believe them to now be more correct. I hope you will agree!
As I undersand it, prior to TS 3.7 this difference between properties and accessors was mostly moot. But more recently it has become quite important (TS2611 errors), and so this PR makes the compiler emit the accessors in a significant set of cases where it should.
One possibly important thing I noticed while reviewing the baseline changes...
tests/cases/compiler/divergentAccessorsTypes6.tshas some code that looks like this:Before this PR, the type of
o2was:o2 : { p2: number; }Now it is:
o2 : { get p2(): number; set p2(value: number); }The original (before this PR) was wrong. It didn't capture the divergent accessor. The version in this PR is wrong, too, in much the same way. But because the accessors are separate now, maybe it's a little more problematic for the setter's type be wrong. Let me know if you think this needs to be fixed as part of this PR.