-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Parse quoted names in qualified names #40708
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
Conversation
@typescript-bot pack this |
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 7ba8bbf. You can monitor the build here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven’t gone over the declaration emitter changes in much detail, but have a couple questions to start with
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(2,15): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(4,15): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(4,32): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(7,20): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(10,18): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(11,18): error TS2554: Expected 1 arguments, but got 2. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(13,14): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(14,11): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(15,8): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(16,11): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(17,17): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(18,5): error TS2322: Type 'undefined' is not assignable to type 'number | null'. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(18,17): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(20,16): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(21,16): error TS8020: JSDoc types can only be used inside documentation comments. | ||
tests/cases/conformance/jsdoc/jsdocDisallowedInTypescript.ts(22,17): error TS8020: JSDoc types can only be used inside documentation comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a lot of these shouldn’t have gone away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the error on .<T>
outside jsdoc into a parse error to simplify the types, which has the side effect of silencing these grammar errors until it's fixed (grammar errors issued in the checker are only issued when the containing file has no parse errors).
return factory.updateIdentifier(name, typeArguments); | ||
} | ||
if (isStringLiteralLike(name)) { | ||
return name; // TODO ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a string entity name have type arguments?
// @Filename: thing.js
/** @template T */
class Thing {
/** @param {T} x */
foo(x) {}
}
exports["a thing"] = Thing;
// @Filename: index.ts
type Thing<T> = import('./thing')."a thing"<T>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort-of yes - those type arguments are attached to the import type (or type reference) itself, though. The type arguments sometimes attached to identifiers are provided for the language service in some cases to provide detailed instantiation information for qualified names (like when we return strings like NS.Cls<T>.B
) - currently a StringLiteralLike
doesn't support appending those, so I'm just omitting them, rather than complicate the types. To be honest, I'd prefer to move them into the QualifiedName
structure instead of having them on Identifier
(and now potentially string literals) if we wanted to uniformly keep the behavior.
} | ||
|
||
export type EntityName = Identifier | QualifiedName; | ||
export type EntityName = Identifier | QualifiedName | StringLiteralLike; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the only kind of EntityName that can start with a StringLiteralLike is the qualifier on an ImportTypeNode, would it make more sense to make that a special case and leave the meaning of EntityName
unchanged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe; but when going in that direction, the suite of utility functions that needed to handle it was less clear. It could be worth the change, regardless.
Unfortunately, we never finished reviewing this PR. It is pretty old now, so I'm going to close it to reduce the number of open PRs. |
As I alluded to in #40679, the real root cause of #40152 is that we can't even reference the type-side of the non-identifier enum members via a type reference, because the reference syntax has no allowances for non-identifier names. This PR allows the type-side of non-identifier exports to be referenced within the type system via references of the form
NS."string name"
. Meaning you can now have code like this:Or like this:
If we want to take this (and I think we do, between current enum and js exports and upcoming wasm exports), we probably want to do it after
master
has swapped to 4.2, since the 4.1 beta has been cut - so there's not much time pressure to look into this yet - I just wanted to have this ready and available for consideration and discussion. We may also want to pair it with additional syntax on the declaration side to allow the declaration of non-identifier export names in ambient contexts (eg,export const "a thing": Type;
) - so js files (and upcoming wasm files) can be better represented with a declaration file.