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

Ensure static index signatures have an errorNode available #44129

Merged

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented May 17, 2021

So they can actually report the compatibility error they were trying to issue when the string and number index signatures mismatched.

Fixes #44082
Fixes #44134

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels May 17, 2021
@@ -36898,6 +36898,11 @@ namespace ts {
const someBaseTypeHasBothIndexers = forEach(getBaseTypes(<InterfaceType>type), base => getIndexTypeOfType(base, IndexKind.String) && getIndexTypeOfType(base, IndexKind.Number));
errorNode = someBaseTypeHasBothIndexers || !type.symbol.declarations ? undefined : type.symbol.declarations[0];
}
if (!errorNode) {
Copy link
Member

@DanielRosenwasser DanielRosenwasser May 17, 2021

Choose a reason for hiding this comment

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

Seems like this isn't the right fix - you'll still end up with the wrong error span, just as in the following example:

class Foo {
  [p: string]: any;
  static [p: string]: number;
}
error: Property 'prototype' of type 'Foo' is not assignable to string index type 'number'.

Copy link
Member

@DanielRosenwasser DanielRosenwasser May 17, 2021

Choose a reason for hiding this comment

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

I've opened up #44134 to track that.

@weswigham weswigham force-pushed the error-node-static-index-signatures branch from ebfdfd4 to a8327a0 Compare May 24, 2021
@weswigham
Copy link
Member Author

weswigham commented May 24, 2021

@DanielRosenwasser This now fixes both reporting issues and, per our discussion to mitigate the impact of that and preserve the usefulness of the feature, also disables checking if index signatures are consistent with prototype properties on class static sides.

@weswigham weswigham merged commit 2203228 into microsoft:master May 24, 2021
9 checks passed
@weswigham
Copy link
Member Author

weswigham commented May 24, 2021

@DanielRosenwasser you want a cherry pick for this, I assume?

@typescript-bot cherry-pick this into release-4.3

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 24, 2021

Heya @weswigham, I've started to run the task to cherry-pick this into release-4.3 on this PR at a8327a0. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 24, 2021

Hey @weswigham, I've opened #44240 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request May 24, 2021
Component commits:
7a9854c Ensure static index signatures have an errorNode available

626b431 Merge branch 'master' into error-node-static-index-signatures

a8327a0 Lookup static index signature declarations in the right symbol table, stop checking prototype props
DanielRosenwasser pushed a commit that referenced this pull request May 24, 2021
Component commits:
7a9854c Ensure static index signatures have an errorNode available

626b431 Merge branch 'master' into error-node-static-index-signatures

a8327a0 Lookup static index signature declarations in the right symbol table, stop checking prototype props

Co-authored-by: Wesley Wigham <t-weswig@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
3 participants