Skip to content

Allowed AsExpression-like type assertions on object literal shorthands #21855

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

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Feb 10, 2018

This PR adds a type?: TypeNode property to the ShorthandPropertyAssignment interface, and adds it to the allowed tokens for shorthand property assignments in parsing. In checking, if a member type is declared on a shorthand assignment, it is used instead of the name.

I was uncertain how best to describe this AST behavior. Two other options I decided against were:

  • Have the node be an AsExpression instead of a ShorthandPropertyAssignment: would break the existing assumption that all properties on an ObjectLiteral contain a .name.
  • Have the node contain an AsExpression: the name of the shorthand node would point to the same area as the expression's .type.

Open issues:

  • Intentionally leaves out the case of <type>name type assertions. I would love to get to those in a separate PR, but it looks like those would need different changes. Same PR for coherency or different PR for smaller changes?

  • An object shorthand's type being before its name would imply it's a type assertion, while being after the name implies it's an as assertion. That's the smallest API change but also a little hacky feeling. Would love to know of a preferred way... Perhaps having either an asType and assertionType but not both?

  • Is asType.parent = node; the correct way to set up the child node's .parent? Without that line, in local tests it would fail on a union type (e.g. age as number | string) with:~~

    TypeError: Cannot read property 'kind' of undefined
        at getAliasSymbolForTypeNode (C:\Code\typescript\lib\tsc.js:27445:31)
    

    ...which maps to node.parent.kind.

    There's a test failing for it, but I couldn't figure out where the right place is.

    Found it: added it to forEachChild.

Fixes #13035.

Josh Goldberg added 3 commits February 10, 2018 01:37
This PR adds a `type?: TypeNode` property to the `ShorthandPropertyAssignment` interface, and adds it to the allowed tokens for shorthand property assignments in parsing. In checking, if a member type is declared on a shorthand assignment, it is used instead of the name.

Fixes microsoft#13035
@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Feb 16, 2018

After cc9f921, confirmed this worked in VS Code. I've never added such a visible feature before, though, so I'd love pointers if I'm missing test cases!

image

@JoshuaKGoldberg JoshuaKGoldberg force-pushed the type-casts-object-shorthand branch from b590cda to cc9f921 Compare February 16, 2018 08:31
# Conflicts:
#	src/compiler/checker.ts
@typescript-bot
Copy link
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@JoshuaKGoldberg
Copy link
Contributor Author

@RyanCavanaugh I would still like this to be merged in, please!

@RyanCavanaugh
Copy link
Member

@JoshuaKGoldberg #13035 hasn't been approved yet; we can't accept PRs for syntax additions outside that process

@JoshuaKGoldberg JoshuaKGoldberg deleted the type-casts-object-shorthand branch March 22, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants