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.

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

Add static index signature #37797

Merged
merged 28 commits into from
Mar 26, 2021
Merged

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Apr 5, 2020

Fixes #6480

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 5, 2020

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 5, 2020

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

@typescript-bot
Copy link
Collaborator

Hey @Kingwl, 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/70462/artifacts?artifactName=tgz&fileId=3AF6CC9DE1A8ED4CFFF5BB83EE19491B3EC20CE91E716546D47CA7344C97A65702&fileName=/typescript-3.9.0-insiders.20200405.tgz"
    }
}

and then running npm install.

@Kingwl Kingwl changed the title Add static index Add static index signature Apr 6, 2020
@orta
Copy link
Contributor

orta commented Apr 8, 2020

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 8, 2020

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 8, 2020

Hey @orta, 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/70679/artifacts?artifactName=tgz&fileId=A68270BE90993FCC02761F5C7DFC4E733CB522BA025C69FCE66DDE502EC10C9B02&fileName=/typescript-3.9.0-insiders.20200408.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Apr 15, 2020
@sandersn
Copy link
Member

@weswigham the reviewer list is broken on this PR, but you should look at this too

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 26, 2020

⬆️

tests/baselines/reference/staticAsIdentifier.errors.txt Outdated Show resolved Hide resolved
@@ -9664,6 +9677,13 @@ namespace ts {
}
}

function getIndexInfoOfIndexSymbol(indexSymbol: Symbol, indexKind: IndexKind) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is used only once in the above changed code, maybe make it a local helper instead?

Copy link
Member

Choose a reason for hiding this comment

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

I kinda prefer it this way to avoid several closure allocations, but it's likely a premature optimization.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I'd be surprised if at least the v8 compiler wouldn't optimize it away...)

@@ -11094,24 +11114,29 @@ namespace ts {
}

function getIndexSymbol(symbol: Symbol): Symbol | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're splitting both getIndexSymbol and getIndexDeclarationOfSymbol to two functions each, because your code calls them with different inputs, and it's the only place that needs the two new entry points. One such refactor seems questionable, but two looks like too much. (But OTOH I can't figure out if there's a way to avoid it.)

@elibarzilay
Copy link
Contributor

Ping @DanielRosenwasser, @weswigham.

@Kingwl
Copy link
Contributor Author

Kingwl commented May 19, 2020

Hi, Folks, I don't know what I can do. Should I work with the parser issue inside this PR?

@DanielRosenwasser
Copy link
Member

I would say no - though, if you want to tackle the problem, coordinate with @rbuckton on #38283.

Otherwise, I'm not familiar enough with the internals. @elibarzilay and @weswigham, can you give this another review?

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

The implementation looks good, but I'd love to see some more tests (you should consider placing them in a subfolder of the conformance directory instead of compiler, since it's a "new" language feature). Specifically, I'm thinking of tests showing:

  • Class static inheritance both inheriting and overriding index signatures (and issuing errors if you override with an incompatible type)
  • Assignability of class static sides to one another when index signatures are in place (readonly and not readonly)
  • Assignability between class static sides and equivalent interfaces describing them
  • Assignability between class static sides with generic static index signatures (this likely requires defining classes within a closure with generic parameters) to have some sanity checks on the behavior of variance digests with these
  • Some usages with indexed access types, eg (typeof Cls)[string]
  • Some usages with keyof, eg keyof typeof Cls
  • Some usages with homomorphic mapped types, eg Extract<typeof Cls, string>

More would also be welcome, ofc, if you can think of any other possible edge cases.

@sandersn
Copy link
Member

@elibarzilay @weswigham I'd like to get this in early in the 4.3 cycle. Does this (1) have enough tests now (2) error correctly on property types?

@Kingwl
Copy link
Contributor Author

Kingwl commented Mar 4, 2021

Friendly ping 👀

@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Mar 4, 2021
@Kingwl
Copy link
Contributor Author

Kingwl commented Mar 25, 2021

👀PiNg AgAiN

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Assuming my question is answered, I think this is probably good now, with @sandersn 's questions both answering in the affirmative. @elibarzilay since you're the PR assignee, you can probably merge this for this cycle whenever you're ready.

src/compiler/checker.ts Show resolved Hide resolved
@Kingwl
Copy link
Contributor Author

Kingwl commented Mar 25, 2021

just for verify

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 25, 2021

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

@typescript-bot
Copy link
Collaborator

Hey @Kingwl, 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/99257/artifacts?artifactName=tgz&fileId=E1B419FD818D7F5E2314236B7CDD646F933C3C20811250634D11FB9268A194FA02&fileName=/typescript-4.3.0-insiders.20210325.tgz"
    }
}

and then running npm install.

@orta
Copy link
Contributor

orta commented Mar 25, 2021

Hrm, I'm not certain why the playground didn't build (as this seems to be completely up to date with master), I'll look into add some more logs in the build process.

@Kingwl
Copy link
Contributor Author

Kingwl commented Mar 26, 2021

Try again.
@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 26, 2021

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 26, 2021

Hey @Kingwl, 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/99342/artifacts?artifactName=tgz&fileId=9F9DC5C2BFBC2F804A9BB87AAC7535FF4F449685817397BB01F4B3AB0D61A51C02&fileName=/typescript-4.3.0-insiders.20210326.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@4.3.0-pr-37797-42".;

@DanielRosenwasser DanielRosenwasser merged commit 41dc625 into microsoft:master Mar 26, 2021
@Kingwl Kingwl deleted the static_index branch June 30, 2021 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug Rescheduled This issue was previously scheduled to an earlier milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Allow defining static index signature
8 participants