-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Improve error message for index signature on generic type when writing #55906
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
Conversation
!!! error TS2860: Type 'string' cannot be used to index type 'T', because type 'T' is generic and an index signature of a constraint of a generic type can only be used for reading. | ||
~~~~~~~~~ | ||
!!! error TS2860: Type 'symbol' cannot be used to index type 'T', because type 'T' is generic and an index signature of a constraint of a generic type can only be used for reading. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems bad that there are two errors reported.
I think you can drop the words "of a constraint" and simplify to "index signature of a generic type".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought briefly about making only one error be reported, but it might be a bit wonky given the structure of the existing code...
We individually check each element of the union type used for indexing (e.g. in T[string | symbol]
we check both string
and symbol
), and while for this error it makes sense to treat string | symbol
as a single thing and report only one error, it's not always the case and I don't know how we could tell which approach to take beforehand, e.g. in this example: https://www.typescriptlang.org/play?ts=5.3.0-dev.20230928#code/GYVwdgxgLglg9mABMOcBMAeAKogpgDylzABMBnRMEAWwCNcAnAbQF1EAyRAb0SbIC5EZKAxhgA5i0FU6jRAB8hIseMQBfAHwAKKAEMG43FEFYANIgAOg4aIkLKNegwCU3AFCJEeg0aYW2ALyIAIwA3G5qQA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. There is already some logic where we try to handle a union of true
and false
as boolean
in getIndexedAccessTypeOrUndefined
to avoid 2 errors for what it's worth, so I think there's an alternative path where you might be able to collect the types that fail and then error on those instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to suggest changing the error message to "Type '{0}' is generic and can only be indexed for reading"
. I would just omit the index type, it adds no value. As a bonus, this means the error message is the same for string
and symbol
, so it should get reduced to a single occurrence in error reporting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that message, but what about the example I linked to above, where number
can be used to index into a generic type for writing, but string
/symbol
cannot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too concerned by that example. I mean, the example is trying to index with something that isn't permitted in a write position, so the error is correct for the given situation. It is true that there might not be an error for a subset of the index types, but that's not what the example is doing.
@@ -3699,6 +3699,10 @@ | |||
"category": "Error", | |||
"code": 2861 | |||
}, | |||
"Type '{0}' is generic and can only be indexed for reading.": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we hate the existing message about "T could be instantiated with a different subtype", buuuuuuuuuuuut..what about:
Type {property access} cannot be used to index '{type variable}' because '{type variable}' could be instantiated with an index signature that is a subtype of '{index signature}'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if/how adding that explanation helps people... Also, mentioning the index type makes it so you get two almost identical error messages if the index type is a union (e.g. string | symbol
), which is why Anders suggested above to omit the index type entirely.
Why do we hate the existing "T could be instantiated with a different subtype"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Ryan, we added the elaboration "T could be instantiated with a different subtype" because people didn't understand exactly why they were getting a (general) assignability error, and it helped people at least in the sense they had a more specific error message they could look up to try and understand the rule. I don't feel it's the same here, the message is already specific about T
being generic. Do you think the extra information here would help people who don't understand the issue already?
Fixes #47357.
Current error message is not intended to be the final one, and I'm looking for feedback on it.
What I'm hoping to get from a new error message is to make explicit what rule is causing the error message. The important parts are that we're type checking an access expression that is being written to, that the type of the expression being accessed is a generic one, and that the access results in us accessing an index signature present in the constraint of the generic type.
What I'm not trying to do: justify the rule (see #47357 (comment) for an explanation of what that could entail), or suggest a fix, as I don't think we have enough information to decide what an appropriate fix is.