Let properties having primitive types discriminate object unions #60718
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
jfet97
changed the title
|
@typescript-bot test it |
|
Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details. |
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The perf test indicates an extra error in the vscode codebase, which should be looked at. |
|
var objStrOrNum3: { prop: string } | { prop: number } = {
prop: strOrNumber
};
var objStrOrNum6: { prop: string; anotherP: string; } | { prop: number } = {
prop: strOrNumber,
anotherP: str
};
var objStrOrNum8: { prop: string; anotherP: string; } | { prop: number; anotherP1: number } = {
prop: strOrNumber,
anotherP: str,
anotherP1: num
};
Edit: Super seeded by the following changes. |
| isDiscriminant = (((transientSymbol.links.checkFlags & CheckFlags.Discriminant) === CheckFlags.Discriminant) | ||
| || !!(considerNonUniformPrimitivePropDiscriminant && (transientSymbol.links.checkFlags & CheckFlags.HasNonUniformType) && someType(propType, t => !!(t.flags & TypeFlags.Primitive)))) | ||
| && !isGenericType(propType); | ||
|
|
There was a problem hiding this comment.
This check can probably be done in createUnionOrIntersectionProperty, setting a new CheckFlag.HasPrimitive, like we do for HasNonUniformType.
There was a problem hiding this comment.
Even if it doesn't matter for the perf benchmarks we have, I think yes, to be consistent and keep similar code in a single place.
There was a problem hiding this comment.
@gabritto some feedback.
The basic change is quite easy, but there are some problems because createUnionOrIntersectionProperty terminates with result.links.type = isUnion ? getUnionType(propTypes) : getIntersectionType(propTypes), and is this resulting type the one on which I'm currently running someType(propType, t => !!(t.flags & TypeFlags.Primitive))). It's like it has already been reduced/cleaned up/whatever.
Just setting to 1 the CheckFlag.HasPrimitiveType beforehand like we do for other flags doesn't seem a good idea. I got 8 failing baselines and some changes are pretty weird. I'd suspect the right condition for setting the HasPrimitiveType flag will be a little convoluted. Or will be convoluted the one inside isDiscriminantProperty.
I pushed the current changes so you can have a look and maybe spot some stupid error I'm doing, or provide some insights for the above conditions.
There was a problem hiding this comment.
@gabritto plot twist, I disabled the new considerNonUniformPrimitivePropDiscriminant discriminating flag everywhere but in the only place I was sure we need it and things have improved a lot. I like it! The new discriminating power is being activated in more cases with your suggestion, that's why I changed more baselines, but the changes seemed to be correct to me. Please re-check them.
I still have to check why the binder.ts stopped compiling tho...
There was a problem hiding this comment.
Pushed a fix that you may dislike, so I'm open to suggestions.
The problem:
node.body // has type body?: ModuleBody | undefinedThe condition node.body is checked so its type is refined to just ModuleBody. That's fine!
Then you check !node.body.parent where:
node.body.parent // has type ModuleDeclaration | Node | ModuleBody | SourceFileIt cannot be non-defined! So the check !node.body.parent now narrows the type of node.body from ModuleBody to never.
Now, this is happening when building that structure so it's perfectly possible that the parent is not set yet. And I understand why you may not want to reflect this into types. Probably for all other intents and purposes that field will be always set. But TS just see the types, and with more refinement capabilities it now understands that the !node.body.parent case should be impossible.
There was a problem hiding this comment.
This PR is turning out to be more and more overzealous; I can already see a ton of projects in the top 400 breaking even worse.
|
@typescript-bot test it |
|
@gabritto Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
22 new errors just in vscode. This will be fun |
|
@gabritto Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
compiler-explorer/compiler-explorerif (currentLabel !== null && currentLabel.initialLine === undefined) {
currentLabel.initialLine = ln;
}
continuedev/continueIn addition to this, this, this and this. Please note they do not set A type PackageDocsResult = {
packageInfo: ParsedPackageInfo;
} & (
| { error: string; details?: never }
| { details: PackageDetailsSuccess; error?: never }
);In if (docsResult.error || !docsResult.details) {
// ...
return
}
docsResult // used after but has type 'never' hereIt seems the above When it comes to // Temporary fix for JetBrains autocomplete bug as described in https://github.com/continuedev/continue/pull/3022
if (llm.model === undefined && llm.completionOptions?.model !== undefined) {
llm.model = llm.completionOptions.model;
}But |
desktop/desktopAbout enum DragType {
Commit,
}
switch (currentDragElement.type) {
case DragType.Commit:
return (
<CommitDragElement
gitHubRepository={gitHubRepository}
commit={commit}
selectedCommits={selectedCommits}
emoji={emoji}
accounts={this.state.accounts}
/>
)
default:
return assertNever(
currentDragElement.type,
`Unknown drag element type: ${currentDragElement}`
)
}It seems they do not need that When it comes to TestNotificationType {
PullRequestReview,
PullRequestComment,
ChecksFailed,
}
switch (selectedFlow.type) {
case TestNotificationType.PullRequestReview: {
// ...
break
}
case TestNotificationType.PullRequestComment: {
// ...
break
}
case TestNotificationType.ChecksFailed: {
// ...
break
}
default:
assertNever(selectedFlow.type, `Unknown flow type: ${selectedFlow}`)
}Same thing!
ether/etherpad-liteNothing new! Same as here. |
google/blocklyif (typeof label === 'string') {
return [parsing.replaceMessageReferences(label), value];
}
hasImages = true;
const imageLabel =
label.alt !== null
? { ...label, alt: parsing.replaceMessageReferences(label.alt) }
: { ...label }; // <-- new error hereThe
ionic-team/ionic-frameworkif (data.year !== undefined) {
// ...
} else if (data.hour !== undefined) { // <-- new error here
rtn = twoDigit(data.hour) + ':' + twoDigit(data.minute); // <-- new errors here
}We have that P.S. interface DatetimeParts {
month: number;
day: number | null;
year: number;
dayOfWeek?: number | null;
hour?: number;
minute?: number;
ampm?: 'am' | 'pm';
}Maybe they forgot to set |
microsoft/vscodeIn addition to this. We have something interesting when it comes to This is interesting because TS loses the ability to solve a circular type dependency while inferring. There is this situation where types of identifiers are mutually dependent. We are in a big let groupMatching: GroupMatching | null = null;
let forceEvaluation = true;
let pos = 0
let lineLength = 0
while (forceEvaluation || pos < lineLength) {
let matches: string[] | null = null;
let action: FuzzyAction | FuzzyAction[] | null = null;
let rule: IRule | null = null;
if (groupMatching) {
const groupEntry = groupMatching.groups.shift()!; // <-- new error here
// they depend on groupMatching
action = groupEntry.action;
matches = groupMatching.matches;
rule = groupMatching.rule;
}
// things happens with action and matches
result = ...
// recursive dependency here
if (matches.length === fn(result)) {
// and here
groupMatching = {
rule: rule,
matches: matches,
groups: []
};
}
}I don't know the reason why the new discriminating capability doesn't let TS solve this circularity anymore. Of course we could easily fix this situation by manually solving the circularity: const groupEntry: {
action: monarchCommon.FuzzyAction;
matched: string;
} = groupMatching.groups.shift()!;As a bonus, the problem seems to be related to the "recursive" inferred type for groupMatching = {
rule: rule,
matches: matches,
groups: []
} as GroupMatching;
if (typeof folderStat.birthtimeMs === 'number') {
ctime = Math.floor(folderStat.birthtimeMs);
} else {
ctime = folderStat.birthtime.getTime(); // <-- new error here
}We have that
if (hasMultipleRoots && !options.noPrefix) {
const rootName = folder?.name ?? basenameOrAuthority(folder.uri); // <-- new error here
relativeLabel = relativeLabel ? `${rootName} • ${relativeLabel}` : rootName;
}Same story:
if (typeof meta?.typeId === 'string') {
delete meta.typeId; // <-- new error here
if (isEmptyObject(meta)) {
meta = undefined;
}
}The type of |
Redocly/redocWe have: const { type, oneOf, discriminatorProp, isCircular } = schema;
// discriminatorProp has type string
// ...
if(discriminatorProp !== undefined) {
// ...
}
// schema is never from now on, so new errors here:
schema.fields?.length
schema.description
schema.externalDocsHere |
|
@gabritto hey there! I went through all the errors, and apart from some occurrences of It is the first one from In the previous messages you'll find notes about this and the other new errors, with references to older messages when needed. If there is anything else I could do to help, let me know! |
|
In #60718 (comment), I don't think it's right to say that the user "doesn't need |
Yeah, it was just a stupid joke because sometimes it seems to me that this PR is all about assertNever(currentDragElement, `Unknown drag element type: ${currentDragElement}`)
// and
assertNever(selectedFlow, `Unknown flow type: ${selectedFlow}`)removing the |
|
@jakebailey may I ask for another "pack this"? It would be easier to play with it. The previous one is outdated given the implementation changes. Thanks ❤️ |
|
@typescript-bot pack this |
|
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 |
Main goal
The ultimate goal of this PR is to allow properties having primitive types discriminate object unions, even when there are no unit/literal types involved. Some examples of this are as follows:
If a property was not previously considered discriminant, it will now be treated as one when the following conditions are met:
Implementation details
isDiscriminantPropertywere a cachedbooleanin thelinksof transient symbols, so the first attempt consisted of just adding inside theisDiscriminantPropertyfunction(prop.links.checkFlags & CheckFlags.HasNonUniformType) && someType(propType, t => !!(t.flags & TypeFlags.Primitive))as a fallback.Unfortunately, I found a couple of problems with this.
The first one is related with
tests/cases/conformance/types/union/contextualTypeWithUnionTypeObjectLiteral.ts:The above assignment must fail (you can find the reason in that conformance test), but considering
propas a discriminant field changed the behaviour of TS, ultimately allowing the assignment. I ended up disabling the new discriminativeness ofpropin similar situations and the simpler solution was by using a flag, which I had to take into consideration when it comes to caching the final result. That test case is 10 years old, that behaviour is literally set in stone, and I don't have the guts to change it, even though the change would be more permissive so it shouldn't be a breaking change, hopefully.The second problem is related to #60702. Enabling the discriminativness on a particular
propmakes objects that declared that property asneverdisappear when narrowing. TLDR TS stopped compiling itself here because of howArrowFunctionis defined: itsnameproperty hasnevertype so it disappeared as a possible type ofnodeafter the assertion. Therefore I changed its type from: neverto?: neveras suggested by Ryan, both insrc/compiler/types, where the problematicArrowFunctionlies, and intests/baselines/reference/api/typescript.d.ts.I don’t think this last change is problematic; rather, I think a possible indirect breaking change might lie in the fact that if a property shared between objects is typed as
neverin one or more cases and is now considered discriminant due to this additional discriminating capability of TS, then the type containing it could end up being eliminated by TS in various circumstances. I’d like to point out that this already happens when a property is considered discriminant, as can be seen in the linked issue. Simply put, enhancing TS’s capabilities in this regard increases the cases where it can occur.Edit: The implementation has changed a bit. I ended up disabling the new capability everywhere except where it was really needed, so now that flag is
falseby default. Furthermore, to check if a property has at least one primitive type I added one specificCheckFlagsthatcreateUnionOrIntersectionPropertywill set when is appropriate. More info here.Final considerations
I wonder if
isDiscriminantWithNeverTypeshould take into account what’s been added by this PR. I’m quite unsure about it.