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

Script side implementation for Brace Completion. #7587

Merged
merged 6 commits into from
Apr 15, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 44 additions & 9 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ namespace FourSlash {
this.assertItemInCompletionList(completions.entries, symbol, text, documentation, kind);
}
else {
this.raiseError(`No completions at position '${ this.currentCaretPosition }' when looking for '${ symbol }'.`);
this.raiseError(`No completions at position '${this.currentCaretPosition}' when looking for '${symbol}'.`);
}
}

Expand Down Expand Up @@ -1752,13 +1752,13 @@ namespace FourSlash {
const actual = (<ts.server.SessionClient>this.languageService).getProjectInfo(
this.activeFile.fileName,
/* needFileNameList */ true
);
);
assert.equal(
expected.join(","),
actual.fileNames.map( file => {
actual.fileNames.map(file => {
return file.replace(this.basePath + "/", "");
}).join(",")
);
}).join(",")
);
}
}

Expand Down Expand Up @@ -1844,6 +1844,37 @@ namespace FourSlash {
});
}

public verifyBraceCompletionAtPostion(negative: boolean, openingBrace: string) {

const openBraceMap: ts.Map<ts.CharacterCodes> = {
Copy link
Member

Choose a reason for hiding this comment

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

For every call you're going to be creating this, so make this a static property

"(": ts.CharacterCodes.openParen,
"{": ts.CharacterCodes.openBrace,
"[": ts.CharacterCodes.openBracket,
"'": ts.CharacterCodes.singleQuote,
'"': ts.CharacterCodes.doubleQuote,
"`": ts.CharacterCodes.backtick,
"<": ts.CharacterCodes.lessThan
};

const charCode = openBraceMap[openingBrace];

if (!charCode) {
this.raiseError(`Invalid openingBrace '${openingBrace}' specified.`);
}
Copy link
Member

Choose a reason for hiding this comment

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

Consider using a Map<CharacterCodes> and writing

const openBraceMap: Map<CharacterCodes> = {
    "{": CharacterCodes.openBrace,
     // etc.
}

Copy link
Member

Choose a reason for hiding this comment

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

You should error when the test-writer accidentally gave the wrong input.


const position = this.currentCaretPosition;

const validBraceCompletion = this.languageService.isValidBraceCompletionAtPostion(this.activeFile.fileName, position, charCode);

if (!negative && !validBraceCompletion) {
this.raiseError(`${position} is not a valid brace completion position for ${openingBrace}`);
}

if (negative && validBraceCompletion) {
this.raiseError(`${position} is a valid brace completion position for ${openingBrace}`);
}
}

