Skip to content

Adds quick info on modifiers and declaration keywords #3189

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Adds quick info on modifiers and declaration keywords #3189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7f209b
Add tests
tinganho May 17, 2015
64a0dd4
Corrects old tests
tinganho May 17, 2015
0283e79
Adds quick info on modifiers and declaration keywords
tinganho May 17, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ module ts {
else {
symbol = createSymbol(0, "__missing");
}
if (!node.name) {
symbol.isAnonymous = true;
}
addDeclarationToSymbol(symbol, node, includes);
symbol.parent = parent;

Expand Down Expand Up @@ -385,6 +388,9 @@ module ts {

function bindAnonymousDeclaration(node: Declaration, symbolKind: SymbolFlags, name: string, isBlockScopeContainer: boolean) {
let symbol = createSymbol(symbolKind, name);
if (!node.name) {
symbol.isAnonymous = true;
}
addDeclarationToSymbol(symbol, node, symbolKind);
bindChildren(node, symbolKind, isBlockScopeContainer);
}
Expand Down Expand Up @@ -424,7 +430,7 @@ module ts {

function bind(node: Node) {
node.parent = parent;

switch (node.kind) {
case SyntaxKind.TypeParameter:
bindDeclaration(<Declaration>node, SymbolFlags.TypeParameter, SymbolFlags.TypeParameterExcludes, /*isBlockScopeContainer*/ false);
Expand Down
29 changes: 29 additions & 0 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ module ts {
getReturnTypeOfSignature,
getSymbolsInScope,
getSymbolAtLocation,
getSymbolFromDeclarationKeyword,
getContextualSignature,
getShorthandAssignmentValueSymbol,
getTypeAtLocation,
typeToString,
Expand Down Expand Up @@ -11378,6 +11380,33 @@ module ts {
return undefined;
}

function getSymbolFromDeclarationKeyword(node: Node): Symbol {
switch(node.kind) {
case SyntaxKind.ClassKeyword:
Copy link
Contributor

Choose a reason for hiding this comment

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

why enable all these keywords? We used to that and found it to be noisy, specially that QuickInfo is triggered whenever the mouse moves. function expressions is a different story, they do not have a name, so there is no target to land on any ways. that is not the case for all other declarations, they all have names.

i would not enable all these, and only special case function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, shall I only special case it on anonymous functions too? Because if we enable it on function name() {} then to be consistent — all declaration keywords should have quick info.

case SyntaxKind.EnumKeyword:
case SyntaxKind.FunctionKeyword:
case SyntaxKind.InterfaceKeyword:
case SyntaxKind.ModuleKeyword:
case SyntaxKind.NamespaceKeyword:
if (node.parent.symbol) {
return node.parent.symbol;
}
return undefined;

case SyntaxKind.ConstKeyword:
case SyntaxKind.LetKeyword:
case SyntaxKind.VarKeyword:
if (node.parent.kind === SyntaxKind.VariableDeclarationList) {
// We only want to show quick info for declaration list of length 1 and on a non-binding pattern.
if ((<VariableDeclarationList>node.parent).declarations.length === 1 &&
(<VariableDeclarationList>node.parent).declarations[0].name.kind === SyntaxKind.Identifier) {
return getSymbolOfEntityNameOrPropertyAccessExpression(<Identifier>(<VariableDeclarationList>node.parent).declarations[0].name);
}
}
return undefined;
}
}

function getSymbolInfo(node: Node) {
if (isInsideWithStatementBody(node)) {
// We cannot answer semantic questions within a with block, do not proceed any further
Expand Down
5 changes: 4 additions & 1 deletion src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,11 +1148,13 @@ module ts {

getSymbolsInScope(location: Node, meaning: SymbolFlags): Symbol[];
getSymbolAtLocation(node: Node): Symbol;
getSymbolFromDeclarationKeyword(node: Node): Symbol;
getShorthandAssignmentValueSymbol(location: Node): Symbol;
getTypeAtLocation(node: Node): Type;
typeToString(type: Type, enclosingDeclaration?: Node, flags?: TypeFormatFlags): string;
symbolToString(symbol: Symbol, enclosingDeclaration?: Node, meaning?: SymbolFlags): string;
getSymbolDisplayBuilder(): SymbolDisplayBuilder;
getContextualSignature(node: FunctionExpression | MethodDeclaration): Signature;
getFullyQualifiedName(symbol: Symbol): string;
getAugmentedPropertiesOfType(type: Type): Symbol[];
getRootSymbols(symbol: Symbol): Symbol[];
Expand Down Expand Up @@ -1377,6 +1379,7 @@ module ts {
/* @internal */ exportSymbol?: Symbol; // Exported symbol associated with this symbol
valueDeclaration?: Declaration; // First value declaration of the symbol
/* @internal */ constEnumOnlyModule?: boolean; // True if module contains only const enums or other modules with only const enums
isAnonymous?: boolean; // True if declaration is anonymous
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need this? this information can be easily inferred from other sources, like the name, and the node flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhegazy I was trying a lot of different ways to pinpoint anonymous declarations. I just thought this was easier. But you're right, we shouldn't add something just to serve one very specific use case.

It would be good if you could help me out on the below section. Where I use this property to add a name of a declaration/symbol.

}

/* @internal */
Expand Down Expand Up @@ -1455,7 +1458,7 @@ module ts {
/* @internal */
ContainsUndefinedOrNull = 0x00040000, // Type is or contains Undefined or Null type
/* @internal */
ContainsObjectLiteral = 0x00080000, // Type is or contains object literal type
ContainsObjectLiteral = 0x00080000, // Type is or contains object literal type
ESSymbol = 0x00100000, // Type of symbol primitive introduced in ES6

/* @internal */
Expand Down
32 changes: 28 additions & 4 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,8 +917,8 @@ module ts {
return false;
}

export function isStatement(n: Node): boolean {
switch (n.kind) {
export function isStatement(node: Node): boolean {
switch (node.kind) {
case SyntaxKind.BreakStatement:
case SyntaxKind.ContinueStatement:
case SyntaxKind.DebuggerStatement:
Expand All @@ -944,8 +944,8 @@ module ts {
}
}

export function isClassElement(n: Node): boolean {
switch (n.kind) {
export function isClassElement(node: Node): boolean {
switch (node.kind) {
case SyntaxKind.Constructor:
case SyntaxKind.PropertyDeclaration:
case SyntaxKind.MethodDeclaration:
Expand All @@ -959,6 +959,30 @@ module ts {
}
}

export function isHeritageKeyword(node: Node): boolean {
return node.kind === SyntaxKind.ImplementsKeyword || node.kind === SyntaxKind.ExtendsKeyword;
}

export function isDeclarationKeyword(node: Node): boolean {
switch (node.kind) {
case SyntaxKind.ClassKeyword:
case SyntaxKind.ConstKeyword:
case SyntaxKind.DefaultKeyword:
case SyntaxKind.EnumKeyword:
case SyntaxKind.ExtendsKeyword:
case SyntaxKind.FunctionKeyword:
case SyntaxKind.InterfaceKeyword:
case SyntaxKind.ImplementsKeyword:
case SyntaxKind.LetKeyword:
case SyntaxKind.ModuleKeyword:
case SyntaxKind.NamespaceKeyword:
case SyntaxKind.SetKeyword:
case SyntaxKind.VarKeyword:
return true;
}
return false;
}

// True if the given identifier, string literal, or number literal is the name of a declaration node
export function isDeclarationName(name: Node): boolean {
if (name.kind !== SyntaxKind.Identifier && name.kind !== SyntaxKind.StringLiteral && name.kind !== SyntaxKind.NumericLiteral) {
Expand Down
2 changes: 1 addition & 1 deletion src/services/formatting/formatting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@ module ts.formatting {
return inheritedIndentation;
}

if (isToken(child)) {
if (isReservedWord(child)) {
// if child node is a token, it does not impact indentation, proceed it using parent indentation scope rules
let tokenInfo = formattingScanner.readTokenInfo(child);
Debug.assert(tokenInfo.token.end === child.end);
Expand Down
2 changes: 1 addition & 1 deletion src/services/formatting/formattingScanner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ module ts.formatting {
// then kind needs to be fixed. This might happen in cases
// when parser interprets token differently, i.e keyword treated as identifier
function fixTokenKind(tokenInfo: TokenInfo, container: Node): TokenInfo {
if (isToken(container) && tokenInfo.token.kind !== container.kind) {
if (isReservedWord(container) && tokenInfo.token.kind !== container.kind) {
tokenInfo.token.kind = container.kind;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want Token here.. @vladima?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just renamed isToken to isReservedWord. I thought isToken where to ambitious. And when I look at the implementation – it only contains a check for reserved words.

}
return tokenInfo;
Expand Down
29 changes: 23 additions & 6 deletions src/services/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,6 @@ module ts {
let list = createNode(SyntaxKind.SyntaxList, nodes.pos, nodes.end, NodeFlags.Synthetic, this);
list._children = [];
let pos = nodes.pos;



for (let node of nodes) {
if (pos < node.pos) {
Expand Down Expand Up @@ -3672,7 +3670,7 @@ module ts {
addFullSymbolName(symbol);
writeTypeParametersOfSymbol(symbol, sourceFile);
}
if ((symbolFlags & SymbolFlags.Interface) && (semanticMeaning & SemanticMeaning.Type)) {
if (symbolFlags & SymbolFlags.Interface) {
addNewLineIfDisplayPartsExist();
displayParts.push(keywordPart(SyntaxKind.InterfaceKeyword));
displayParts.push(spacePart());
Expand Down Expand Up @@ -3839,7 +3837,9 @@ module ts {
if (symbolKind) {
pushTypePart(symbolKind);
displayParts.push(spacePart());
addFullSymbolName(symbol);
if (!symbol.isAnonymous) {
addFullSymbolName(symbol);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhegazy here is where I build the display part and I use .isAnonymous property to decide whether to add a symbol name. I did a lot of checks before to pin point anonymous declarations. Do you happen to know an effective way without using .isAnonymous?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like:

node.name ? node.name.text : node.flags & NodeFlags.Default ? "default" : ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You only got the symbol?

And the symbol.declarations[0] node is not always defined there and I know if I add that check it will breaks some other parts.

Also as I recall it the symbol name for a default function is just default. I can't just check for default symbol name because a function named default will not have a name then.

That's why I added the .isAnonymous property.

Copy link
Contributor

Choose a reason for hiding this comment

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

first sorry for the delay, just recovering from a long vacation :D.

i am not sure i understand what isAnonymous gets you that a check for name does not. so something like isAnonymous = symbol.valueDeclaration && symbol.valueDeclaration.name;?

Copy link
Contributor

Choose a reason for hiding this comment

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

and if understand correctly, what we are trying to handle here is export default function and class declarations with no name, and this change is looking much bigger than the scope of the original problem. in my mind these two cases can be easily special cased in getQuickInfo.

}
}

Expand All @@ -3852,6 +3852,9 @@ module ts {
case ScriptElementKind.constructorImplementationElement:
displayParts.push(textOrKeywordPart(symbolKind));
return;
case ScriptElementKind.localFunctionElement:
displayParts.push(textOrKeywordPart(ScriptElementKind.functionElement));
return;
default:
displayParts.push(punctuationPart(SyntaxKind.OpenParenToken));
displayParts.push(textOrKeywordPart(symbolKind));
Expand Down Expand Up @@ -3882,11 +3885,22 @@ module ts {
}
}

function getQuickInfoNodeBySkippingModifiersAndHeritageKeywords(sourceFile: SourceFile, position: number, beignWithLastTokenPath = false): Node {
let node = getTouchingPropertyName(sourceFile, position, beignWithLastTokenPath);
if (!node) {
return undefined;
}
else if (isModifier(node.kind) || isHeritageKeyword(node)) {
node = getQuickInfoNodeBySkippingModifiersAndHeritageKeywords(sourceFile, node.end + 1, /*beignWithLastTokenPath*/ true);
}
return node;
}

function getQuickInfoAtPosition(fileName: string, position: number): QuickInfo {
synchronizeHostData();

let sourceFile = getValidSourceFile(fileName);
let node = getTouchingPropertyName(sourceFile, position);
let node = getQuickInfoNodeBySkippingModifiersAndHeritageKeywords(sourceFile, position);
if (!node) {
return undefined;
}
Expand All @@ -3897,6 +3911,9 @@ module ts {

let typeChecker = program.getTypeChecker();
let symbol = typeChecker.getSymbolAtLocation(node);
if(!symbol && isDeclarationKeyword(node)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

as i mentioned earlier, i would check for function keyword here and special case it.

symbol = typeChecker.getSymbolFromDeclarationKeyword(node);
}

if (!symbol) {
// Try getting just type at this position and show
Expand Down Expand Up @@ -6228,7 +6245,7 @@ module ts {
if (textSpanIntersectsWith(span, element.getFullStart(), element.getFullWidth())) {
let children = element.getChildren();
for (let child of children) {
if (isToken(child)) {
if (isReservedWord(child)) {
classifyToken(child);
}
else {
Expand Down
36 changes: 22 additions & 14 deletions src/services/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,37 +266,45 @@ module ts {
/* Gets the token whose text has range [start, end) and position >= start
* and (position < end or (position === end && token is keyword or identifier or numeric\string litera))
*/
export function getTouchingPropertyName(sourceFile: SourceFile, position: number): Node {
return getTouchingToken(sourceFile, position, n => isPropertyName(n.kind));
export function getTouchingPropertyName(sourceFile: SourceFile, position: number, beignWithLastTokenPath = false): Node {
return getTouchingToken(sourceFile, position, n => isPropertyName(n.kind), /*beignWithLastTokenPath*/ beignWithLastTokenPath);
}

/** Returns the token if position is in [start, end) or if position === end and includeItemAtEndPosition(token) === true */
export function getTouchingToken(sourceFile: SourceFile, position: number, includeItemAtEndPosition?: (n: Node) => boolean): Node {
return getTokenAtPositionWorker(sourceFile, position, /*allowPositionInLeadingTrivia*/ false, includeItemAtEndPosition);
export function getTouchingToken(sourceFile: SourceFile, position: number, includeItemAtEndPosition?: (n: Node) => boolean, beignWithLastTokenPath = false): Node {
return getTokenAtPositionWorker(sourceFile, position, /*allowPositionInLeadingTrivia*/ false, includeItemAtEndPosition, /*beignWithLastTokenPath*/ beignWithLastTokenPath);
}

/** Returns a token if position is in [start-of-leading-trivia, end) */
export function getTokenAtPosition(sourceFile: SourceFile, position: number): Node {
return getTokenAtPositionWorker(sourceFile, position, /*allowPositionInLeadingTrivia*/ true, /*includeItemAtEndPosition*/ undefined);
}

export let lastTokenPath: { [depth: number]: number } = {};

/** Get the token whose text contains the position */
function getTokenAtPositionWorker(sourceFile: SourceFile, position: number, allowPositionInLeadingTrivia: boolean, includeItemAtEndPosition: (n: Node) => boolean): Node {
function getTokenAtPositionWorker(sourceFile: SourceFile, position: number, allowPositionInLeadingTrivia: boolean, includeItemAtEndPosition: (n: Node) => boolean, beignWithLastTokenPath = false): Node {
let current: Node = sourceFile;
let depthLevel = 0;
if(!beignWithLastTokenPath) {
lastTokenPath = {};
}
outer: while (true) {
if (isToken(current)) {
// exit early
if (isReservedWord(current)) {
return current;
}

// find the child that contains 'position'

// Find the child that contains 'position' and it will begin with the old token path if specified.
let start = beignWithLastTokenPath && lastTokenPath[depthLevel] ? lastTokenPath[depthLevel] : 0;
for (let i = 0, n = current.getChildCount(sourceFile); i < n; i++) {
let child = current.getChildAt(i);
let start = allowPositionInLeadingTrivia ? child.getFullStart() : child.getStart(sourceFile);
if (start <= position) {
let end = child.getEnd();
if (position < end || (position === end && child.kind === SyntaxKind.EndOfFileToken)) {
current = child;
lastTokenPath[depthLevel] = i;
depthLevel++;
continue outer;
}
else if (includeItemAtEndPosition && end === position) {
Expand All @@ -323,7 +331,7 @@ module ts {
// Ideally, getTokenAtPosition should return a token. However, it is currently
// broken, so we do a check to make sure the result was indeed a token.
let tokenAtPosition = getTokenAtPosition(file, position);
if (isToken(tokenAtPosition) && position > tokenAtPosition.getStart(file) && position < tokenAtPosition.getEnd()) {
if (isReservedWord(tokenAtPosition) && position > tokenAtPosition.getStart(file) && position < tokenAtPosition.getEnd()) {
return tokenAtPosition;
}

Expand All @@ -334,7 +342,7 @@ module ts {
return find(parent);

function find(n: Node): Node {
if (isToken(n) && n.pos === previousToken.end) {
if (isReservedWord(n) && n.pos === previousToken.end) {
// this is token that starts at the end of previous token - return it
return n;
}
Expand All @@ -360,7 +368,7 @@ module ts {
return find(startNode || sourceFile);

function findRightmostToken(n: Node): Node {
if (isToken(n)) {
if (isReservedWord(n)) {
return n;
}

Expand All @@ -371,7 +379,7 @@ module ts {
}

function find(n: Node): Node {
if (isToken(n)) {
if (isReservedWord(n)) {
return n;
}

Expand Down Expand Up @@ -447,7 +455,7 @@ module ts {
return undefined;
}

export function isToken(n: Node): boolean {
export function isReservedWord(n: Node): boolean {
return n.kind >= SyntaxKind.FirstToken && n.kind <= SyntaxKind.LastToken;
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry just saw that. that is fine then.

}

Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/completionListInNamedFunctionExpression.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ goTo.marker("insideFunctionExpression");
verify.memberListContains("foo");

goTo.marker("referenceInsideFunctionExpression");
verify.quickInfoIs("(local function) foo(): number");
verify.quickInfoIs("function foo(): number");

goTo.marker("referenceInGlobalScope");
verify.quickInfoIs("function foo(a: number): string");
4 changes: 2 additions & 2 deletions tests/cases/fourslash/completionListWithModulesInsideModuleScope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ goToMarkAndGeneralVerify('mod1');
// from function in mod1
goToMarkAndGeneralVerify('function');
verify.completionListContains('bar', '(local var) bar: number');
verify.completionListContains('foob', '(local function) foob(): void');
verify.completionListContains('foob', 'function foob(): void');

// from class in mod1
goToMarkAndGeneralVerify('class');
Expand All @@ -303,7 +303,7 @@ verify.completionListContains('m1eMod', 'namespace mod1mod.m1eMod');
// from exported function in mod1
goToMarkAndGeneralVerify('exportedFunction');
verify.completionListContains('bar', '(local var) bar: number');
verify.completionListContains('foob', '(local function) foob(): void');
verify.completionListContains('foob', 'function foob(): void');

// from exported class in mod1
goToMarkAndGeneralVerify('exportedClass');
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/completionListWithModulesOutsideModuleScope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ verifyNotContainInterfaceMembers();
// from function scope
goToMarkAndGeneralVerify('function');
verify.completionListContains('sfvar', '(local var) sfvar: number');
verify.completionListContains('sffn', '(local function) sffn(): void');
verify.completionListContains('sffn', 'function sffn(): void');

verifyNotContainClassMembers();
verifyNotContainInterfaceMembers();
Expand Down
Loading