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 3 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
47 changes: 38 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,31 @@ namespace FourSlash {
});
}

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

let charCode = 0x28; // '('
Copy link
Member

Choose a reason for hiding this comment

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

Use 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.

Just make perform the assignment in the switch to make it more uniform

switch (openingBrace) {
case "{": charCode = ts.CharacterCodes.openBrace; break;
case "[": charCode = ts.CharacterCodes.openBracket; break;
case "'": charCode = ts.CharacterCodes.singleQuote; break;
case '"': charCode = ts.CharacterCodes.doubleQuote; break;
case "`": charCode = ts.CharacterCodes.backtick; break;
case "<": charCode = ts.CharacterCodes.lessThan; break;
}
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 +2258,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 +2283,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 +2397,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 +2889,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 +3111,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
37 changes: 37 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,40 @@ 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 (isInJsxAttribute(sourceFile, position)) {
return false;
}

if (isInsideJsxElement(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 +7640,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
31 changes: 31 additions & 0 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,37 @@ namespace ts {
return isInCommentHelper(sourceFile, position, /*predicate*/ undefined);
}

export function isInJsxAttribute(sourceFile: SourceFile, position: number) {
let token = getTokenAtPosition(sourceFile, position);

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.

Wouldn't the parent be a JsxAttribute or JsxSpreadAttribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, turns out it's not

return true;
}

return false;
}

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

// make a distinction if we're dealing with an empty expression or a non-empty expression i.e
// <div>|</div vs <div>Hello | World</div> , where | is current cursor
if (token && (token.kind === SyntaxKind.JsxText ||
(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.

What is the distinction of these situations? Leave a comment explaining each.

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on why you need to do this in a comment (as discussed offline)?

return true;
}

return false;
}

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

Choose a reason for hiding this comment

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

pass sourceFile to getStart

}

/**
* Returns true if the cursor at position in sourceFile is within a comment that additionally
* satisfies predicate, and false otherwise.
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
19 changes: 19 additions & 0 deletions tests/cases/fourslash/jsxBraceCompletionPosition.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/// <reference path="fourslash.ts" />

//@Filename: file.tsx
//// declare var React: any;
////
//// var x = <div a="/*2*/" b={/*3*/}>
//// /*1*/
//// </div>;

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

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

goTo.marker('3');
verify.not.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('(');

16 changes: 16 additions & 0 deletions tests/cases/fourslash/stringTemplateBraceCompletionPosition.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/// <reference path="fourslash.ts" />

//// var x = `/*1*/`;
//// var y = `hello /*2*/world, ${100}how /*3*/are you{ 200 } to/*4*/day!?`

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

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

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

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

//// function parseInt(/*1*/){}
//// class aa/*2*/{
//// public b/*3*/(){}
//// }
//// interface I/*4*/{}
//// var x = /*5*/{ a:true }

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

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

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

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

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