Remove read file contents request sent to extension by language serve…

…r (use is file check) (#3198)

extension/src/fileSystem/index.ts

@@ -22,14 +22,19 @@ import { delay } from '../util/time'
 
 export const exists = (path: string): boolean => existsSync(path)
 
-export const isDirectory = (path: string): boolean => {
+const checkStats = (path: string, check: 'isDirectory' | 'isFile'): boolean => {
   try {
-    return lstatSync(path).isDirectory()
+    return lstatSync(path)[check]()
   } catch {
     return false
   }
 }
 
+export const isDirectory = (path: string): boolean =>
+  checkStats(path, 'isDirectory')
+
+export const isFile = (path: string): boolean => checkStats(path, 'isFile')
+
 export const getModifiedTime = (path: string): number =>
   lstatSync(path).mtime.getTime()
 
@@ -75,7 +80,7 @@ export const findAbsoluteDvcRootPath = async (
 const findDotGitDir = (gitRoot: string) => {
   const dotGitPath = join(gitRoot, gitPath.DOT_GIT)
 
-  const isSubmodule = lstatSync(dotGitPath).isFile()
+  const isSubmodule = isFile(dotGitPath)
   if (isSubmodule) {
     const dotGitAsFileContent = readFileSync(dotGitPath, 'utf8')
     const gitDirPrefix = 'gitdir: '
@@ -204,15 +209,3 @@ export const getBinDisplayText = (
     ? '.' + sep + relative(workspaceRoot, path)
     : path
 }
-
-export const readFileContents = (
-  uriString: string
-): { contents: string } | null => {
-  try {
-    const uri = Uri.parse(uriString)
-    if (!isDirectory(uri.fsPath)) {
-      return { contents: readFileSync(uri.fsPath, 'utf8') }
-    }
-  } catch {}
-  return null
-}

extension/src/languageClient/index.ts

@@ -1,4 +1,4 @@
-import { workspace } from 'vscode'
+import { Uri, workspace } from 'vscode'
 import {
   LanguageClient as Client,
   LanguageClientOptions,
@@ -7,7 +7,7 @@ import {
 } from 'vscode-languageclient/node'
 import { documentSelector, serverModule } from 'dvc-vscode-lsp'
 import { Disposable } from '../class/dispose'
-import { readFileContents } from '../fileSystem'
+import { isFile } from '../fileSystem'
 
 export class LanguageClient extends Disposable {
   private client: Client
@@ -32,7 +32,14 @@ export class LanguageClient extends Disposable {
     )
 
     this.dispose.track(
-      this.client.onRequest('readFileContents', readFileContents)
+      this.client.onRequest('isFile', (uri: string) => {
+        try {
+          const path = Uri.parse(uri).fsPath
+          return { isFile: isFile(path) }
+        } catch {
+          return null
+        }
+      })
     )
 
     void this.client.start()

extension/src/test/suite/fileSystem/index.test.ts

@@ -1,6 +1,5 @@
 import { join, resolve } from 'path'
 import process from 'process'
-import { Uri } from 'vscode'
 import { afterEach, beforeEach, describe, it, suite } from 'mocha'
 import { expect } from 'chai'
 import { Disposable } from '@hediet/std/disposable'
@@ -11,7 +10,8 @@ import {
   checkSignalFile,
   exists,
   getGitPath,
-  readFileContents
+  isDirectory,
+  isFile
 } from '../../../fileSystem'
 import { gitPath } from '../../../cli/git/constants'
 import { GitReader } from '../../../cli/git/reader'
@@ -100,19 +100,28 @@ suite('File System Test Suite', () => {
     })
   })
 
-  describe('readFileContents', () => {
-    it('should read the contents of a file when it exists', () => {
-      const uriString = Uri.file(join(dvcDemoPath, 'train.py')).toString()
+  describe('isFile', () => {
+    it('should return true when the path is a file', () => {
+      const path = standardizePath(join(dvcDemoPath, 'train.py'))
 
-      const file = readFileContents(uriString)
-      expect(file?.contents).to.contain('main()')
+      const result = isFile(path)
+      expect(result).to.be.true
     })
 
-    it('should return null when the file cannot be found', () => {
-      const uriString = 'file:///some/fun/file.txt'
+    it('should return false when the file cannot be found', () => {
+      const path = standardizePath(join('some', 'fun', 'file.txt'))
 
-      const contents = readFileContents(uriString)
-      expect(contents).to.be.null
+      const result = isFile(path)
+      expect(result).to.be.false
+    })
+
+    it('should return false for a directory', () => {
+      const path = standardizePath(join(dvcDemoPath, 'training'))
+
+      expect(isDirectory(path)).to.be.true
+
+      const result = isFile(path)
+      expect(result).to.be.false
     })
   })
 })

languageServer/src/languageServer.ts

@@ -15,11 +15,7 @@ import {
 } from 'vscode-languageserver/node'
 import { TextDocument } from 'vscode-languageserver-textdocument'
 import { URI } from 'vscode-uri'
-import {
-  getTextDocumentLocation,
-  getUriLocation,
-  symbolAt
-} from './textDocument'
+import { getLocationToOpen, symbolAt } from './textDocument'
 
 export class LanguageServer {
   private documentsKnownToEditor!: TextDocuments<TextDocument>
@@ -58,7 +54,7 @@ export class LanguageServer {
       .all()
       .filter(doc => URI.parse(doc.uri).fsPath.endsWith(filePath))
 
-    return matchingFiles.map(doc => getTextDocumentLocation(doc))
+    return matchingFiles.map(doc => getLocationToOpen(doc.uri))
   }
 
   private async onDefinition(params: DefinitionParams, connection: Connection) {
@@ -98,11 +94,11 @@ export class LanguageServer {
       const possiblePath = URI.parse(
         [dirname(document.uri), possibleFile].join('/')
       ).toString()
-      const file = await connection.sendRequest<{
-        contents: string
-      } | null>('readFileContents', possiblePath)
-      if (file) {
-        const location = getUriLocation(possiblePath, file.contents)
+      const result = await connection.sendRequest<{
+        isFile: boolean
+      } | null>('isFile', possiblePath)
+      if (result?.isFile) {
+        const location = getLocationToOpen(possiblePath)
         fileLocations.push(location)
       }
     }

languageServer/src/test/definitions.test.ts

@@ -7,7 +7,6 @@ import {
   train_dvc_yaml
 } from './fixtures/examples/valid'
 import { params } from './fixtures/params'
-import { train } from './fixtures/python'
 import { requestDefinitions } from './utils/requestDefinitions'
 import { openTheseFilesAndNotifyServer } from './utils/openTheseFilesAndNotifyServer'
 import {
@@ -58,7 +57,7 @@ describe('textDocument/definitions', () => {
 
     expect(response).toBeTruthy()
     expect(response).toStrictEqual({
-      range: Range.create(Position.create(0, 0), Position.create(5, 9)),
+      range: Range.create(Position.create(0, 0), Position.create(0, 0)),
       uri: 'file:///params.yaml'
     })
   })
@@ -68,7 +67,7 @@ describe('textDocument/definitions', () => {
 
     mockedReadFileContents.mockImplementation(path => {
       if (path === trainUri) {
-        return { contents: train }
+        return { isFile: true }
       }
       return null
     })
@@ -89,7 +88,7 @@ describe('textDocument/definitions', () => {
 
     expect(response).toBeTruthy()
     expect(response).toStrictEqual({
-      range: Range.create(Position.create(0, 0), Position.create(7, 13)),
+      range: Range.create(Position.create(0, 0), Position.create(0, 0)),
       uri: trainUri
     })
   })

languageServer/src/test/utils/setup-test-connections.ts

@@ -24,7 +24,7 @@ export const setupTestConnections = (
 
   server = createConnection(up, down)
   client = createConnection(down, up)
-  client.onRequest('readFileContents', mockedReadFileContents)
+  client.onRequest('isFile', mockedReadFileContents)
 
   dvcLanguageService.listen(server)
   client.listen()

languageServer/src/textDocument.ts

@@ -17,19 +17,11 @@ import {
   visit
 } from 'yaml'
 
-export const getTextDocumentLocation = (
-  textDocument: TextDocument
-): Location => {
-  const start = Position.create(0, 0)
-  const end = textDocument.positionAt(textDocument.getText().length - 1)
-  const range = Range.create(start, end)
-
-  return Location.create(textDocument.uri, range)
-}
+export const getLocationToOpen = (uri: string): Location => {
+  const position = Position.create(0, 0)
+  const range = Range.create(position, position)
 
-export const getUriLocation = (uri: string, content: string): Location => {
-  const textDocument = TextDocument.create(uri, 'plain/text', 0, content)
-  return getTextDocumentLocation(textDocument)
+  return Location.create(uri, range)
 }
 
 const yamlScalarNodeToDocumentSymbols = (