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

[isolatedDeclarations][5.5] Disagreement between transpileDeclaration API and typechecker on Symbol.iterator as computed property #58490

Closed
MichaelMitchell-at opened this issue May 9, 2024 · 7 comments Β· Fixed by #58596
Labels
Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag Domain: ts.transpileDeclaration Issues regarding the transpileDeclaration API, which do not reproduce without it

Comments

@MichaelMitchell-at
Copy link

πŸ”Ž Search Terms

isolated declarations transpileDeclaration symbol iterator

πŸ•— Version & Regression Information

  • nightly

⏯ Playground Link

https://tsplay.dev/wXLvLw

πŸ’» Code

export class Foo implements Iterable<number> {
  public [Symbol.iterator]() {
    return {
      next: () => ({ value: 42, done: false }),
    };
  }
}

πŸ™ Actual behavior

ts.transpileDeclaration has diagnostic: Computed properties must be number or string literals, variables or dotted expressions with --isolatedDeclarations.

tsc --isolatedDeclarations completes successfully

πŸ™‚ Expected behavior

I'm not sure which is more correct here. If we treat this as not an error, then that seems to imply that Symbol.iterator is treated specially, which runs into weird edge cases like Symbol being overridden in the current scope. But if we do treat this as an error, then the question becomes how does one add an explicit type annotation for this case?

Additional information about the issue

No response

@MichaelMitchell-at
Copy link
Author

@dragomirtitian @weswigham here's a fun one for y'all πŸ™ƒ

@dragomirtitian
Copy link
Contributor

For computed properties we allow them to be used in isolated declarations even if we can't determine the type is valid locally. This was due to the fact that computed properties with unique symbols would otherwise be completely unusable with isolated declarations.

In transpileDecalarations the checker should only be used when needed. In this case the checker will not actually find a type for Symbol.iterator so it will complain. I think since transpileDecalarations uses noChecck it should probably fall back on the isolated declaration test and just emit the entity name even if it can't check the type is correct. @weswigham thought?

@weswigham weswigham added the Domain: ts.transpileDeclaration Issues regarding the transpileDeclaration API, which do not reproduce without it label May 13, 2024
@weswigham
Copy link
Member

weswigham commented May 13, 2024

it should probably fall back on the isolated declaration test and just emit the entity name even if it can't check the type is correct

It is. The issue is that the check for the ID error (in visitDeclarationSubtree in declarations.ts) relies on elements of the Symbol global resolving, in this case. Specifically, that resolver.isEntityNameVisible check in the condition. If the checker isn't running and you have no other files in the program, the name isn't gonna resolve for a missing global. Yeah, that would be a checker error if the checker was running in the same noResove-ish single-file mode, but if you're not even capturing those... ??? The error behavior here is checker dependent because of the entity name lookup - hence the difference in command-line (where the global resolves) and API (where it doesn't) behavior.

Symbols are a big hole in isolatedDeclarations usability that we don't have great answers for right now. For builtin symbols, I could easily remove the error by passing a more complete lib into the transpileDeclarations program, so Symbol.iterator is recognized as a valid late-bound property, but that's basically a stop-gap. Custom symbols like

const sym = Symbol();
export class Foo {
  public [sym]() {
    return {
      next: () => ({ value: 42, done: false }),
    };
  }
}

I've currently patched into emitting "correctly", but still issuing the isolatedDeclarations error - I imagine this case should be similar. The ID error should appear in all cases (even when the name fails to resolve), but the emit should be reasonably sane.

@MichaelMitchell-at
Copy link
Author

still issuing the isolatedDeclarations error

Would you happen to know of a clean and concise way to annotate code following this pattern to satisfy isolatedDeclarations? IIRC they can't be suppressed by @ts-expect-error (on my phone so can't validate quickly)

@weswigham
Copy link
Member

weswigham commented May 13, 2024

The issue with the symbol computed names is that there isn't one - to write it you'd still need a type with a symbol computed name, which then has the same issue. Call it a flaw with the whole idea that you can do declaration emit without expression checking, since computed names mean even types can rely on (nonlocal!) expression evaluation - minimally you need to make breaking assumptions that expressions in computed names probably resolve to valid property names that probably don't conflict with other property names.

Or you give up and do some limited checking-like analysis - looking for references to the global Symbol and members thereof. This is probably the direction we'll go - just finding the right way to slice the analysis is weird. It's unclear how much of the lib we should bring in.

@MichaelMitchell-at
Copy link
Author

Hm. Wonder if it would be too special casey to look for expressions that match the pattern Symbol.iterator satisfies unique symbol and emit without error in those cases

@weswigham
Copy link
Member

weswigham commented May 13, 2024

To be even more explicit, this

// @filename: defines.d.ts
export const mySymbol: unique symbol;
// @filename: usage.ts
import {mySymbol} from "./defines.js"
export class Foo {
  [mySymbol]() {}
}

is something you want to allow because it's how symbols are meant to be used, but

// @filename: defines.d.ts
export const mySymbol: string;
// @filename: usage.ts
import {mySymbol} from "./defines.js"
export class Foo {
  [mySymbol]() {}
}

is something you need to forbid because you don't know if mySymbol conflicts with anything. And when you only look at usage.ts, these two cases are identical. So you're either conservative and forbid basically all computed names (very bad for usability), incredibly loose and allow most computed names (which is a big footgun for anyone relying on the output computed name actually working/existing when transformed into a type), or you slice it in some weird way to sometimes work and sometimes not based on some common usecase heuristics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag Domain: ts.transpileDeclaration Issues regarding the transpileDeclaration API, which do not reproduce without it
Projects
None yet
3 participants