-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Added DefinitionAndBoundSpan command #19175
Changes from 5 commits
1cb2d24
c6a8a32
b86153d
8004fec
16c3255
051da11
f8ccde5
ae266f6
7f577dc
c62f288
7fd9fe6
5b7abf2
f3059ce
c4a675e
d5c18a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -584,18 +584,23 @@ namespace FourSlash { | |
|
||
public verifyGoToDefinition(arg0: any, endMarkerNames?: string | string[]) { | ||
this.verifyGoToX(arg0, endMarkerNames, () => this.getGoToDefinition()); | ||
this.verifyGoToX(arg0, endMarkerNames, () => this.getGoToDefinitionAndBoundSpan()); | ||
} | ||
|
||
private getGoToDefinition(): ts.DefinitionInfo[] { | ||
return this.languageService.getDefinitionAtPosition(this.activeFile.fileName, this.currentCaretPosition); | ||
} | ||
|
||
private getGoToDefinitionAndBoundSpan(): ts.DefinitionInfoAndBoundSpan { | ||
return this.languageService.getDefinitionAndBoundSpan(this.activeFile.fileName, this.currentCaretPosition); | ||
} | ||
|
||
public verifyGoToType(arg0: any, endMarkerNames?: string | string[]) { | ||
this.verifyGoToX(arg0, endMarkerNames, () => | ||
this.languageService.getTypeDefinitionAtPosition(this.activeFile.fileName, this.currentCaretPosition)); | ||
} | ||
|
||
private verifyGoToX(arg0: any, endMarkerNames: string | string[] | undefined, getDefs: () => ts.DefinitionInfo[] | undefined) { | ||
private verifyGoToX(arg0: any, endMarkerNames: string | string[] | undefined, getDefs: () => ts.DefinitionInfo[] | ts.DefinitionInfoAndBoundSpan | undefined) { | ||
if (endMarkerNames) { | ||
this.verifyGoToXPlain(arg0, endMarkerNames, getDefs); | ||
} | ||
|
@@ -615,7 +620,7 @@ namespace FourSlash { | |
} | ||
} | ||
|
||
private verifyGoToXPlain(startMarkerNames: string | string[], endMarkerNames: string | string[], getDefs: () => ts.DefinitionInfo[] | undefined) { | ||
private verifyGoToXPlain(startMarkerNames: string | string[], endMarkerNames: string | string[], getDefs: () => ts.DefinitionInfo[] | ts.DefinitionInfoAndBoundSpan | undefined) { | ||
for (const start of toArray(startMarkerNames)) { | ||
this.verifyGoToXSingle(start, endMarkerNames, getDefs); | ||
} | ||
|
@@ -627,26 +632,60 @@ namespace FourSlash { | |
} | ||
} | ||
|
||
private verifyGoToXSingle(startMarkerName: string, endMarkerNames: string | string[], getDefs: () => ts.DefinitionInfo[] | undefined) { | ||
private verifyGoToXSingle(startMarkerName: string, endMarkerNames: string | string[], getDefs: () => ts.DefinitionInfo[] | ts.DefinitionInfoAndBoundSpan | undefined) { | ||
this.goToMarker(startMarkerName); | ||
this.verifyGoToXWorker(toArray(endMarkerNames), getDefs); | ||
this.verifyGoToXWorker(toArray(endMarkerNames), getDefs, startMarkerName); | ||
} | ||
|
||
private verifyGoToXWorker(endMarkers: string[], getDefs: () => ts.DefinitionInfo[] | undefined) { | ||
const definitions = getDefs() || []; | ||
private verifyGoToXWorker(endMarkers: string[], getDefs: () => ts.DefinitionInfo[] | ts.DefinitionInfoAndBoundSpan | undefined, startMarkerName?: string) { | ||
const defs = getDefs(); | ||
let definitions: ts.DefinitionInfo[] | ReadonlyArray<ts.DefinitionInfo>; | ||
let testName: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It seems like this is actually the name of the API under test. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which one is reported when the argument is In reply to: 146340127 [](ancestors = 146340127) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The argument is not going to be undefined only the response from the argument function. It happens when the token doesn't contain a definition. |
||
if (this.isDefinitionInfoAndBoundSpan(defs)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be easier to just check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be the same IMO, if anything maybe a little bit harder to read. First, we need to compare against undefined as well making the comparison a little bit bigger. Second, we'll need to use a type assertion instead of a type guard. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, |
||
this.verifyDefinitionTextSpan(defs, startMarkerName); | ||
|
||
definitions = defs.definitions; | ||
testName = "goToDefinitionsAndBoundSpan"; | ||
} | ||
else { | ||
definitions = defs || []; | ||
testName = "goToDefinitions"; | ||
} | ||
|
||
if (endMarkers.length !== definitions.length) { | ||
this.raiseError(`goToDefinitions failed - expected to find ${endMarkers.length} definitions but got ${definitions.length}`); | ||
this.raiseError(`${testName} failed - expected to find ${endMarkers.length} definitions but got ${definitions.length}`); | ||
} | ||
|
||
ts.zipWith(endMarkers, definitions, (endMarker, definition, i) => { | ||
const marker = this.getMarkerByName(endMarker); | ||
if (marker.fileName !== definition.fileName || marker.position !== definition.textSpan.start) { | ||
this.raiseError(`goToDefinition failed for definition ${endMarker} (${i}): expected ${marker.fileName} at ${marker.position}, got ${definition.fileName} at ${definition.textSpan.start}`); | ||
this.raiseError(`${testName} failed for definition ${endMarker} (${i}): expected ${marker.fileName} at ${marker.position}, got ${definition.fileName} at ${definition.textSpan.start}`); | ||
} | ||
}); | ||
} | ||
|
||
private verifyDefinitionTextSpan(defs: ts.DefinitionInfoAndBoundSpan, startMarkerName: string) { | ||
const range = this.testData.ranges.find(range => this.markerName(range.marker) === startMarkerName); | ||
|
||
if (!range && !defs.textSpan) { | ||
return; | ||
} | ||
|
||
if (!range) { | ||
this.raiseError(`goToDefinitionsAndBoundSpan failed - found a TextSpan ${JSON.stringify(defs.textSpan)} when it wasn't expected.`); | ||
} | ||
else if (defs.textSpan.start !== range.start || defs.textSpan.length !== range.end - range.start) { | ||
const expected: ts.TextSpan = { | ||
start: range.start, length: range.end - range.start | ||
}; | ||
this.raiseError(`goToDefinitionsAndBoundSpan failed - expected to find TextSpan ${JSON.stringify(expected)} but got ${JSON.stringify(defs.textSpan)}`); | ||
} | ||
} | ||
|
||
private isDefinitionInfoAndBoundSpan(definition: ts.DefinitionInfo[] | ts.DefinitionInfoAndBoundSpan | undefined): definition is ts.DefinitionInfoAndBoundSpan { | ||
return definition && (<ts.DefinitionInfoAndBoundSpan>definition).definitions !== undefined; | ||
} | ||
|
||
public verifyGetEmitOutputForCurrentFile(expected: string): void { | ||
const emit = this.languageService.getEmitOutput(this.activeFile.fileName); | ||
if (emit.outputFiles.length !== 1) { | ||
|
@@ -3828,6 +3867,7 @@ namespace FourSlashInterface { | |
} | ||
|
||
public goToDefinition(startMarkerName: string | string[], endMarkerName: string | string[]): void; | ||
public goToDefinition(startMarkerName: string | string[], endMarkerName: string | string[], range: FourSlash.Range): void; | ||
public goToDefinition(startsAndEnds: [string | string[], string | string[]][]): void; | ||
public goToDefinition(startsAndEnds: { [startMarkerName: string]: string | string[] }): void; | ||
public goToDefinition(arg0: any, endMarkerName?: string | string[]) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,7 +117,7 @@ namespace ts.server { | |
body: undefined | ||
}); | ||
}); | ||
it ("should handle literal types in request", () => { | ||
it("should handle literal types in request", () => { | ||
const configureRequest: protocol.ConfigureRequest = { | ||
command: CommandNames.Configure, | ||
seq: 0, | ||
|
@@ -175,6 +175,8 @@ namespace ts.server { | |
CommandNames.Configure, | ||
CommandNames.Definition, | ||
CommandNames.DefinitionFull, | ||
CommandNames.DefinitionAndBoundSpan, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I notice that the regular There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Offline, I think we established that VS Code will probably not consume the new API. If this API is just for VS, should it have "Full" in the name? I think that's how we usually distinguish. In reply to: 144675978 [](ancestors = 144675978) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on a discussion with @mhegazy, I have added the method for the simplified, hence, the "full" has been updated. |
||
CommandNames.DefinitionAndBoundSpanFull, | ||
CommandNames.Implementation, | ||
CommandNames.ImplementationFull, | ||
CommandNames.Exit, | ||
|
@@ -341,7 +343,7 @@ namespace ts.server { | |
session.addProtocolHandler(command, () => resp); | ||
|
||
expect(() => session.addProtocolHandler(command, () => resp)) | ||
.to.throw(`Protocol handler already exists for command "${command}"`); | ||
.to.throw(`Protocol handler already exists for command "${command}"`); | ||
}); | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -167,7 +167,7 @@ namespace ts.server { | |
private timerHandle: any; | ||
private immediateId: number | undefined; | ||
|
||
constructor(private readonly operationHost: MultistepOperationHost) {} | ||
constructor(private readonly operationHost: MultistepOperationHost) { } | ||
|
||
public startNew(action: (next: NextStep) => void) { | ||
this.complete(); | ||
|
@@ -579,7 +579,7 @@ namespace ts.server { | |
|
||
private getDiagnosticsWorker( | ||
args: protocol.FileRequestArgs, isSemantic: boolean, selector: (project: Project, file: string) => ReadonlyArray<Diagnostic>, includeLinePosition: boolean | ||
): ReadonlyArray<protocol.DiagnosticWithLinePosition> | ReadonlyArray<protocol.Diagnostic> { | ||
): ReadonlyArray<protocol.DiagnosticWithLinePosition> | ReadonlyArray<protocol.Diagnostic> { | ||
const { project, file } = this.getFileAndProject(args); | ||
if (isSemantic && isDeclarationFileInJSOnlyNonConfiguredProject(project, file)) { | ||
return emptyArray; | ||
|
@@ -601,20 +601,58 @@ namespace ts.server { | |
} | ||
|
||
if (simplifiedResult) { | ||
return definitions.map(def => { | ||
const defScriptInfo = project.getScriptInfo(def.fileName); | ||
return { | ||
file: def.fileName, | ||
start: defScriptInfo.positionToLineOffset(def.textSpan.start), | ||
end: defScriptInfo.positionToLineOffset(textSpanEnd(def.textSpan)) | ||
}; | ||
}); | ||
return this.getSimplifiedDefinitions(definitions, project); | ||
} | ||
else { | ||
return definitions; | ||
} | ||
} | ||
|
||
private getDefinitionAndBoundSpan(args: protocol.FileLocationRequestArgs, simplifiedResult: boolean): protocol.DefinitionInfoAndBoundSpan | DefinitionInfoAndBoundSpan { | ||
const { file, project } = this.getFileAndProject(args); | ||
const position = this.getPositionInFile(args, file); | ||
const scriptInfo = project.getScriptInfo(file); | ||
|
||
const definitionAndBoundSpan = project.getLanguageService().getDefinitionAndBoundSpan(file, position); | ||
|
||
if (!definitionAndBoundSpan || !definitionAndBoundSpan.definitions) { | ||
return { | ||
definitions: emptyArray, | ||
textSpan: undefined | ||
}; | ||
} | ||
|
||
if (simplifiedResult) { | ||
return { | ||
definitions: this.getSimplifiedDefinitions(definitionAndBoundSpan.definitions, project), | ||
textSpan: this.getSimplifiedTextSpan(scriptInfo, definitionAndBoundSpan.textSpan) | ||
}; | ||
} | ||
|
||
return definitionAndBoundSpan; | ||
} | ||
|
||
private getSimplifiedDefinitions(definitions: ReadonlyArray<DefinitionInfo>, project: Project): ReadonlyArray<protocol.FileSpan> { | ||
return definitions.map(def => this.getSimplifiedFileSpan(def.fileName, def.textSpan, project)); | ||
} | ||
|
||
private getSimplifiedFileSpan(fileName: string, textSpan: TextSpan, project: Project): protocol.FileSpan { | ||
const scriptInfo = project.getScriptInfo(fileName); | ||
const simplifiedTextSpan = this.getSimplifiedTextSpan(scriptInfo, textSpan); | ||
|
||
return { | ||
file: fileName, | ||
...simplifiedTextSpan | ||
}; | ||
} | ||
|
||
private getSimplifiedTextSpan(scriptInfo: ScriptInfo, textSpan: TextSpan): protocol.TextSpan { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Should this be called in more places? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, there must be some other places where a "simplified textSpan" might be required and this method could be reused. I didn't check for that thou. In any case, this was refactored to reuse in two different places. line 628 and line 641. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored for reusability. Also based on a conversation with @mhegazy, changed a couple of method names. |
||
return { | ||
start: scriptInfo.positionToLineOffset(textSpan.start), | ||
end: scriptInfo.positionToLineOffset(textSpanEnd(textSpan)) | ||
}; | ||
} | ||
|
||
private getTypeDefinition(args: protocol.FileLocationRequestArgs): ReadonlyArray<protocol.FileSpan> { | ||
const { file, project } = this.getFileAndProject(args); | ||
const position = this.getPositionInFile(args, file); | ||
|
@@ -1707,6 +1745,12 @@ namespace ts.server { | |
[CommandNames.DefinitionFull]: (request: protocol.DefinitionRequest) => { | ||
return this.requiredResponse(this.getDefinition(request.arguments, /*simplifiedResult*/ false)); | ||
}, | ||
[CommandNames.DefinitionAndBoundSpan]: (request: protocol.DefinitionRequest) => { | ||
return this.requiredResponse(this.getDefinitionAndBoundSpan(request.arguments, /*simplifiedResult*/ true)); | ||
}, | ||
[CommandNames.DefinitionAndBoundSpanFull]: (request: protocol.DefinitionRequest) => { | ||
return this.requiredResponse(this.getDefinitionAndBoundSpan(request.arguments, /*simplifiedResult*/ false)); | ||
}, | ||
[CommandNames.TypeDefinition]: (request: protocol.FileLocationRequest) => { | ||
return this.requiredResponse(this.getTypeDefinition(request.arguments)); | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,7 +88,7 @@ namespace ts.GoToDefinition { | |
// } | ||
// bar<Test>(({pr/*goto*/op1})=>{}); | ||
if (isPropertyName(node) && isBindingElement(node.parent) && isObjectBindingPattern(node.parent.parent) && | ||
(node === (node.parent.propertyName || node.parent.name))) { | ||
(node === (node.parent.propertyName || node.parent.name))) { | ||
const type = typeChecker.getTypeAtLocation(node.parent.parent); | ||
if (type) { | ||
const propSymbols = getPropertySymbolsFromType(type, node); | ||
|
@@ -149,6 +149,25 @@ namespace ts.GoToDefinition { | |
return getDefinitionFromSymbol(typeChecker, type.symbol, node); | ||
} | ||
|
||
export function getDefinitionAndBoundSpan(program: Program, sourceFile: SourceFile, position: number): DefinitionInfoAndBoundSpan { | ||
const definitions = getDefinitionAtPosition(program, sourceFile, position); | ||
|
||
if (!definitions || definitions.length === 0) { | ||
return undefined; | ||
} | ||
|
||
// TODO: Add textSpan for triple slash references (file and type). | ||
const comment = findReferenceInPosition(sourceFile.referencedFiles, position); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this should be: const comment = findReferenceInPosition(sourceFile.referencedFiles, position) || findReferenceInPosition(sourceFile.typeReferenceDirectives, position); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed. |
||
if (comment && tryResolveScriptReference(program, sourceFile, comment) || findReferenceInPosition(sourceFile.typeReferenceDirectives, position)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do not think you need to call tryResolveScriptReference here. just make sure definitions is not empty, which you already checked earlier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
return { definitions, textSpan: undefined }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would make the textSpan the full line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code has changed so this is no longer valid. The new one has it full line. |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this block do? Is it for ctrl-click on the file path in a triple-slash reference? Is the TODO still relevant? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This block checks if the position we're searching is within a comment and if that comment is a script reference (triple slash). If true we don't search for a textSpan as the method for that involves a comment parser and complicates a little bit more the process, so undefined is returned instead for now. The TODO is relevant because of this. Ctrl + Click is going to be enabled with definitions but no textSpan for underlining it is implemented yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added bug #19447 for tracking this. |
||
|
||
const node = getTouchingPropertyName(sourceFile, position, /*includeJsDocComment*/ true); | ||
const textSpan = createTextSpan(node.getStart(), node.getWidth()); | ||
|
||
return { definitions, textSpan }; | ||
} | ||
|
||
// Go to the original declaration for cases: | ||
// | ||
// (1) when the aliased symbol was declared in the location(parent). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why we need two calls here.
getGoToDefinitionAndBoundSpan
subsumes the other one, so would be sufficient to do here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would say always use
getGoToDefinitionAndBoundSpan
and check the span only if the marker name was provided.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed as suggested.