Uh oh!
There was an error while loading. Please reload this page.
Filter out classes with private members from the contextual types for object literal nodes - #56183
Conversation
… object literal nodes
| function containsNonPublicProperties(props: Symbol[]) { | ||
| return some(props, p => !!(getDeclarationModifierFlagsFromSymbol(p) & ModifierFlags.NonPublicAccessibilityModifier)); | ||
| return some(props, p => !!(getDeclarationModifierFlagsFromSymbol(p) & ModifierFlags.NonPublicAccessibilityModifier) || !!p.valueDeclaration && isNamedDeclaration(p.valueDeclaration) && isPrivateIdentifier(p.valueDeclaration.name)); |
There was a problem hiding this comment.
likely this alone (or something close to it) would fix the completion problem (private foo is already filtered out from completions, only #foo is not) but that wouldn't fix the contextuallyTypedParametersPositionIncludesClassWithPrivateMember test cases
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Jake Bailey (jakebailey)
commented
Nov 3, 2023
TypeScript Bot (@typescript-bot) test top200 TypeScript Bot (@typescript-bot) perf test this TypeScript Bot (@typescript-bot) user test tsserver |
Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at dcff0c7. You can monitor the build here. |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based user code test suite on this PR at dcff0c7. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based user code test suite (tsserver) on this PR at dcff0c7. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite on this PR at dcff0c7. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite (tsserver) on this PR at dcff0c7. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the regular perf test suite on this PR at dcff0c7. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at dcff0c7. You can monitor the build here. Update: The results are in! |
Hey Jake Bailey (@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 |
TypeScript Bot (typescript-bot)
commented
Nov 3, 2023
Jake Bailey (@jakebailey) Here they are:CompilerComparison Report - baseline..pr
System info unknown Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Nov 3, 2023
Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
TypeScript Bot (typescript-bot)
commented
Nov 3, 2023
Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing Everything looks good! |
TypeScript Bot (typescript-bot)
commented
Nov 3, 2023
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. |
TypeScript Bot (typescript-bot)
commented
Nov 3, 2023
Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. DetailsServer exited prematurely with code unknown and signal SIGABRTAffected reposcalcom/cal.comRaw error text:RepoResults7/calcom.cal.com.rawError.txt in the artifact folderLast few requests{"seq":808,"type":"request","command":"getOutliningSpans","arguments":{"file":"@PROJECT_ROOT@/apps/swagger/pages/_app.tsx"}}
{"seq":809,"type":"request","command":"updateOpen","arguments":{"changedFiles":[{"fileName":"@PROJECT_ROOT@/apps/swagger/pages/_app.tsx","textChanges":[{"newText":" //comment","start":{"line":1,"offset":42},"end":{"line":1,"offset":42}}]}],"closedFiles":[],"openFiles":[]}}
{"seq":810,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/apps/api/test/lib/middleware/withMiddleware.test.ts"],"openFiles":[]}}
{"seq":811,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/apps/web/app/layout.tsx","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps
|
TypeScript Bot (typescript-bot)
commented
Nov 3, 2023
Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing Everything looks good! |
Jake Bailey (jakebailey)
commented
Nov 4, 2023
There's a crash above, but not sure if it's due to this PR or not. |
…th-private-members-for-contextual-type
Mateusz Burzyński (Andarist)
commented
Dec 26, 2023
I really doubt that this crash is related to this PR - but who knows? Could you rerun the extended test suite here before I start investigating this? |
Jake Bailey (jakebailey)
commented
Dec 26, 2023
TypeScript Bot (@typescript-bot) test top200 |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite on this PR at 34d43f0. You can monitor the build here. Update: The results are in! |
TypeScript Bot (typescript-bot)
commented
Dec 26, 2023
Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing Everything looks good! |
Uh oh!
There was an error while loading. Please reload this page.
…th-private-members-for-contextual-type
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
I'm a little bit suspicious of the fix itself from the PoV of just adding another layer of workers and then filtering, as compared to doing this more deeply in getContextualType, but I can't quite put my finger on what is better.
Uh oh!
There was an error while loading. Please reload this page.
Mateusz Burzyński (Andarist)
commented
Jan 4, 2024
it has been a while since I was working on this. I vaguely remember similar concerns that I had. IIRC, I do this filtering here because I can't access the information about being within "literal expression object" context at a deeper level. At the end of the day, the same operation would have to be done deeper - we get a set of values and we need to reject some. So this level works as OK as any other (as long as it doesn't introduce problems, ofc). |
…th-private-members-for-contextual-type
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with TypeScript's type inference for object literals when classes with private members are part of union types. The fix ensures that when TypeScript provides contextual typing for object literals, classes containing private members are filtered out from consideration, preventing incorrect inference that would include inaccessible private properties.
Key changes:
- Enhanced type discrimination logic to exclude classes with private members from contextual typing
- Updated completion logic to detect both traditional private members and ES private fields (#property)
- Added comprehensive test coverage for various scenarios involving private members
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/compiler/checker.ts | Core fix - filters out classes with private members during type discrimination for contextual typing |
| src/services/completions.ts | Enhanced detection of non-public properties to include ES private fields alongside traditional private members |
| tests/cases/compiler/contextuallyTypedParametersPositionIncludesClassWithPrivateMember1.ts | Test case for ES private fields (#property) in contextual typing scenarios |
| tests/cases/compiler/contextuallyTypedParametersPositionIncludesClassWithPrivateMember2.ts | Test case for traditional private members in contextual typing scenarios |
| tests/cases/fourslash/completionsObjectLiteralPositionIncludesClassWithPrivateMember1.ts | Fourslash test for ES private fields in object literal completions |
| tests/cases/fourslash/completionsObjectLiteralPositionIncludesClassWithPrivateMember2.ts | Fourslash test for ES private fields in object literal completions (nested object variant) |
| tests/cases/fourslash/completionsObjectLiteralPositionIncludesClassWithPrivateMember3.ts | Fourslash test for traditional private members in object literal completions |
| tests/cases/fourslash/completionsObjectLiteralPositionIncludesClassWithPrivateMember4.ts | Fourslash test for traditional private members in object literal completions (nested object variant) |
| tests/baselines/reference/* | Generated baseline files showing expected compilation results and error messages |
Comments suppressed due to low confidence (1)
Mateusz Burzyński (Andarist)
commented
Aug 23, 2025
I think you were right. From what I can tell, this can just easily be put into |
TypeScript Bot (typescript-bot)
commented
Mar 24, 2026
With 6.0 out as the final release vehicle for this codebase, we're closing all PRs that don't fit the merge criteria for post-6.0 patches. If you think this was a mistake and this PR fits the post-6.0 patch criteria, please post to the 6.0 iteration issue with details (specifically, which PR and which patch criteria it satisfies). Next steps for PRs:
|
fixes#56177