Skip to content

Prevent readonly symbols widening #54778

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

Closed

Conversation

Andarist
Copy link
Contributor

closes #53276 , cc @weswigham

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jun 26, 2023
@@ -37570,7 +37570,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function widenTypeInferredFromInitializer(declaration: HasExpressionInitializer, type: Type) {
const widened = getCombinedNodeFlagsCached(declaration) & NodeFlags.Constant || isDeclarationReadonly(declaration) ? type : getWidenedLiteralType(type);
const widened = getCombinedNodeFlagsCached(declaration) & NodeFlags.Constant || isDeclarationReadonly(declaration) ? type : getWidenedUniqueESSymbolType(getWidenedLiteralType(type));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that perhaps as const should preserve the uniqueness of symbols created in object literals but that's not strictly related to this issue so I think it should be fine to tackle this separately if that's something that you think should be done.

// always widen a 'unique symbol' type if the type was created for a different declaration.
if (type.flags & TypeFlags.UniqueESSymbol && (isBindingElement(declaration) || !declaration.type) && type.symbol !== getSymbolOfDeclaration(declaration)) {
// always widen a 'unique symbol' type if the type was created for a different declaration and if it isn't accessible
if (type.flags & TypeFlags.UniqueESSymbol && !declaration.type && type.symbol !== getSymbolOfDeclaration(declaration) && !isValueSymbolAccessible(type.symbol, type.symbol.valueDeclaration)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling that passing type.symbol.valueDeclaration to isValueSymbolAccessible here might not be 100% correct but so far I wasn't able to write a test that would fail with this

@sandersn sandersn requested a review from weswigham July 18, 2023 23:43
@sandersn sandersn requested a review from iisaduan July 18, 2023 23:43
@Andarist
Copy link
Contributor Author

Andarist commented Aug 9, 2023

@jakebailey would u mind creating a playground for this one? :)

@jakebailey
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 9, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at d7dc3f9. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 9, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/156395/artifacts?artifactName=tgz&fileId=1A95C96C6617FFB7587CCF405EF9CDDDD6B202F8CA34673E41874DDECD34143302&fileName=/typescript-5.3.0-insiders.20230809.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, when I opened the original issue I had a specific kind of fix in mind - right now all literal types have "freshness" that determines if we widen them or not at specific locations. unique symbols are literal types which currently lack fresh and un-fresh variants, and instead just always widen (and always widen in more places than most other literal types do). Freshness for unique symbols is a bit more complicated though - they don't come from a literal, per sey, so it's not that expression literals produce fresh types, but rather that the Symbol() call and... const assignments of unique symbols (? or at least references to members of the global Symbol constructor?) produce "fresh" (widening) variants of the types.

In any case, I don't feel great about widening being controlled by an accessibility check - that's probably not good for perf, and doesn't actually check completely what you'd hope it does - it checks if the symbol is accessible via simple dotted accesses from a name at the given location, but the symbol may, for example, be a member of a type and is thus still "accessible" but via an indexed access on a type symbol... or something more convoluted. I just don't feel like accessibility determining widening behavior has great rules or a great feel, and doesn't feel like it can be reliably replicated into a declaration file to preserve the widening behavior.

I dunno, I've talked about it before among the team and maybe @rbuckton has more thoughts on why unique symbols so aggressively widen today.

@Andarist
Copy link
Contributor Author

How would the narrowed-down fix for your issue look like? Checking if it comes from the global Symbol would suffer from some of the problems you just mentioned - unless I'd build a reverse lookup of what symbols are available on the global Symbol.

@sandersn
Copy link
Member

sandersn commented Dec 4, 2023

Note: this PR blocks #42104.

@weswigham would you be able to look at this again? It sounds like you wanted quite a different approach from what's currently here.

@jakebailey
Copy link
Member

I think this predated our design meeting in which we basically said "symbol should do widening just like literals but doesn't" such that a final fix will probably be to rework symbols to have some sort of freshness. I just haven't had time to do so.

That or I've misunderstood the situation.

@Andarist
Copy link
Contributor Author

Andarist commented Dec 6, 2023

I think this predated our design meeting in which we basically said "symbol should do widening just like literals but doesn't" such that a final fix will probably be to rework symbols to have some sort of freshness. I just haven't had time to do so.

If you are already committed to this kind of change then it's probably best to close this PR.

@sandersn sandersn closed this Apr 1, 2025
@github-project-automation github-project-automation bot moved this from Waiting on reviewers to Done in PR Backlog Apr 1, 2025
@sandersn sandersn removed this from PR Backlog Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unique symbols from the global SymbolConstructor widen way too eagerly
5 participants