Adjust TypeFact calculation for intersections to omit negative typeof facts when an equivalent positive fact is present #39016
Conversation
… facts when an equivalent positive fact is present
weswigham
force-pushed
the
control-flow-function-typeof
branch
from
b95ac8a to
269f5ba
Compare
src/compiler/checker.ts
Outdated
| // Get the set of positive facts for the intersection by masking with the negative set shifted left, | ||
| // then shift those present positive facts into the negative fact value range, and unset any of those | ||
| // bits (by negating that mask and then intersecting it with the original value) | ||
| const positiveFacts = result & (TypeFacts.NegativeTypeofFacts >> 8); |
There was a problem hiding this comment.
It seems like the test coverage here is not adequate to ensure that this math continues to work for all cases in the future. Let's have a testcase that makes sure that e.g. someone reordering the EQ members without also pairwise reordering the NE members sees a baseline diff.
There was a problem hiding this comment.
I'm not quite sure how to make a baseline for that, but a unittest can be made if the TypeFacts gets export + @internal:
describe("getTypeFactsOfTypes", () => {
// See: https://github.com/microsoft/TypeScript/pull/39016
it("correctly can strip NegativeTypeofFacts when there are PositiveTypeofFacts of the same type via bitmask logic", () => {
const TypeFacts = (ts as any).TypeFacts;
// For a set of facts, which include both the positive and negative of each other
let ExampleFacts = TypeFacts.TypeofEQNumber | TypeFacts.TypeofNENumber | TypeFacts.TypeofNEBigInt;
// Using this line of code (with the 8 which effectively represents the number of EQ types)
const positiveFacts = ExampleFacts & (TypeFacts.NegativeTypeofFacts >> 8);
// Then this should drop the negative version of the same type
ExampleFacts &= ~(positiveFacts << 8);
assert.isTrue((ExampleFacts & TypeFacts.TypeofNENumber) === 0);
// And it keeps the rest
assert.isTrue((ExampleFacts & TypeFacts.TypeofEQNumber) !== 0);
assert.isTrue((ExampleFacts & TypeFacts.TypeofNEBigInt) !== 0);
// If this test has failed, you _probably_ need to adjust the 8 here, and in
// getTypeFactsOfTypes.
});
});```
… facts when an equivalent positive fact is present
typescript-bot
added
the
For Milestone Bug
|
There we go, added that test |
|
@RyanCavanaugh (and @sandersn ) 👍 👎 when you get a chance? |
| @@ -90,6 +91,7 @@ namespace ts { | |||
| // The following members encode facts about particular kinds of types for use in the getTypeFacts function. | |||
| // The presence of a particular fact means that the given test is true for some (and possibly all) values | |||
| // of that kind of type. | |||
| NegativeTypeofFacts = TypeofNEString | TypeofNENumber | TypeofNEBigInt | TypeofNEBoolean | TypeofNESymbol | TypeofNEObject | TypeofNEFunction | TypeofNEHostObject, | |||
There was a problem hiding this comment.
NegativeTypeofFacts is only ever used to left-shift 8 bits. It would be easier to read if it were just PositiveTypeofFacts, which would need shifting
| if (!isUnion) { | ||
| // Get the set of positive facts for the intersection by masking with the negative set shifted left, | ||
| // then shift those present positive facts into the negative fact value range, and unset any of those | ||
| // bits (by negating that mask and then intersecting it with the original value) |
There was a problem hiding this comment.
I don't like "intersecting" here, but "ANDing" sounds silly too. I think there's a technical term, but it might be too technical because I can't remember it.
Consider my vague dislike registered and continue as appropriate.
|
Unfortunately, we never finished reviewing this PR. It is pretty old now, so I'm going to close it to reduce the number of open PRs. |
Fixes #32928