Support interpreting non-literal computed properties in classes as implicit index signatures #59860
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 Uncommitted Bug
|
@typescript-bot pack this |
|
Hey @weswigham, 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 |
|
@typescript-bot test it |
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Seems like the angular codebase is gaining new errors? |
|
@weswigham Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
…t literal expressions
|
@typescript-bot test it Now I'm using exactly the same index signature creation logic we use for object literal expressions, with all of it's flaws (like making a lot of |
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hm. Still some new errors in angular, but everything else looks fine now. Guess I'll have to look at what those are. |
|
@weswigham Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
…gnature types - cant forget those
|
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. |
|
@typescript-bot test it |
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There we go, no more new errors in angular - now just to see if top400 comes back cleaner, too. (I think it should, the most recent fix is squarely aimed at those errors, since they're the same cause as the ones in angular - forgetting to include the explicit prop types in the implied index signature type) |
|
@weswigham Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
jakebailey
approved these changes
Sep 18, 2024
Comment on lines
+16313
to
+16315
| if (hasComputedStringProperty && !findIndexInfo(indexInfos, stringType)) indexInfos.push(getObjectLiteralIndexInfo(readonlyComputedStringProperty, 0, allPropertySymbols, stringType)); | ||
| if (hasComputedNumberProperty && !findIndexInfo(indexInfos, numberType)) indexInfos.push(getObjectLiteralIndexInfo(readonlyComputedNumberProperty, 0, allPropertySymbols, numberType)); | ||
| if (hasComputedSymbolProperty && !findIndexInfo(indexInfos, esSymbolType)) indexInfos.push(getObjectLiteralIndexInfo(readonlyComputedSymbolProperty, 0, allPropertySymbols, esSymbolType)); |
There was a problem hiding this comment.
Are all of the findIndexInfo calls likely to cost a bunch or is indexInfos just always short?
There was a problem hiding this comment.
In practice, it's always short (there's usually just string or number indexes, if any) - in theory, a user could explicitly define hundreds of pattern literal index infos and it'd be bad, however, and you'd prefer you did the up-front work to make a map keyed on key type (if you had multiple kinds of computed names). But that's not code anyone's realistically writing.
|
Hmm... I hit a bit of a snag here. I have a class like this: import util from 'node:util'
const inspect = util.inspect.custom
export class MyMap extends Map<MyObjectA, MyObjectB> {
[inspect](depth: number, options: util.InspectOptions): string {
// my fancy code for making this look pretty...
}
// ... and here goes all the extra stuff I want
}Now this compiles out as a export declare class MyMap extends Map<MyObjectA, MyObjectB> {
[x: symbol]: ((depth: number, options: util.InspectOptions) => string)
| (() => MapIterator<[string, string]>);
// ... all the other niceties
}... so far so good (maybe???) but when I try to import my map in some other project I get an error: Why? Because `` declares interface Map<K, V> {
readonly [Symbol.toStringTag]: string;
}Maybe the typescript compiler should merge all symbol declarations and write something like: export declare class MyMap extends Map<MyObjectA, MyObjectB> {
[x: symbol]: string
| ((depth: number, options: util.InspectOptions) => string)
| (() => MapIterator<[string, string]>);
// ... all the other niceties
}I don't know what'd be the right solution in these cases. For my code, I can simply remove that assignment import util from 'node:util'
export class MyMap extends Map<MyObjectA, MyObjectB> {
[util.inspect.custom](depth: number, options: util.InspectOptions): string {
// my fancy code for making this look pretty...
}
// ... and here goes all the extra stuff I want
}produces a more correct import util from 'node:util';
export declare class MyMap extends Map<MyObjectA, MyObjectB> {
[util.inspect.custom]: ((depth: number, options: util.InspectOptions) => string)
| (() => MapIterator<[string, string]>);
// ... all the other niceties
}But I noticed that anytime I import a |
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.
Instead of silently dropping them (if they're declared by methods) as we do today.
This still has some minor issues and open questions, but is far enough along to share and talk about. Today, when you have a class like
that's just fine - no errors anywhere. But if you add a
you get an error that
Ahas no symbol index signature. What's up with that? Well, we dropped the[a]member entirely, silently. There are historical reasons for this, but they don't really hold much weight nowadays beyond "for back compat". Under non-strict, this can actually be insidious, as when you use an expression, the(new A())[a]expression can silently beany, since under non-strict mode, non-existing element accesses resolve toanywithout error. With this PR,A[typeof a]instead resolves to() => number(the method's type) as we've created an implicit index signature for the computed name, just like we do for object literals.Technically, this involves late-binding the
__indexmember of the class like we do other computed literal names (so you could call these "late bound index signatures" as I do in the code), but that's not so important compared to the effect. This is pretty different from how these kinds of indexes are built for object literals, but that's because object literals are built somewhat on-demand from their constituent declarations, rather than having a big table of declared metadata built up-front like classes and interfaces do.The big questions I'm still considering are:
A computed property name in a class property declaration must have a simple literal type or a 'unique symbol' type.(1166)be removed, too? Methods don't emit an error, which is what motivated me here, but all this logic equally applies to properties, so the error doesn't make too much sense for them now.noImplicitAnyor another (strict?) flag? (preferably not - this being flagged somewhat defeats the point, imo)And, ofc, this needs more tests, covering other key types (unions, pattern literals) and modifiers (static, readonly) on the computed names (though I've informally handled them in the code, I think) before this can actually be merged (though these probably have some coverage already from documenting our current behavior - they could stand to be more comprehensive).
This is, IMO, a worthwhile step towards unifying all of our computed property name behaviors. In an ideal world, a computed property name in a class, object, in a source or declaration file, could all be interpreted the same way (and thus, it could always be safe to copy one from one place or file kind into the other). Unfortunately, today we have some behavioral carve-outs from the time before we even meaningfully supported literal types, or even had
--strict, that get in the way of that, like this.