Use missingType in --noUncheckedIndexedAccess mode
#51653
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 61104d0. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 61104d0. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 61104d0. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 61104d0. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 61104d0. You can monitor the build here. |
|
@ahejlsberg Here are the results of running the user test suite comparing Everything looks good! |
|
@ahejlsberg Here they are:Comparison Report - main..51653
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
I love it when „in the next day or two” means „in the next half an hour” 😉 |
| else { | ||
| obj.test; // undefined | ||
| } | ||
| } |
There was a problem hiding this comment.
I've hoped that this solution would make it work (somehow through CFA) for at least the in variant:
function f5(obj: Record<string, string>, prop: string) {
if (obj[prop]) {
obj[prop]; // actual: string | undefined; expected: string
}
if (prop in obj) {
obj[prop]; // actual: string | undefined; expected: string
}
}Is this kind of analysis supported for any kind of patterns today?
There was a problem hiding this comment.
obj[prop] where prop isn’t a hardcoded literal can’t be narrowed in any way to my knowledge. There are many, many issues attesting to that if you go looking, and it’s considered to be by design.
There was a problem hiding this comment.
Ye, I've figured out as much - although I wonder how hard a limitation that is.
There was a problem hiding this comment.
Everything gets duped to #10530 which is kind of misleading since that issue was originally about bracketed access with a literal key (which has since been fixed), but the picture that develops after enough hunting is that it’s a performance sinkhole. See e.g. #25109 (comment)
There was a problem hiding this comment.
@Andarist It works when prop is declared as a const with a singleton literal type, e.g. const prop = "foo", but no support beyond that. We could conceivably support non-singleton-literal-type const variables, but it isn't trivial.
There was a problem hiding this comment.
There are many, many issues attesting to that if you go looking, and it’s considered to be by design.
I understand that this seems difficult to implement and we might never get this feature, but I'm having a hard time understanding how this could be "by design". What I mean is, this seems like fairly straight forward JS and as far as I understand TypeScript's design goals, we really should be able to support (type correctly) this kind of code. I really am sympathetic to the fact that it might not be feasible to do in the current implementation, I'm just challenging the idea that we want it to be this way.
There was a problem hiding this comment.
“By design” = ”intentional decision made during the design process”. Some such decisions are known compromises. I don’t use the phrase to imply it’s what the user wants, because nobody can even define that except for the individual users themselves, and the team can’t go around asking every single user before they’re allowed to label things “working as intended”. 😉
There was a problem hiding this comment.
Sure there always needs to be compromises made in the design of the language, but this isn't that. This seems to be a limitation of the current implementation (an implementation detail if you will), not some inherent limitation in the design of the language itself.
There was a problem hiding this comment.
I mean, the implementation is still part of the design of the TypeScript compiler in particular. Obviously a language wouldn't be intentionally designed that way in isolation, but that's a moot point since there's no TypeScript language specification. There used to be a formal spec a long time ago, but nowadays the implementation is the specification, and that's an entirely pragmatic design process. If the implementation is designed a certain way, in practice that means the language is too.
There was a problem hiding this comment.
...at least until someone gets fed up with the limitations and makes an alternative implementation. 😅
|
|
||
| function f4(obj: Record<string, string>) { | ||
| obj.test; // string | undefined | ||
| if (obj.hasOwnProperty("test")) { |
There was a problem hiding this comment.
wow, how does this one work if hasOwnProperty's signature is "just":
hasOwnProperty(v: PropertyKey): boolean;
what kind of sorcery is going on here? :D does this method have some special support in the compiler itself?
There was a problem hiding this comment.
does this method have some special support in the compiler itself?
It does indeed.
There was a problem hiding this comment.
I’m going to assume that feature was implemented early on since nowadays feature requests to special-case built-in methods (like Object.assign) get rejected if the special case can’t be represented in the method signature.
There was a problem hiding this comment.
For reference, I found the code related to hasOwnProperty here
|
@ahejlsberg Here are the results of running the top-repos suite comparing Everything looks good! |
This implements what I discuss here. I borrwed tests from that PR. Thanks @Andarist.
Fixes #43614.