public verifyMatchingBracePosition(bracePosition: number, expectedMatchPosition: number) {
const actual = this.languageService.getBraceMatchingAtPosition(this.activeFile.fileName, bracePosition);

Expand Down Expand Up @@ -2233,7 +2264,7 @@ namespace FourSlash {
};

const host = Harness.Compiler.createCompilerHost(
[ fourslashFile, testFile ],
[fourslashFile, testFile],
(fn, contents) => result = contents,
ts.ScriptTarget.Latest,
Harness.IO.useCaseSensitiveFileNames(),
Expand All @@ -2258,7 +2289,7 @@ namespace FourSlash {
function runCode(code: string, state: TestState): void {
// Compile and execute the test
const wrappedCode =
`(function(test, goTo, verify, edit, debug, format, cancellation, classification, verifyOperationIsCancelled) {
`(function(test, goTo, verify, edit, debug, format, cancellation, classification, verifyOperationIsCancelled) {
${code}
})`;
try {
Expand Down Expand Up @@ -2372,7 +2403,7 @@ ${code}
}
}
}
// TODO: should be '==='?
// TODO: should be '==='?
}
else if (line == "" || lineLength === 0) {
// Previously blank lines between fourslash content caused it to be considered as 2 files,
Expand Down Expand Up @@ -2864,6 +2895,10 @@ namespace FourSlashInterface {
public verifyDefinitionsName(name: string, containerName: string) {
this.state.verifyDefinitionsName(this.negative, name, containerName);
}

public isValidBraceCompletionAtPostion(openingBrace: string) {
this.state.verifyBraceCompletionAtPostion(this.negative, openingBrace);
}
}

export class Verify extends VerifyNegatable {
Expand Down Expand Up @@ -3082,7 +3117,7 @@ namespace FourSlashInterface {
this.state.getSemanticDiagnostics(expected);
}

public ProjectInfo(expected: string []) {
public ProjectInfo(expected: string[]) {
this.state.verifyProjectInfo(expected);
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,9 @@ namespace Harness.LanguageService {
getDocCommentTemplateAtPosition(fileName: string, position: number): ts.TextInsertion {
return unwrapJSONCallResult(this.shim.getDocCommentTemplateAtPosition(fileName, position));
}
isValidBraceCompletionAtPostion(fileName: string, position: number, openingBrace: number): boolean {
return unwrapJSONCallResult(this.shim.isValidBraceCompletionAtPostion(fileName, position, openingBrace));
}
getEmitOutput(fileName: string): ts.EmitOutput {
return unwrapJSONCallResult(this.shim.getEmitOutput(fileName));
}
Expand Down
4 changes: 4 additions & 0 deletions src/server/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,10 @@ namespace ts.server {
throw new Error("Not Implemented Yet.");
}

isValidBraceCompletionAtPostion(fileName: string, position: number, openingBrace: number): boolean {
throw new Error("Not Implemented Yet.");
}

getBraceMatchingAtPosition(fileName: string, position: number): TextSpan[] {
var lineOffset = this.positionToOneBasedLineOffset(fileName, position);
var args: protocol.FileLocationRequestArgs = {
Expand Down
33 changes: 33 additions & 0 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1108,6 +1108,8 @@ namespace ts {

getDocCommentTemplateAtPosition(fileName: string, position: number): TextInsertion;

isValidBraceCompletionAtPostion(fileName: string, position: number, openingBrace: number): boolean;

getEmitOutput(fileName: string): EmitOutput;

getProgram(): Program;
Expand Down Expand Up @@ -7310,6 +7312,36 @@ namespace ts {
return { newText: result, caretOffset: preamble.length };
}

function isValidBraceCompletionAtPostion(fileName: string, position: number, openingBrace: number): boolean {

// '<' is currently not supported, figuring out if we're in a Generic Type vs. a comparison is too
// expensive to do during typing scenarios
// i.e. whether we're dealing with:
// var x = new foo<| ( with class foo<T>{} )
// or
// var y = 3 <|
Copy link
Member

Choose a reason for hiding this comment

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

Why not always complete, even in the var y = 3 < { case? If it's just a typo, then the user will just backspace immediately, which should delete both the { and the }.

Copy link
Member

Choose a reason for hiding this comment

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

That's a very unpleasant experience for if you're just trying to write <. You shouldn't make the user do more work to undo the language service's mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

Discussed with Paul offline. This is for completion of '<' with '>'. I thought it was for completion of '{' with '}'.

if (openingBrace === CharacterCodes.lessThan) {
return false;
}

const sourceFile = syntaxTreeCache.getCurrentSourceFile(fileName);

// Check if in a context where we don't want to perform any insertion
if (isInString(sourceFile, position) || isInComment(sourceFile, position)) {
return false;
}

if (isInsideJsxElementOrAttribute(sourceFile, position)) {
return openingBrace === CharacterCodes.openBrace;
}

if (isInTemplateString(sourceFile, position)) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand, why wouldn't you want to complete a curly brace following a ${?

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 don't support brace completion in an existing brace completion session. This is a VS limitation, and the required additional effort doesn't seem worth it (at the moment).

Copy link
Member

Choose a reason for hiding this comment

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

(as discussed offline)

}

return true;
}

function getParametersForJsDocOwningNode(commentOwner: Node): ParameterDeclaration[] {
if (isFunctionLike(commentOwner)) {
return commentOwner.parameters;
Expand Down Expand Up @@ -7604,6 +7636,7 @@ namespace ts {
getFormattingEditsForDocument,
getFormattingEditsAfterKeystroke,
getDocCommentTemplateAtPosition,
isValidBraceCompletionAtPostion,
getEmitOutput,
getSourceFile,
getProgram
Expand Down
14 changes: 14 additions & 0 deletions src/services/shims.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,13 @@ namespace ts {
*/
getDocCommentTemplateAtPosition(fileName: string, position: number): string;

/**
* Returns JSON-encoded boolean to indicate whether we should support brace location
* at the current position.
* E.g. we don't want brace completion inside string-literals, comments, etc.
*/
isValidBraceCompletionAtPostion(fileName: string, position: number, openingBrace: number): string;

getEmitOutput(fileName: string): string;
}

Expand Down Expand Up @@ -725,6 +732,13 @@ namespace ts {
);
}

public isValidBraceCompletionAtPostion(fileName: string, position: number, openingBrace: number): string {
return this.forwardJSONCall(
`isValidBraceCompletionAtPostion('${fileName}', ${position}, ${openingBrace})`,
() => this.languageService.isValidBraceCompletionAtPostion(fileName, position, openingBrace)
);
}

/// GET SMART INDENT
public getIndentationAtPosition(fileName: string, position: number, options: string /*Services.EditorOptions*/): string {
return this.forwardJSONCall(
Expand Down
44 changes: 42 additions & 2 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -429,21 +429,61 @@ namespace ts {

export function isInString(sourceFile: SourceFile, position: number) {
let token = getTokenAtPosition(sourceFile, position);
return token && (token.kind === SyntaxKind.StringLiteral || token.kind === SyntaxKind.StringLiteralType) && position > token.getStart();
return token && (token.kind === SyntaxKind.StringLiteral || token.kind === SyntaxKind.StringLiteralType) && position > token.getStart(sourceFile);
}

export function isInComment(sourceFile: SourceFile, position: number) {
return isInCommentHelper(sourceFile, position, /*predicate*/ undefined);
}

/**
* returns true if the position is in between the open and close elements of an JSX expression.
*/
export function isInsideJsxElementOrAttribute(sourceFile: SourceFile, position: number) {
let token = getTokenAtPosition(sourceFile, position);

if (!token) {
return false;
}

// <div>Hello |</div>
if (token.kind === SyntaxKind.LessThanToken && token.parent.kind === SyntaxKind.JsxText) {
Copy link
Member

Choose a reason for hiding this comment

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

Still doesn't make sense to me that the token is considered a child of the JsxText. That should be the child of the closing JSX tag. So I can't imagine how you're hitting this.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, this should be unreachable. Log a bug with a repro if this line gets hit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the case where that happens at marker #1

declare var react: any;

 var x = <div a="/*2*/" b={/*3*/}>
 /*1*/
 </div>;

return true;
}

// <div> { | </div> or <div a={| </div>
if (token.kind === SyntaxKind.LessThanToken && token.parent.kind === SyntaxKind.JsxExpression) {
Copy link
Member

Choose a reason for hiding this comment

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

The comment here doesn't seem to reflect the logic? When does a less-than token have anything to do with the JSX Expression?

return true;
}

// <div> {
// |
// } < /div>
if (token && token.kind === SyntaxKind.CloseBraceToken && token.parent.kind === SyntaxKind.JsxExpression) {
Copy link
Member

Choose a reason for hiding this comment

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

Where's the close brace in this comment?

return true;
}

// <div>|</div>
if (token.kind === SyntaxKind.LessThanToken && token.parent.kind === SyntaxKind.JsxClosingElement) {
return true;
}

return false;
}

export function isInTemplateString(sourceFile: SourceFile, position: number) {
let token = getTokenAtPosition(sourceFile, position);
return isTemplateLiteralKind(token.kind) && position > token.getStart(sourceFile);
}

/**
* Returns true if the cursor at position in sourceFile is within a comment that additionally
* satisfies predicate, and false otherwise.
*/
export function isInCommentHelper(sourceFile: SourceFile, position: number, predicate?: (c: CommentRange) => boolean): boolean {
let token = getTokenAtPosition(sourceFile, position);

if (token && position <= token.getStart()) {
if (token && position <= token.getStart(sourceFile)) {
let commentRanges = getLeadingCommentRanges(sourceFile.text, token.pos);

// The end marker of a single-line comment does not include the newline character.
Expand Down
23 changes: 23 additions & 0 deletions tests/cases/fourslash/commentBraceCompletionPosition.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/// <reference path="fourslash.ts" />

//// /**
//// * inside jsdoc /*1*/
//// */
//// function f() {
//// // inside regular comment /*2*/
//// var c = "";
////
//// /* inside multi-
//// line comment /*3*/
//// */
//// var y =12;
//// }

goTo.marker('1');
verify.not.isValidBraceCompletionAtPostion('(');

goTo.marker('2');
verify.not.isValidBraceCompletionAtPostion('(');

goTo.marker('3');
verify.not.isValidBraceCompletionAtPostion('(');
2 changes: 2 additions & 0 deletions tests/cases/fourslash/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ declare namespace FourSlashInterface {
typeDefinitionCountIs(expectedCount: number): void;
definitionLocationExists(): void;
verifyDefinitionsName(name: string, containerName: string): void;
isValidBraceCompletionAtPostion(openingBrace?: string): void;
}
class verify extends verifyNegatable {
assertHasRanges(ranges: FourSlash.Range[]): void;
Expand Down Expand Up @@ -173,6 +174,7 @@ declare namespace FourSlashInterface {
noMatchingBracePositionInCurrentFile(bracePosition: number): void;
DocCommentTemplate(expectedText: string, expectedOffset: number, empty?: boolean): void;
noDocCommentTemplate(): void;

getScriptLexicalStructureListCount(count: number): void;
getScriptLexicalStructureListContains(name: string, kind: string, fileName?: string, parentName?: string, isAdditionalSpan?: boolean, markerPosition?: number): void;
navigationItemsListCount(count: number, searchValue: string, matchKind?: string): void;
Expand Down
47 changes: 47 additions & 0 deletions tests/cases/fourslash/jsxBraceCompletionPosition.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/// <reference path="fourslash.ts" />

//@Filename: file.tsx
//// declare var React: any;
////
//// var x = <div a="/*2*/" b={/*3*/}>
//// /*1*/
//// </div>;
//// var y = <div>/*4*/</div>
//// var z = <div>
//// hello /*5*/
//// </div>
//// var z2 = <div> { /*6*/
//// </div>
//// var z3 = <div>
//// {
//// /*7*/
//// }
//// </div>

goTo.marker('1');
verify.not.isValidBraceCompletionAtPostion('(');
verify.isValidBraceCompletionAtPostion('{');

goTo.marker('2');
verify.not.isValidBraceCompletionAtPostion('(');
verify.not.isValidBraceCompletionAtPostion('{');

goTo.marker('3');
verify.not.isValidBraceCompletionAtPostion('(');
verify.isValidBraceCompletionAtPostion('{');

goTo.marker('4');
verify.not.isValidBraceCompletionAtPostion('(');
verify.isValidBraceCompletionAtPostion('{');

goTo.marker('5');
verify.not.isValidBraceCompletionAtPostion('(');
verify.isValidBraceCompletionAtPostion('{');

goTo.marker('6');
verify.not.isValidBraceCompletionAtPostion('(');
verify.isValidBraceCompletionAtPostion('{');

goTo.marker('7');
verify.not.isValidBraceCompletionAtPostion('(');
verify.isValidBraceCompletionAtPostion('{');
16 changes: 16 additions & 0 deletions tests/cases/fourslash/stringBraceCompletionPosition.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/// <reference path="fourslash.ts" />

//// var x = "/*1*/";
//// var x = '/*2*/';
//// var x = "hello \
//// /*3*/";

goTo.marker('1');
verify.not.isValidBraceCompletionAtPostion('(');

goTo.marker('2');
verify.not.isValidBraceCompletionAtPostion('(');

goTo.marker('3');
verify.not.isValidBraceCompletionAtPostion('(');