Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed issue with method called new being emitted as construct signature #55109

Closed
wants to merge 3 commits into from
Closed

Fixed issue with method called new being emitted as construct signature #55109

wants to merge 3 commits into from

Conversation

Andarist
Copy link
Contributor

fixes #55075

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jul 22, 2023
jakebailey
jakebailey reviewed Jul 24, 2023
View reviewed changes
tests/baselines/reference/objectTypesIdentityWithConstructSignatures2.types
@@ -32,8 +32,8 @@ var a: { new(x: Date): string }
>x : Date

var b = { new(x: RegExp) { return ''; } }; // not a construct signature, function called new
>b : { new(x: RegExp): string; }
>{ new(x: RegExp) { return ''; } } : { new(x: RegExp): string; }
>b : { "new"(x: RegExp): string; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right... aren't all of these cases changed explicitly construct signatures that shouldn't be quoted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one definitely isnt wrong. Notice how this is inferred~ from an object literal and that cant even have a construct signature - it’s just a method called new

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agh, yeah. I think I skimmed this PR on my phone on the weekend and had it in my head that there was something going on.

@jakebailey
Copy link
Member

@typescript-bot test this
@typescript-bot test top100
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 24, 2023 •
edited
Loading

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 3825f0e. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 24, 2023 •
edited
Loading

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 3825f0e. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 24, 2023 •
edited
Loading

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 3825f0e. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 24, 2023 •
edited
Loading

Heya @jakebailey, I've started to run the extended test suite on this PR at 3825f0e. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/55109/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Unknown failure"
  • 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

  • [NEW] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55109/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • [MISSING] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/55109/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@jakebailey
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 10, 2023 •
edited
Loading

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 3825f0e. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 10, 2023 •
edited
Loading

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/156440/artifacts?artifactName=tgz&fileId=DDAF959970F2E00EEDF1EAE0F17206EE6A09387D4FAC998D5A4AB6332C0B965F02&fileName=/typescript-5.3.0-insiders.20230810.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.3.0-pr-55109-10".;

jakebailey
jakebailey previously approved these changes Aug 10, 2023
View reviewed changes
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though maybe @rbuckton may have some other trick that would be preferred over this particular fix.

@jakebailey
Copy link
Member

Actually, maybe this is a silly question, but should this be a declaration emit fix? I assume not, given the types baselines and tooltips are also wrong?

@Andarist
Copy link
Contributor Author

@jakebailey could you rephrase? from what I can tell both types baselines and tooltips are changed (and fixed) by this change

@jakebailey
Copy link
Member

My question was "should this fix be in declaration emit", but I half convinced myself the answer was "no" because of tooltips and baselines.

Of course, the fix could be applied to both declarations.ts and typeToTypeNode or whatever that thing is called.

@sandersn sandersn requested a review from rbuckton August 18, 2023 21:15
@jakebailey jakebailey self-requested a review September 15, 2023 03:19
@jakebailey
Copy link
Member

On second thought, this doesn't feel right; I think I found a better fix via getPropertyNameNodeForSymbol / createPropertyNameNodeForIdentifierOrLiteral.

Would you rather I send a new PR for this or push to your branch? (I am keeping the credit for the tests, of course; I just cloned this PR and then swapped the fix.)

@jakebailey
Copy link
Member

I don't want to stomp on your branch, I'll send a different PR.

@jakebailey jakebailey mentioned this pull request Sep 15, 2023
Merged
@jakebailey
Copy link
Member

See: #55750

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbuckton rbuckton Awaiting requested review from rbuckton

@jakebailey jakebailey Awaiting requested review from jakebailey

Assignees

@rbuckton rbuckton

Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Declaration for object with method named "new" incorrectly emitted with construct signature
4 participants
@Andarist @jakebailey @typescript-bot @rbuckton