Fix type test symbol handling #21070
Conversation
github-actions
bot
added
area: build
build-tools/packages/build-tools/src/typeValidator/compatibility.ts
Outdated
Show resolved
Hide resolved
| type WellKnownSymbols = OnlySymbols<ValueOf<typeof Symbol>>; | ||
| type OnlySymbols<T> = T extends symbol ? T : never; | ||
| // Omit (replace with never) a key if it is a symbol thats not a well known symbol from global Symbol. | ||
| type FilterKey<P> = symbol extends P |
There was a problem hiding this comment.
symbol extends P? What do symbols extend from? Am I understanding right that this means something like "if the key's type is so 'fundamental' that even symbols extend from it, then the key should be considered for type testing"? (The rest of the ternary(ies) I do understand).
There was a problem hiding this comment.
thats how you detect the type symbol. If P is a symbol, and symbol is P, then P == symbol (well not always due to union handling, but close). We want to allow the type symbol but not custom unique symbols, so we need to separate them and this is how to do that.
There was a problem hiding this comment.
So the only case where symbol extends P is true is when P is symbol itself (and not any custom unique symbol), correct? In that case we want to allow the key, so use P. Else, use P if it's not a symbol at all or it is a well-known symbol, and use never if it's a custom unique symbol to disallow them.
There was a problem hiding this comment.
I have updated the code and docs a bit to make this more clear.
| : T extends boolean | bigint | symbol | ||
| ? FilterKey<T> | ||
| : { | ||
| [P in keyof T as FilterKey<P>]: TypeOnly<T[P]>; |
There was a problem hiding this comment.
I'm having a hard time wrapping my head around as FilterKey<P> here... Does it make it so keys of T which are not-well-known symbols get replaced with never here, and that somehow results in the key "disappearing" from the view of T produced by TypeOnly<T>?
Also, the comment in line 21 wants to be updated to match the new code.
There was a problem hiding this comment.
I'm having a hard time wrapping my head around
as FilterKey<P>here... Does it make it so keys of T which are not-well-known symbols get replaced withneverhere, and that somehow results in the key "disappearing" from the view of T produced byTypeOnly<T>?
Yes: that is how you remove members in mapped types.
There was a problem hiding this comment.
Looks good to me.
Other improvements I am thinking about:
- Have an import for TypeOnly instead of injecting into every file.
import type { TypeOnly } from "@fluidframework/build-tools";which would be good to keep added complexity like this from spreading.
- Detect simple self-recursion and filter those out or replace with "type-only-recurses-here". We can rely on all of the other checks up to that point to validate. No need to check the type again and thus support recursive types.
| [P in keyof T]: TypeOnly<T[P]>; | ||
| }; | ||
| ? string | ||
| : T extends boolean | bigint | symbol |
There was a problem hiding this comment.
Would be nice to separate symbol from boolean | bigint. We don't need those going through the filter (I don't want to really think about what it means for those types even though I can without much trouble.)
| : T extends string | ||
| ? string |
There was a problem hiding this comment.
While we are in here, do you know why string is special cased? There is a comment for number, but not string. Relaxing a literal to a string seems like it is missing good check.
There was a problem hiding this comment.
Maybe its not needed. Its hard to tell with the tests in a bad state for multiple other reasons, but I think we can keep string, maybe. I'll try and.
## Description #21070 made some improvements to type tests which were needed to regenerate the type tests for client. This change integrates that, as well as regenerates the type tests with the new generator.
## Description Improve type test symbols handling. Well known symbols are preserved, as is `symbol`, but unique symbols are replaced (with never), both when used as keys and as value types preventing valse positives (detected API breaks that are op-ops). This was manually tested with the RC4 client packages, a determined to fix the issue with the new fluid handle symbol.
## Description microsoft#21070 made some improvements to type tests which were needed to regenerate the type tests for client. This change integrates that, as well as regenerates the type tests with the new generator.
Description
Improve type test symbols handling. Well known symbols are preserved, as is
symbol, but unique symbols are replaced (with never), both when used as keys and as value types preventing valse positives (detected API breaks that are op-ops).This was manually tested with the RC4 client packages, a determined to fix the issue with the new fluid handle symbol.
Reviewer Guidance
The review process is outlined on this wiki page.