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

Shorthand property #1127

Merged
merged 14 commits into from
Nov 17, 2014
Merged

Shorthand property #1127

merged 14 commits into from
Nov 17, 2014

Conversation

yuit
Copy link
Contributor

@yuit yuit commented Nov 11, 2014

Implementation of short-hand property assignment in compiler and language service. Initial conformance tests are added and more tests for fourslash will be added.

@@ -110,7 +110,8 @@ module ts {
getAliasedSymbol: resolveImport,
isUndefinedSymbol: symbol => symbol === undefinedSymbol,
isArgumentsSymbol: symbol => symbol === argumentsSymbol,
hasEarlyErrors: hasEarlyErrors
hasEarlyErrors: hasEarlyErrors,
resolveEntityNameForShortHandPropertyAssignment: resolveEntityNameForShortHandPropertyAssignment,
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like an extremely specific operation to export from the checker. Why can't we export our normal members, and they check once they were called if they were called on a shorthand property assignment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can certainly do that. I was just trying to minimize the exposure of the entire resolveEntityName since we only need it in the case of short-hand. Also we have to manually pass in the meaning flag

Copy link
Contributor

Choose a reason for hiding this comment

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

What about getSymbolInfo? That should eventually call resolveEntityName

Copy link
Contributor

Choose a reason for hiding this comment

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

the problem here is that we have two meaning for the node. so when you ask getSymbolInfo, which symbol do you want, the value or the declaration.

Copy link
Contributor

Choose a reason for hiding this comment

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

This duality is common in other languages (like VB). For example, in a VB query, an identifier can be a query variable, or the name of something referenced. This is one of the reasons that roslyn separates out the concept of 'GetDeclaredSymbol' (to give you info about the thing being declared) versus 'GetSymbolInfo' (which will give you info about something being referenced).

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand what Cyrus is saying, this duality can be dealt with in the services layer, possibly by making two calls to the typeInfoResolver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, then should we do something similar to rosyln and have a function call getDeclaredSymbol in the case we want a declaration symbol instead of value one

@yuit
Copy link
Contributor Author

yuit commented Nov 15, 2014

I will be having separate CR for using shorthand property assignment in compiler

@mhegazy
Copy link
Contributor

mhegazy commented Nov 17, 2014

👍

yuit pushed a commit that referenced this pull request Nov 17, 2014
@yuit yuit merged commit acc2550 into master Nov 17, 2014
@yuit yuit deleted the shorthandProperty branch November 19, 2014 03:15
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants