Skip to content
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

Isolated declarations uses type information when emitting computed object keys #58533

Open
lucacasonato opened this issue May 14, 2024 · 6 comments
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases

Comments

@lucacasonato
Copy link

πŸ”Ž Search Terms

  • isolated declarations
  • computed keys
  • object literals

πŸ•— Version & Regression Information

5.5.0-dev.20240514

⏯ Playground Link

https://www.typescriptlang.org/play/?isolatedDeclarations=true&ts=5.5.0-dev.20240514#code/KYDwDg9gTgLgBAYwgOwM7wNLAJ5wLxwBEMAFsFtoQNwBQNAJsAgDYCGUwcA5sxAEatmcAN4044uKEixEKdHADivAcwr4ipYEv6CKhMRKnR4SNPACqyAJYBHAK7AAytgC2fCMwBccO9fudUV3dmWglJcGNZMzgAORRLWwdnNw9vQJSQmgBfOiMZU3kAQXVRMIBtCgBdbxgoBwAaA3Ey5OCAOhgIR1qrZC4AFVYuargAM0FUYEbyhP9WjxHx5knpiTLtFSrvJZXsmiA

πŸ’» Code

export const Key = "theKey";

declare global {
    export const GlobalKey = "theGlobalKey"
    export const UniqueSymbol: unique symbol;
    export const NonUniqueSymbol: symbol;
}

export const A = {
    [Key]: true,
    [Symbol.toStringTag]: false,
    [UniqueSymbol]: false,
    [GlobalKey]: false,
}

πŸ™ Actual behavior

TypeScript is emitting the computed property keys with type information that can not be determined without a type checker.

export declare const Key = "theKey";
declare global {
    export const GlobalKey = "theGlobalKey";
    export const UniqueSymbol: unique symbol;
    export const NonUniqueSymbol: symbol;
}
export declare const A: {
    theKey: boolean;
    [Symbol.toStringTag]: boolean;
    [UniqueSymbol]: boolean;
    theGlobalKey: boolean;
};

πŸ™‚ Expected behavior

export declare const Key = "theKey";
declare global {
    export const GlobalKey = "theGlobalKey";
    export const UniqueSymbol: unique symbol;
    export const NonUniqueSymbol: symbol;
}
export declare const A: {
    [Key]: true,
    [Symbol.toStringTag]: false,
    [UniqueSymbol]: false,
    [GlobalKey]: false,
};

Additional information about the issue

No response

@weswigham
Copy link
Member

weswigham commented May 14, 2024

FWIW, I don't think computed property names are compatible with isolatedDeclarations at a fundamental level - wherever we land on the fix is going to be a compromise. Computed names are, by their nature, a window from the type structure world into nonlocal expression checking space. I'm willing to say "we do the right thing when all the information is in the file" for the sake of convenience, since when the information isn't in the file, even a simple [key]: whatever is completely unsafe and not transformable into a type that's guaranteed to work the same way as the input implies. A export const a: {[expr]: number} does not have the same behavior as a export const a = {[expr]: 1}, and the differences all come down to the type of expr. As an expression, we allow expr to be string, any - any number of non-literal things. And depending on context, those are either elided entirely, transformed into index signatures, or maybe even splat out into multiple optional properties. In a type, expr must typecheck as a property name literal type to be valid (not a union thereof, not a PropertyKey extending param, exactly a literal type). Otherwise we issue a A computed property name in a type literal must refer to an expression whose type is a literal type or a 'unique symbol' type. error when we read the type back in.

When it comes down to it, there's no simple rule to transform from an expression computed name to a type one that actually works (without full potentially nonlocal context). Any "rule" we apply is heuristic, at best. :S

Anyways, also ref #58490 which is on the same subject.

@weswigham weswigham added the Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag label May 14, 2024
@lucacasonato
Copy link
Author

@weswigham I agree with you - however I think ID already handles this on a checker level, because the checker disallows any types in computed key expressions that are not string literals, number literals, or unique symbol - types that would result in type emitted output that is not a property member, but a index member or similar are already forbidden (number, string, symbol). See https://www.typescriptlang.org/play/?isolatedDeclarations=true&ts=5.5.0-dev.20240514#code/KYDwDg9gTgLgBAYwgOwM7wNLAJ4EYBcc6UAlsgOYDcAUKJLIiunFtgEyHICuAtgEbAoNOtHhI0mHAGZCqbPwgAbarXCjGEuAEE4AXjgBvanBNwA2q1wBdQjChdgAGmOmLONjbh2Hz0+dZSnt5O1AC+1EA

This issue was not about changing the checker, just about changing the emit behaviour specifically for the allowed types (string literals, number literals, or unique symbol), to retain the original expression rather than replacing it with the type value.

There is no semantic issue here as far as I can tell, it's just something that is needed to achieve token level equality of the output.

@weswigham
Copy link
Member

weswigham commented May 14, 2024

There is no semantic issue here as far as I can tell, it's just something that is needed to achieve token level equality of the output.

ish? The ID error can't be correctly emitted without full type information (see #58490), so you can't know if it's even safe to copy an expression computed name to a type computed name without doing whole-program typechecking. So while ID does have an error for this, it fails to emit it under the transpileDeclaration API (and just allows a potentially very wrong transform).

@lucacasonato
Copy link
Author

Yeah but that's already the case for other transforms, no? My understanding was that isolated declarations always operates on the assumption that the code is valid and does type check. Maybe I am misunderstanding this.

@lucacasonato
Copy link
Author

I retract my previous comment. I don't think it is reasonable for the emitter to have to make the assumption that the ID checker passed. Doing so would make it impossible to analyze without TSC's ID checker whether an arbitrary given file is eligible for ID emit in the first place, which makes the performance gains quite useless. You would still have to check all files anyway to determine whether you can ID emit them before emit.

@weswigham weswigham added Domain: Declaration Emit The issue relates to the emission of d.ts files Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases labels May 23, 2024
@weswigham
Copy link
Member

These are all errors for now, but as we allow some of these forms as special cases, we do want to think about possibly aligning the emit output where possible, so this'll track that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
Development

No branches or pull requests

2 participants