'in' should not operate on primitive types #41928
Conversation
typescript-bot
added
the
For Milestone Bug
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 8c7ca65. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 8c7ca65. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 8c7ca65. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 8c7ca65. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 8c7ca65. You can monitor the build here. |
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@DanielRosenwasser Here they are:Comparison Report - master..41928
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
The user test suite indicates 4 new breaks in Fluent UI. |
if (typeof val === 'object' && '__isMaybe' in val) {This error is actually correct - |
|
Well...ish. The type is defined as |
|
But in this instance, you're probably right, this type should probably account for a |
|
@RyanCavanaugh @DanielRosenwasser
I agree, the error message needs some tweaking. If we wanted to exclude
Just out of curiosity and because the contributing file didn't mention these tests, how do they relate to the main test suite? 🙂 |
There was a problem hiding this comment.
This error is actually correct -
valcould benull- but boy are we going to get bug reports about this. Maybe we need to special-case the error message.
Well, the real problem with this is that since type parameters don’t narrow, you can’t make this error go away by adding the correct fix:
- if (typeof val === 'object' && '__isMaybe' in val) {
+ if (val && typeof val === 'object' && '__isMaybe' in val) {which I think is @DanielRosenwasser’s point about implementing a more conservative fix with respect to unconstrained type parameters. So if I understand correctly, @jonhue your invalidHasKey example test actually should not error. We’re only trying to catch cases that are definitely errors, like
function f<T extends string | number>(val: T) {
'__isMaybe' in val;
}
src/compiler/checker.ts
Outdated
| if (!allTypesAssignableToKind(rightType, TypeFlags.NonPrimitive | TypeFlags.InstantiableNonPrimitive) || | ||
| rightType.immediateBaseConstraint && allTypesAssignableToKind(rightType.immediateBaseConstraint, TypeFlags.Primitive)) { |
There was a problem hiding this comment.
Thank you all for the comments and hints!
I wasn't able to extract the information we need directly from rightTypes flags. Both alsoValidHasKey and invalidHasKey generate the same type flag for the right type. I guess this is to be expected though as we're looking at constraints on the type to identify whether this is a valid/invalid use of the in operator.
I'm not familiar enough with the codebase though to be able to tell whether we can always expect rightType.immediateBaseConstraint to be present in the cases we care about.
There was a problem hiding this comment.
I’m not 100% on this, but I believe all you need is
| if (!allTypesAssignableToKind(rightType, TypeFlags.NonPrimitive | TypeFlags.InstantiableNonPrimitive) || | |
| rightType.immediateBaseConstraint && allTypesAssignableToKind(rightType.immediateBaseConstraint, TypeFlags.Primitive)) { | |
| if (!allTypesAssignableToKind(getApparentType(rightType), TypeFlags.NonPrimitive)) { |
getApparentType will get the (resolved) base constraint if the type is instantiable (the immediate base constraint might also be instantiable), which means the result should never be instantiable, so you no longer have to check for InstantiableNonPrimitive. But make sure I’m not making stuff up by running the tests, and someone else on the team should double check my understanding here too.
There was a problem hiding this comment.
@andrewbranch I tried this out both comparing against NonPrimitive and comparing against NonPrimitive | InstantiableNonPrimitive, but both approaches lead to many scenarios where don't throw an error even though we should.
For example the following is accepted:
var b1: number;
var rb1 = 'x' in b1;
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at d2cd9b1. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at d2cd9b1. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at d2cd9b1. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at d2cd9b1. You can monitor the build here. |
|
@DanielRosenwasser Here they are:Comparison Report - master..41928
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@andrewbranch I think we should probably alter the error message as well. Maybe something like |
… instantiable non primitive
|
Thank you for these test cases @andrewbranch! I'm not entirely sure about the behavior Apart from the |
|
I just used the string literal function f<T extends object | string>(p: T) {
"" in p; // Not safe
if (typeof p === "object") {
"" in p; // Safe
} else {
"" in p; // Always an error
}
}We would prefer lines 2 and 6 to be an error, and 4 to be ok. But because we cannot refine the type of |
|
@andrewbranch sorry for the late follow-up! Thank you for the additional example. I made the change so that only for unions and intersections we require all types to be non primitive (i.e. the type must be non primitive). For other types we only require that it may be non primitive. |
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at afcf859. You can monitor the build here. |
|
Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@andrewbranch just out of curiosity, why is narrowing impossible on type parameters? Like in the example you gave above Is there a theoretical problem I'm not seeing that would make this unsound or is this a practical implementation issue or maybe even a conscious decision in favor of better performance? If it is a practical implementation issue, how hard do you think it would be to solve? In a quick search through the issues I wasn't able to find one that already tackles this in general, but I'm sure there must be one. I think #41333 is related. Thank you 🤗 |
|
Actually #4742 might be the first and broadest |
| // If constraint is a union/intersection, ensure no types are primitive. | ||
| isTypeAssignableToKind(rightType, TypeFlags.UnionOrIntersection) && !allTypesAssignableToKind(rightTypeConstraint, TypeFlags.NonPrimitive | TypeFlags.InstantiableNonPrimitive) || | ||
| // Otherwise, ensure that at least one type is not a primitive. | ||
| !maybeTypeOfKind(rightTypeConstraint, TypeFlags.NonPrimitive | TypeFlags.InstantiableNonPrimitive | TypeFlags.Object) |
There was a problem hiding this comment.
I think maybeTypeOfKind is correct here, but everything else uses assignability to NonPrimitive, and this isn't any kind of base case. I'd feel happier if the whole thing used assignability.
There was a problem hiding this comment.
Pasting from discussion in chat:
...applying your suggestion breaks a test case I designed—it adds new errors that are strictly more correct but more breaky, and a bigger divergence from existing behavior.
What maybeTypeOfKind allows for is this:
function union4<T extends object | "hello">(thing: T) { "key" in thing; // Ok (because narrowing is impossible) }I asserted that this should remain errorless, because we can't track the narrowing of thing if you type guarded that it was an object or not a string. Swapping
maybeTypeOfKindforisTypeAssignableToKindmakes it an error.
|
Oops, I can’t push to your branch, so I had to make a new one at #42288 |
…42288) * 'in' should not operate on primitive types * accept baselines of failing tests * review * update error message * check if constraint of right type is assignable to a non primitive or instantiable non primitive * do not throw errors where narrowing is impossible * accept baselines * fix test case failures * Add more accurate comment discussion and document failing edge case in test * Update baselines Co-authored-by: Jonas Hübotter <[email protected]>
…branch) (microsoft#42288) * 'in' should not operate on primitive types * accept baselines of failing tests * review * update error message * check if constraint of right type is assignable to a non primitive or instantiable non primitive * do not throw errors where narrowing is impossible * accept baselines * fix test case failures * Add more accurate comment discussion and document failing edge case in test * Update baselines Co-authored-by: Jonas Hübotter <[email protected]>
Fixes #41317
To address the issue I had to disallow values of types that are assignable to
TypeFlags.TypeParameterfrom being used as the right argument to theinoperator. Along the way, I also stopped allowing values whose type is assignable toTypeFlags.ConditionalorTypeFlags.Substitution.With this change all right arguments to
inwhose type is assignable to a primitive type, immediately lead to an error.This change led to three test case failures:
inOperatorWithValidOperandskeyofAndIndexedAccessconditionalTypeDoesntSpinForeverI presume that in case we want to pursue this change we will want to alter these test cases to explicitly annotate right arguments to
inas objects. For the moment, I accepted the new baselines of these failing tests.I'm also wondering if I should include the new test case I added (
inDoesNotOperateOnPrimitiveTypes) ininOperatorWithValidOperands.Also, we would probably have to change the error message.
Please let me know if any of this looks wrong to you!