-
Notifications
You must be signed in to change notification settings - Fork 277
[Typekit] - Add builtin.is(type: Type): boolean
#7407
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds a new builtin.is(type: Type): boolean
method to detect built-in types (including those defined in the global TypeSpec
namespace and pure intrinsics) and supplements compiler logic with comprehensive tests.
- Implemented
builtin.is()
in the builtin kit with a helper to locate the proper namespace ancestor. - Added extensive Vitest scenarios covering models, properties, unions, enums, scalars, aliases, and intrinsic types.
- Introduced
getImmediateChildOfGlobal
to support namespace ancestry resolution.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
packages/compiler/src/typekit/kits/builtin.ts | Added is() method and getImmediateChildOfGlobal to identify built-in types based on namespace ancestry. |
packages/compiler/test/typekit/builtin.test.ts | Added a suite of tests for builtin.is() covering various type scenarios and edge cases. |
All changed packages have been documented.
Show changes
|
is(type: Type): boolean { | ||
// Model property does not have a namespace, | ||
// so we will use the Model where it is defined. | ||
if (type.kind === "ModelProperty" && type.model) { |
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.
not sure this is worth it?
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.
Updated the tests to show when this is helpful
namespace TypeSpec {
model InTypeSpec {
foo: string;
}
}
model Foo {
bar: TypeSpec.InTypeSpec.foo;
}
Without this $.builtin.is(Foo.bar.type) would return true
|
||
// Finally, compare by identity | ||
const typeSpecNs = globalNs.namespaces.get("TypeSpec"); | ||
return topLevel === typeSpecNs; |
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.
is that always working when using mutators?
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.
I seem to recall at some point that we had special-cased TypeSpec so as to avoid mutating built-in types. Is that not the case? If it's not, then I think we might have more bugs in other places, e.g. the intrinsic is checks use reference equality.
You can try these changes here
|
Introduce
builtin.is(type: Type): boolean
, which returns true for any type defined in the globalTypeSpec
namespace (i.e. built-in/standard library types).