fix(57635): duplicate property name error when trying to overwrite early-bound prop with late-bound prop #57717
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rly-bound prop with late-bound prop
jakebailey
reviewed
Mar 11, 2024
Comment on lines
15
to
-22
| [c0]: number; | ||
| ~~~~ | ||
| !!! error TS2718: Duplicate property '1'. | ||
| 1: number; | ||
| ~ | ||
| !!! error TS2733: Property '1' was also declared here. |
There was a problem hiding this comment.
I was going to say that this sure felt wrong, but your playground link does show that doing effectively this in other forms does not error. (Makes me wonder if we're missing errors; but that's more of a design question I don't have context for.)
There was a problem hiding this comment.
I don't think we should allow it, but it's definitely a separate question -- and disallowing it will probably break some amount of code.
|
@typescript-bot test top400 @typescript-bot perf test this faster |
|
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
|
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
sandersn
requested changes
Mar 20, 2024
src/compiler/checker.ts
Outdated
| @@ -12997,7 +12997,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| // conflicting flags. | |||
There was a problem hiding this comment.
please update this comment to read "Report an error if there's a symbol declaration with the same name and conflicting flags.", since there's no longer an error for any earlySymbol with the same name.
sandersn
approved these changes
Mar 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #57635
As written in the issue, I noticed there's a conflict in which the late-bound member already exists as an early-bound member.
My naive approach was to remove
earlySmybolfrom theifstatement and see which tests are failing, and I saw thatdynamicNamesErrors.tswas the only test to fail (https://github.com/microsoft/TypeScript/blob/main/tests/cases/compiler/dynamicNamesErrors.ts#L6-L12).I thought there's a reason for that, and that
Functionshould behave differently fromInterface, but they appear to behave similarly in that case.In the example below, you can see that when one prop is early-bound and the second is late-bound, there's an error pointing to duplication. However, when they are both early-bound or late-bound, there are no errors (TS Playground):
I think that regardless of whether we want to forbid or allow duplications in functions and interfaces, it should be consistent, and not inside
lateBindMember.