Cross-platform path handling cleanup (#260)

tomv564 · felixfbecker · commit d09c839da84a · 2017-06-02T01:51:30.000+02:00
* exclude resolve support from other path-uri conversion functions

path2uri -&gt; resolvepath2uri
new path2uri and uri2path are absolute and detect windows uris/paths without strict or checking process
add tests from whatwg url conversion

* detect platform from root when resolving to uri

* localfilesystem tests should be platform-ignorant

* Remove os/strict references from util

* Support windows paths in memfs, allow unix path in code fix

* Fix resolution by speaking unix paths to typescript module resolver

* Use valid uris in project manager tests

* Remove old commented out calls

* Return content from overlay in getContent

* Take overlay into account on fileExists
diff --git a/src/fs.ts b/src/fs.ts
@@ -5,7 +5,7 @@ import iterate from 'iterare';
 import { Span } from 'opentracing';
 import Semaphore from 'semaphore-async-await';
 import { InMemoryFileSystem } from './memfs';
-import { normalizeUri, uriToLocalPath } from './util';
+import { normalizeUri, uri2path } from './util';
 
 export interface FileSystem {
 	/**
@@ -58,7 +58,7 @@ export class LocalFileSystem implements FileSystem {
 	 * Converts the URI to an absolute path on the local disk
 	 */
 	protected resolveUriToPath(uri: string): string {
-		return uriToLocalPath(uri);
+		return uri2path(uri);
 	}
 
 	async getWorkspaceFiles(base = this.rootUri): Promise<Iterable<string>> {
diff --git a/src/language-server-stdio.ts b/src/language-server-stdio.ts
@@ -5,7 +5,6 @@ import { MessageEmitter, MessageLogOptions, MessageWriter, registerLanguageHandl
 import { RemoteLanguageClient } from './lang-handler';
 import { FileLogger, StderrLogger } from './logging';
 import { TypeScriptService, TypeScriptServiceOptions } from './typescript-service';
-import * as util from './util';
 
 const packageJson = require('../package.json');
 const program = require('commander');
@@ -17,8 +16,6 @@ program
 	.option('-l, --logfile [file]', 'log to this file')
 	.parse(process.argv);
 
-util.setStrict(program.strict);
-
 const logger = program.logfile ? new FileLogger(program.logfile) : new StderrLogger();
 const options: TypeScriptServiceOptions & MessageLogOptions & RegisterLanguageHandlerOptions = {
 	strict: program.strict,
diff --git a/src/language-server.ts b/src/language-server.ts
@@ -3,7 +3,6 @@
 import { FileLogger, StdioLogger } from './logging';
 import { serve, ServeOptions } from './server';
 import { TypeScriptService, TypeScriptServiceOptions } from './typescript-service';
-import * as util from './util';
 const program = require('commander');
 const packageJson = require('../package.json');
 
@@ -19,8 +18,6 @@ program
 	.option('-l, --logfile [file]', 'log to this file')
 	.parse(process.argv);
 
-util.setStrict(program.strict);
-
 const options: ServeOptions & TypeScriptServiceOptions = {
 	clusterSize: program.cluster || numCPUs,
 	lspPort: program.port || defaultLspPort,
diff --git a/src/memfs.ts b/src/memfs.ts
@@ -76,8 +76,9 @@ export class InMemoryFileSystem extends EventEmitter implements ts.ParseConfigHo
 			this.files.set(uri, content);
 		}
 		// Add to directory tree
+		// TODO: convert this to use URIs.
 		const filePath = uri2path(uri);
-		const components = filePath.split('/').filter(c => c);
+		const components = filePath.split(/[\/\\]/).filter(c => c);
 		let node = this.rootNode;
 		for (const [i, component] of components.entries()) {
 			const n = node.children.get(component);
@@ -109,11 +110,12 @@ export class InMemoryFileSystem extends EventEmitter implements ts.ParseConfigHo
 	 * Returns the file content for the given URI.
 	 * Will throw an Error if no available in-memory.
 	 * Use FileSystemUpdater.ensure() to ensure that the file is available.
-	 *
-	 * TODO take overlay into account
 	 */
 	getContent(uri: string): string {
-		let content = this.files.get(uri);
+		let content = this.overlay.get(uri);
+		if (content === undefined) {
+			content = this.files.get(uri);
+		}
 		if (content === undefined) {
 			content = typeScriptLibraries.get(uri2path(uri));
 		}
@@ -129,7 +131,8 @@ export class InMemoryFileSystem extends EventEmitter implements ts.ParseConfigHo
 	 * @param path File path or URI (both absolute or relative file paths are accepted)
 	 */
 	fileExists(path: string): boolean {
-		return this.readFileIfExists(path) !== undefined || this.files.has(path) || this.files.has(path2uri(this.path, path));
+		const uri = path2uri(path);
+		return this.overlay.has(uri) || this.files.has(uri) || typeScriptLibraries.has(path);
 	}
 
 	/**
@@ -147,7 +150,7 @@ export class InMemoryFileSystem extends EventEmitter implements ts.ParseConfigHo
 	 * If there is no such file, returns undefined
 	 */
 	private readFileIfExists(path: string): string | undefined {
-		const uri = path2uri(this.path, path);
+		const uri = path2uri(path);
 		let content = this.overlay.get(uri);
 		if (content !== undefined) {
 			return content;
diff --git a/src/project-manager.ts b/src/project-manager.ts
@@ -385,7 +385,7 @@ export class ProjectManager implements Disposable {
 				return Observable.merge(
 					// References with `import`
 					Observable.from(info.importedFiles)
-						.map(importedFile => ts.resolveModuleName(toUnixPath(importedFile.fileName), filePath, compilerOpt, config.moduleResolutionHost()))
+						.map(importedFile => ts.resolveModuleName(importedFile.fileName, toUnixPath(filePath), compilerOpt, config.moduleResolutionHost()))
 						// false means we didn't find a file defining the module. It
 						// could still exist as an ambient module, which is why we
 						// fetch global*.d.ts files.
@@ -416,7 +416,7 @@ export class ProjectManager implements Disposable {
 				);
 			})
 			// Use same scheme, slashes, host for referenced URI as input file
-			.map(filePath => path2uri('', filePath))
+			.map(filePath => path2uri(filePath))
 			// Don't cache errors
 			.catch(err => {
 				this.referencedFiles.delete(uri);
@@ -661,14 +661,10 @@ export class InMemoryLanguageServiceHost implements ts.LanguageServiceHost {
 	}
 
 	/**
-	 * @param fileName relative or absolute file path
+	 * @param fileName absolute file path
 	 */
-	getScriptVersion(fileName: string): string {
-
-		const uri = path2uri(this.rootPath, fileName);
-		if (path.posix.isAbsolute(fileName) || path.isAbsolute(fileName)) {
-			fileName = path.posix.relative(this.rootPath, toUnixPath(fileName));
-		}
+	getScriptVersion(filePath: string): string {
+		const uri = path2uri(filePath);
 		let version = this.versions.get(uri);
 		if (!version) {
 			version = 1;
@@ -678,18 +674,14 @@ export class InMemoryLanguageServiceHost implements ts.LanguageServiceHost {
 	}
 
 	/**
-	 * @param fileName relative or absolute file path
+	 * @param filePath absolute file path
 	 */
-	getScriptSnapshot(fileName: string): ts.IScriptSnapshot | undefined {
-		let exists = this.fs.fileExists(fileName);
-		if (!exists) {
-			fileName = path.posix.join(this.rootPath, fileName);
-			exists = this.fs.fileExists(fileName);
-		}
+	getScriptSnapshot(filePath: string): ts.IScriptSnapshot | undefined {
+		const exists = this.fs.fileExists(filePath);
 		if (!exists) {
 			return undefined;
 		}
-		return ts.ScriptSnapshot.fromString(this.fs.readFile(fileName));
+		return ts.ScriptSnapshot.fromString(this.fs.readFile(filePath));
 	}
 
 	getCurrentDirectory(): string {
diff --git a/src/symbols.ts b/src/symbols.ts
@@ -13,7 +13,7 @@ export function locationUri(filePath: string): string {
 	if (isTypeScriptLibrary(filePath)) {
 		return 'git://github.com/Microsoft/TypeScript?v' + ts.version + '#lib/' + filePath.split(/[\/\\]/g).pop();
 	}
-	return path2uri('', filePath);
+	return path2uri(filePath);
 }
 
 /**
diff --git a/src/test/fs.test.ts b/src/test/fs.test.ts
@@ -31,7 +31,7 @@ describe('fs.ts', () => {
 
 			// the project dir
 			const projectDir = path.join(temporaryDir, 'project');
-			rootUri = path2uri('', projectDir) + '/';
+			rootUri = path2uri(projectDir) + '/';
 			await fs.mkdir(projectDir);
 			await fs.mkdir(path.join(projectDir, 'foo'));
 			await fs.mkdir(path.join(projectDir, '@types'));
@@ -42,7 +42,7 @@ describe('fs.ts', () => {
 			await fs.writeFile(path.join(projectDir, 'foo', 'bar.ts'), 'baz');
 			await fs.writeFile(path.join(projectDir, '@types', 'diff', 'index.d.ts'), 'baz');
 
-			// global package is symolinked into project using npm link
+			// global package is symlinked into project using npm link
 			await fs.symlink(somePackageDir, path.join(projectDir, 'node_modules', 'some_package'), 'junction');
 			fileSystem = new LocalFileSystem(rootUri);
 		});
diff --git a/src/test/project-manager.test.ts b/src/test/project-manager.test.ts
@@ -88,7 +88,7 @@ describe('ProjectManager', () => {
 			await projectManager.ensureAllFiles();
 		});
 		it('should resolve best configuration based on file name', () => {
-			const config = projectManager.getParentConfiguration('/src/foo.ts');
+			const config = projectManager.getParentConfiguration('file:///src/foo.ts');
 			assert.isDefined(config);
 			assert.equal('/tsconfig.json', config!.configFilePath);
 		});
@@ -106,7 +106,7 @@ describe('ProjectManager', () => {
 			await projectManager.ensureAllFiles();
 		});
 		it('should resolve best configuration based on file name', () => {
-			const configs = Array.from(projectManager.getChildConfigurations('/foo')).map(config => config.configFilePath);
+			const configs = Array.from(projectManager.getChildConfigurations('file:///foo')).map(config => config.configFilePath);
 			assert.deepEqual(configs, [
 				'/foo/bar/tsconfig.json',
 				'/foo/baz/tsconfig.json'
diff --git a/src/test/typescript-service-helpers.ts b/src/test/typescript-service-helpers.ts
@@ -7,18 +7,14 @@ import { LanguageClient, RemoteLanguageClient } from '../lang-handler';
 import { TextDocumentContentParams, WorkspaceFilesParams } from '../request-type';
 import { SymbolLocationInformation } from '../request-type';
 import { TypeScriptService, TypeScriptServiceFactory } from '../typescript-service';
+import { toUnixPath, uri2path } from '../util';
 import chaiAsPromised = require('chai-as-promised');
 import { apply } from 'json-patch';
 import { ISuiteCallbackContext, ITestCallbackContext } from 'mocha';
+
 chai.use(chaiAsPromised);
 const assert = chai.assert;
 
-/**
- * Enforcing strict mode to make tests pass on Windows
- */
-import { setStrict, uri2path } from '../util';
-setStrict(true);
-
 export interface TestContext {
 
 	/** TypeScript service under test */
@@ -2382,7 +2378,7 @@ export function describeTypeScriptService(createService: TypeScriptServiceFactor
 				title: 'Add \'this.\' to unresolved variable.',
 				command: 'codeFix',
 				arguments: [{
-					fileName: uri2path(rootUri + 'a.ts'),
+					fileName: toUnixPath(uri2path(rootUri + 'a.ts')), // path only used by TS service
 					textChanges: [{
 						span: { start: 49, length: 13 },
 						newText: '\t\tthis.missingThis'
diff --git a/src/test/util-test.ts b/src/test/util-test.ts
@@ -1,5 +1,5 @@
 import * as assert from 'assert';
-import { getMatchingPropertyCount, getPropertyCount, isGlobalTSFile, isSymbolDescriptorMatch, JSONPTR } from '../util';
+import { getMatchingPropertyCount, getPropertyCount, isGlobalTSFile, isSymbolDescriptorMatch, JSONPTR, path2uri, uri2path } from '../util';
 
 describe('util', () => {
 	describe('JSONPTR', () => {
@@ -129,4 +129,47 @@ describe('util', () => {
 			assert.equal(isGlobalTSFile('/node_modules/@types/mocha/index.d.ts'), true);
 		});
 	});
+	describe('path2uri()', () => {
+		it('should throw an error if a non-absolute uri is passed in', () => {
+			assert.throws(() => path2uri('baz/qux'));
+		});
+		it('should convert a Unix file path to a URI', () => {
+			const uri = path2uri('/baz/qux');
+			assert.equal(uri, 'file:///baz/qux');
+		});
+		it('should convert a Windows file path to a URI', () => {
+			const uri = path2uri('C:\\baz\\qux');
+			assert.equal(uri, 'file:///C:/baz/qux');
+		});
+		it('should encode special characters', () => {
+			const uri = path2uri('/💩');
+			assert.equal(uri, 'file:///%F0%9F%92%A9');
+		});
+		it('should encode unreserved special characters', () => {
+			const uri = path2uri('/@baz');
+			assert.equal(uri, 'file:///%40baz');
+		});
+	});
+	describe('uri2path()', () => {
+		it('should convert a Unix file URI to a file path', () => {
+			const filePath = uri2path('file:///baz/qux');
+			assert.equal(filePath, '/baz/qux');
+		});
+		it('should convert a Windows file URI to a file path', () => {
+			const filePath = uri2path('file:///c:/baz/qux');
+			assert.equal(filePath, 'c:\\baz\\qux');
+		});
+		it('should convert a Windows file URI with uppercase drive letter to a file path', () => {
+			const filePath = uri2path('file:///C:/baz/qux');
+			assert.equal(filePath, 'C:\\baz\\qux');
+		});
+		it('should decode special characters', () => {
+			const filePath = uri2path('file:///%F0%9F%92%A9');
+			assert.equal(filePath, '/💩');
+		});
+		it('should decode unreserved special characters', () => {
+			const filePath = uri2path('file:///%40foo');
+			assert.equal(filePath, '/@foo');
+		});
+	});
 });
diff --git a/src/typescript-service.ts b/src/typescript-service.ts
@@ -198,7 +198,7 @@ export class TypeScriptService {
 	initialize(params: InitializeParams, span = new Span()): Observable<OpPatch> {
 		if (params.rootUri || params.rootPath) {
 			this.root = params.rootPath || uri2path(params.rootUri!);
-			this.rootUri = params.rootUri || path2uri('', params.rootPath!);
+			this.rootUri = params.rootUri || path2uri(params.rootPath!);
 			// The root URI always refers to a directory
 			if (!this.rootUri.endsWith('/')) {
 				this.rootUri += '/';
@@ -279,7 +279,7 @@ export class TypeScriptService {
 	 */
 	protected _initializeFileSystems(accessDisk: boolean): void {
 		this.fileSystem = accessDisk ? new LocalFileSystem(this.rootUri) : new RemoteFileSystem(this.client);
-		this.inMemoryFileSystem = new InMemoryFileSystem(this.root);
+		this.inMemoryFileSystem = new InMemoryFileSystem(this.root, this.logger);
 	}
 
 	/**
@@ -613,7 +613,7 @@ export class TypeScriptService {
 						const start = ts.getLineAndCharacterOfPosition(sourceFile, reference.textSpan.start);
 						const end = ts.getLineAndCharacterOfPosition(sourceFile, reference.textSpan.start + reference.textSpan.length);
 						return {
-							uri: path2uri(this.root, reference.fileName),
+							uri: path2uri(reference.fileName),
 							range: {
 								start,
 								end
@@ -806,7 +806,7 @@ export class TypeScriptService {
 											}
 											// If SymbolDescriptor matched and the query contains a PackageDescriptor, get package.json and match PackageDescriptor name
 											// TODO match full PackageDescriptor (version)
-											const uri = path2uri('', definition.fileName);
+											const uri = path2uri(definition.fileName);
 											return this._getPackageDescriptor(uri)
 												.mergeMap(packageDescriptor => {
 													symbol.package = packageDescriptor;
@@ -1150,7 +1150,7 @@ export class TypeScriptService {
 					if (!sourceFile) {
 						throw new Error(`Expected source file ${change.fileName} to exist in configuration`);
 					}
-					const uri = path2uri(this.root, change.fileName);
+					const uri = path2uri(change.fileName);
 					changes[uri] = change.textChanges.map(({ span, newText }): TextEdit => ({
 						range: {
 							start: ts.getLineAndCharacterOfPosition(sourceFile, span.start),
@@ -1201,7 +1201,7 @@ export class TypeScriptService {
 						if (!sourceFile) {
 							throw new Error(`expected source file ${location.fileName} to exist in configuration`);
 						}
-						const editUri = path2uri(this.root, location.fileName);
+						const editUri = path2uri(location.fileName);
 						const start = ts.getLineAndCharacterOfPosition(sourceFile, location.textSpan.start);
 						const end = ts.getLineAndCharacterOfPosition(sourceFile, location.textSpan.start + location.textSpan.length);
 						const edit: TextEdit = { range: { start, end }, newText: params.newName };
@@ -1398,7 +1398,7 @@ export class TypeScriptService {
 											if (!query.package || !query.package.name) {
 												return [{ score, tree, parent }];
 											}
-											const uri = path2uri('', sourceFile.fileName);
+											const uri = path2uri(sourceFile.fileName);
 											return Observable.from(this.packageManager.getClosestPackageJson(uri, span))
 												// If PackageDescriptor matches, increase score
 												.map(packageJson => {
diff --git a/src/util.ts b/src/util.ts

Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ export function locationUri(filePath: string): string {`
`13`	`13`	`if (isTypeScriptLibrary(filePath)) {`
`14`	`14`	`return 'git://github.com/Microsoft/TypeScript?v' + ts.version + '#lib/' + filePath.split(/[\/\\]/g).pop();`
`15`	`15`	`}`
`16`		`- return path2uri('', filePath);`
	`16`	`+ return path2uri(filePath);`
`17`	`17`	`}`
`18`	`18`
`19`	`19`	`/**`