Fast path for negative case when relating to unions of primtives #53192
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 922d47e. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 922d47e. 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 922d47e. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 922d47e. 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 922d47e. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here are the results of running the user test suite comparing Everything looks good! |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 922d47e. You can monitor the build here. |
|
Hey @jakebailey, 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 |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Branch only errors:Package: dom-mediacapture-transform Package: dom-screen-wake-lock Package: web-animations-js Package: webappsec-credential-management Package: dom-webcodecs Package: w3c-css-typed-object-model-level-1 Package: use-color-scheme Package: dom-mediacapture-record Package: ramda |
|
RE: DT above, the results are still compared against nightly, so some of those are preexisting breaks caused elsewhere today and can be ignored. I think @gabritto is looking switching that to compare against main. (In particular, DOM was updated which is most of the breaks, plus a dts emit change I need to go update in ramda.) |
|
@ahejlsberg Here are the results of running the top-repos suite comparing Everything looks good! |
|
@ahejlsberg Here they are:Comparison Report - main..53192
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg
requested review from
DanielRosenwasser,
RyanCavanaugh and
weswigham
|
If they "fail", then they'll take long enough to time out the runner, or at least be suspiciously slow. Maybe that's enough, but I don't feel strongly. |
weswigham
approved these changes
Mar 15, 2023
There was a problem hiding this comment.
IMO, we should add the test cases from the other issues, since, while we don't collect test perf metrics right now, we've talked about starting to collect them, once we have some more perf CI machine bandwidth, since the metrics are actually collected already. You can already require("./.parallelperf.json") to get an object with the timing data from your last run, so you can already anecdotally use the data today.
| @@ -20837,6 +20833,21 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (containsType(targetTypes, source)) { | |||
| return Ternary.True; | |||
| } | |||
| if (getObjectFlags(target) & ObjectFlags.PrimitiveUnion && !(source.flags & TypeFlags.EnumLiteral) && ( | |||
| source.flags & (TypeFlags.StringLiteral | TypeFlags.BooleanLiteral | TypeFlags.BigIntLiteral) || | |||
There was a problem hiding this comment.
This can be expanded to cover unique symbol literals, too.
There was a problem hiding this comment.
I considered that, but we'd then have to exclude them from the alternateForm check below because unique symbols don't have fresh forms. The extra complexity just doesn't seem worth it, I honestly can't imagine union types with multiple thousand unique symbols types anyways.
|
I'm going to leave the tests out. Even with the optimization in this PR they still take a while to complete, and I don't want to add that overhead to every test run in perpituity when they don't actually check anything new. Once we have better performance infrastructure we should consider adding them there. |
This was referenced Mar 18, 2023
|
The regression reported here can be traced back to this PR. The minimal~ repro case for it is this: enum AutomationMode {
NONE = "",
TIME = "time",
SYSTEM = "system",
LOCATION = "location",
}
interface ThemePreset {
id: string;
}
interface Automation {
mode: AutomationMode;
}
interface UserSettings {
presets: ThemePreset[];
automation: Automation;
}
interface ExtensionData {
settings: UserSettings;
}
export function getMockData(): ExtensionData {
return {
settings: {
presets: [],
automation: {
mode: "",
},
} as UserSettings,
}
} |
SlurpTheo
reviewed
May 9, 2023
| const containedUndefinedType = some(typeSet, containsMissingType) ? missingType : undefinedType; | ||
| removeFromEach(typeSet, TypeFlags.Undefined); | ||
| result = getUnionType([getIntersectionType(typeSet), containedUndefinedType], UnionReduction.Literal, aliasSymbol, aliasTypeArguments); | ||
| } | ||
| else if (eachIsUnionContaining(typeSet, TypeFlags.Null)) { | ||
| else if (every(typeSet, t => !!(t.flags & TypeFlags.Union && ((t as UnionType).types[0].flags & TypeFlags.Null || (t as UnionType).types[1].flags & TypeFlags.Null)))) { |
There was a problem hiding this comment.
(lines:-5+0)
Why does this only need to check?
types[0]forTypeFlags.Undefinedtypes[0] | types[1]forTypeFlags.Null
Is there some rule/convention that null is first in UnionType#types if it's part of the union, unless undefined is in the union (which is really always first if it is part of the union)?
There was a problem hiding this comment.
Yes, union types are sorted like this internally - so if a union contains undefined it has to be the first element etc.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements a fast path for the negative case of relating a literal type to a union of primtive types. This reduces the check time of the example in #53191 from ~45s to ~0.4s.
Fixes #53191.