-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add NoInfer<T>
intrinsic represented as special substitution type
#56794
Conversation
@typescript-bot pack this |
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at b25bf3e. 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 |
} | ||
|
||
const intrinsicTypeKinds: ReadonlyMap<string, IntrinsicTypeKind> = new Map(Object.entries({ | ||
Uppercase: IntrinsicTypeKind.Uppercase, | ||
Lowercase: IntrinsicTypeKind.Lowercase, | ||
Capitalize: IntrinsicTypeKind.Capitalize, | ||
Uncapitalize: IntrinsicTypeKind.Uncapitalize, | ||
NoInfer: IntrinsicTypeKind.NoInfer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly related question: should ThisType
become an intrinsic type as well (it is kinda intrinsic by nature but it's not defined as such in the typedefs)? Or is it better to not touch it at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously this shouldn't be touched as part of this PR - I just take this opportunity to ask a question about it ;p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to leave ThisType
as is. An intrinsic
declaration doesn't really say anything about the internal representation of the type, and we already have a perfectly good solution for that with the current interface-based declaration.
@typescript-bot test top100 |
Heya @ahejlsberg, I've started to run the tsc-only perf test suite on this PR at b25bf3e. 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 b25bf3e. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at b25bf3e. 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 b25bf3e. You can monitor the build here. Update: The results are in! |
@ahejlsberg Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@ahejlsberg Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
Hey @ahejlsberg, the results of running the DT tests are ready. |
Huh, so it does... here I had thought TS never inferred cross-candidate unions, but apparently there are cases when it does? |
Unions can be formed under the same constrained primitive family. That's why my TS Congress talk had to disavow knowledge of primitive types. |
@ahejlsberg Here are the results of running the top-repos suite comparing Everything looks good! |
Tests and performance all look unaffected. |
All these cases can be addressed by using
It is not a trick. Do you have any stronger cases? I would not call those test cases buggy. With only that evidence it seems like perhaps this pull is a performance optimization, because you are explicitly shutting down inference, which could only shorten processing time, which is not a bad thing at all. |
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 |
src/compiler/checker.ts
Outdated
@@ -6710,7 +6712,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
return visitAndTransformType(type, type => conditionalTypeToTypeNode(type as ConditionalType)); | |||
} | |||
if (type.flags & TypeFlags.Substitution) { | |||
return typeToTypeNodeHelper((type as SubstitutionType).baseType, context); | |||
const typeNode = typeToTypeNodeHelper((type as SubstitutionType).baseType, context); | |||
return isNoInferType(type) ? factory.createTypeReferenceNode("NoInfer", [typeNode]) : typeNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens in declaration emit when something like this gets serialized?
// foo.ts
export const f: <T>(x: T, y: NoInfer<T>) => bool;
// bar.ts
import { f } from "./foo.js";
type NoInfer<T> = number;
export const g = f;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the latest commit this will now emit globalThis.NoInfer<T>
, similar to what we do for other global types in conflict situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test for that? Same with Uppercase etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't, at least not that I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test added.
My gut feeling is that the I have to explain what I mean by "belongs to the call". You might have a multiple parameter types that appears in multiple calls:
Trying to infer In order to infer Switching to member The ability to reach into type structure to infer type from specified members could be useful, but NoInfer types do not make a difference to what is currently allowed. (I tried). And if it did, the choice of which member to infer from is likely depend upon the function, not the parameter type passed. |
Cases from 2.8 release documentation + NoInfer
Try with predefined type
|
@typescript-bot pack this |
Heya @ahejlsberg, I've started to run the tarball bundle task on this PR at 2fc975a. You can monitor the build here. |
Hey @ahejlsberg, 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 |
I'm surprised not to see more examples where the type is infered from the returned value. Here is a (simplified) real world example of a code I faced where it was unpossible to catch errors without the // TS <5.4
function getValue<T = string>(): T {
return undefined as T;
}
function getNumberInline(): number {
return getValue(); // no error, T in inferred as number (weird behavior)
}
function getNumberUsingVariable(): number{
const value = getValue(); // error, T takes the default string value
return value;
}
// TS >=5.4
function getValueNoInfer<T = string>(): NoInfer<T> {
return undefined as T;
}
function getNumberInlineNoInfer(): number {
return getValueNoInfer(); // error, T takes the default string value
}
function getNumberUsingVariableNoInfer(): number{
const value = getValueNoInfer(); // error, T takes the default string value
return value;
} |
It would be nice if the following pattern could work: declare function create<T>(fn: (get: () => NoInfer<T>) => T): T
create(get => ({
id: 123,
getId() {
return get().id
}
})) Right now If I understand |
No, it was just a simplified example. Here you can read about real world usage. (Check the first collapsible section |
@dhmk083 currently, TS can't defer this sufficiently enough, it has to assign the contextual type for It's probably not impossible to improve this by deferring the instantiation of those parameter types that are only observable by the return types. I don't feel that it's particularly related to And note that ur |
This PR adds official support for a
NoInfer<T>
marker type that blocks inferences to the contained type. The PR supersedes #52968, but borrows as much as possible from that PR (lots of thanks for your work, @Andarist).The PR represents
NoInfer<T>
types as special substitution types (types with kindTypeFlags.Substitution
) having constraint typeunknown
, a pattern that otherwise never occurs. This representation ensures that we maximally leverage our existing infrastructure for erasing marker types when appropriate, and that we minimally disrupt the tooling ecosystem (because we're not introducing a new kind of type).Other than blocking inference,
NoInfer<T>
markers have no effect onT
. Indeed,T
andNoInfer<T>
are considered identical types in all other contexts.Some examples:
Above, inferences to the type of
b
are blocked by theNoInfer<T>
marker, thus causing errors to be reported when the argument types differ. Without theNoInfer<T>
markers, inference simply produces the union"foo" | "bar"
.Fixes #14829.