polish, extract util functions, fix issue with overeager hex detection

src/vs/base/common/strings.ts

@@ -338,6 +338,34 @@ export function compareIgnoreCase(a: string, b: string): number {
 	}
 }
 
+/**
+ * [0-9]
+ */
+export function isAsciiDigit(code: number): boolean {
+	return code >= CharCode.Digit0 && code <= CharCode.Digit9;
+}
+
+/**
+ * [a-f]
+ */
+export function isLowerAsciiHex(code: number): boolean {
+	return code >= CharCode.a && code <= CharCode.f;
+}
+
+/**
+ * [A-F]
+ */
+export function isUpperAsciiHex(code: number): boolean {
+	return code >= CharCode.A && code <= CharCode.F;
+}
+
+/**
+ * [0-9a-fA-F]
+ */
+export function isAsciiHex(code: number): boolean {
+	return isAsciiDigit(code) || isLowerAsciiHex(code) || isUpperAsciiHex(code);
+}
+
 export function isLowerAsciiLetter(code: number): boolean {
 	return code >= CharCode.a && code <= CharCode.z;
 }
@@ -346,7 +374,7 @@ export function isUpperAsciiLetter(code: number): boolean {
 	return code >= CharCode.A && code <= CharCode.Z;
 }
 
-function isAsciiLetter(code: number): boolean {
+export function isAsciiLetter(code: number): boolean {
 	return isLowerAsciiLetter(code) || isUpperAsciiLetter(code);
 }
 

src/vs/base/common/uri.ts

@@ -5,7 +5,7 @@
 
 import { isWindows } from 'vs/base/common/platform';
 import { CharCode } from 'vs/base/common/charCode';
-import { isHighSurrogate, isLowSurrogate } from 'vs/base/common/strings';
+import { isHighSurrogate, isLowSurrogate, isLowerAsciiHex, isAsciiLetter } from 'vs/base/common/strings';
 
 const _schemeRegExp = /^\w[\w\d+.-]*$/;
 
@@ -207,10 +207,7 @@ export class URI implements UriComponents {
 	 * with URIs that represent files on disk (`file` scheme).
 	 */
 	get fsPath(): string {
-		// if (this.scheme !== 'file') {
-		// 	console.warn(`[UriError] calling fsPath with scheme ${this.scheme}`);
-		// }
-		return _makeFsPath(this);
+		return _toFsPath(this.scheme, this.authority, this.path);
 	}
 
 	// ---- modify to new -------------------------
@@ -287,7 +284,7 @@ export class URI implements UriComponents {
 			percentDecode(query),
 			percentDecode(fragment),
 		);
-		result._formatted = _toString(_normalEncoder, scheme, authority, path, query, fragment);
+		result._external = _toExternal(_normalEncoder, scheme, authority, path, query, fragment);
 		return result;
 	}
 
@@ -375,7 +372,7 @@ export class URI implements UriComponents {
 	 * @param skipEncoding Do not encode the result, default is `false`
 	 */
 	toString(skipEncoding: boolean = false): string {
-		return _toString(skipEncoding ? _minimalEncoder : _normalEncoder, this.scheme, this.authority, this.path, this.query, this.fragment);
+		return _toExternal(skipEncoding ? _minimalEncoder : _normalEncoder, this.scheme, this.authority, this.path, this.query, this.fragment);
 	}
 
 	toJSON(): UriComponents {
@@ -393,7 +390,7 @@ export class URI implements UriComponents {
 			return data;
 		} else {
 			const result = new _URI(data);
-			result._formatted = (<UriState>data).external;
+			result._external = (<UriState>data).external;
 			result._fsPath = (<UriState>data)._sep === _pathSepMarker ? (<UriState>data).fsPath : null;
 			return result;
 		}
@@ -420,12 +417,12 @@ const _pathSepMarker = isWindows ? 1 : undefined;
 // tslint:disable-next-line:class-name
 class _URI extends URI {
 
-	_formatted: string | null = null;
+	_external: string | null = null;
 	_fsPath: string | null = null;
 
 	get fsPath(): string {
 		if (!this._fsPath) {
-			this._fsPath = _makeFsPath(this);
+			this._fsPath = _toFsPath(this.scheme, this.authority, this.path);
 		}
 		return this._fsPath;
 	}
@@ -433,12 +430,12 @@ class _URI extends URI {
 	toString(skipEncoding: boolean = false): string {
 		if (skipEncoding) {
 			// we don't cache that
-			return _toString(_minimalEncoder, this.scheme, this.authority, this.path, this.query, this.fragment);
+			return _toExternal(_minimalEncoder, this.scheme, this.authority, this.path, this.query, this.fragment);
 		}
-		if (!this._formatted) {
-			this._formatted = _toString(_normalEncoder, this.scheme, this.authority, this.path, this.query, this.fragment);
+		if (!this._external) {
+			this._external = _toExternal(_normalEncoder, this.scheme, this.authority, this.path, this.query, this.fragment);
 		}
-		return this._formatted;
+		return this._external;
 	}
 
 	toJSON(): UriComponents {
@@ -450,8 +447,8 @@ class _URI extends URI {
 			res.fsPath = this._fsPath;
 			res._sep = _pathSepMarker;
 		}
-		if (this._formatted) {
-			res.external = this._formatted;
+		if (this._external) {
+			res.external = this._external;
 		}
 		// uri components
 		if (this.path) {
@@ -476,22 +473,22 @@ class _URI extends URI {
 /**
  * Compute `fsPath` for the given uri
  */
-function _makeFsPath(uri: URI): string {
+function _toFsPath(scheme: string, authority: string, path: string): string {
 
 	let value: string;
-	if (uri.authority && uri.path.length > 1 && uri.scheme === 'file') {
+	if (authority && path.length > 1 && scheme === 'file') {
 		// unc path: file://shares/c$/far/boo
-		value = `//${uri.authority}${uri.path}`;
+		value = `//${authority}${path}`;
 	} else if (
-		uri.path.charCodeAt(0) === CharCode.Slash
-		&& (uri.path.charCodeAt(1) >= CharCode.A && uri.path.charCodeAt(1) <= CharCode.Z || uri.path.charCodeAt(1) >= CharCode.a && uri.path.charCodeAt(1) <= CharCode.z)
-		&& uri.path.charCodeAt(2) === CharCode.Colon
+		path.charCodeAt(0) === CharCode.Slash
+		&& isAsciiLetter(path.charCodeAt(1))
+		&& path.charCodeAt(2) === CharCode.Colon
 	) {
 		// windows drive letter: file:///c:/far/boo
-		value = uri.path[1].toLowerCase() + uri.path.substr(2);
+		value = path[1].toLowerCase() + path.substr(2);
 	} else {
 		// other path
-		value = uri.path;
+		value = path;
 	}
 	if (isWindows) {
 		value = value.replace(_slashRegExp, '\\');
@@ -562,11 +559,6 @@ function isHashOrQuestionMark(code: number): boolean {
 	return code === CharCode.Hash || code === CharCode.QuestionMark;
 }
 
-function isLowerAsciiHex(code: number): boolean {
-	return code >= CharCode.Digit0 && code <= CharCode.Digit9
-		|| code >= CharCode.a && code <= CharCode.z;
-}
-
 const _encodeTable: string[] = (function () {
 	let table: string[] = [];
 	for (let code = 0; code < 128; code++) {
@@ -639,7 +631,7 @@ const _driveLetterRegExp = /^(\/?[a-z])(:|%3a)/i;
 /**
  * Create the external version of a uri
  */
-function _toString(encoder: { (code: number): boolean }[], scheme: string, authority: string, path: string, query: string, fragment: string): string {
+function _toExternal(encoder: { (code: number): boolean }[], scheme: string, authority: string, path: string, query: string, fragment: string): string {
 
 	let res = '';
 	if (scheme) {

src/vs/base/test/node/uri.test.ts

@@ -88,7 +88,7 @@ suite('URI', () => {
 		assert.equal(uri2.fragment, uri3.fragment);
 	});
 
-	test('with, identity', () => {
+	test('URI#with, identity', () => {
 		let uri = URI.parse('foo:bar/path');
 
 		let uri2 = uri.with(null!);
@@ -101,7 +101,7 @@ suite('URI', () => {
 		assert.ok(uri === uri2);
 	});
 
-	test('with, changes', () => {
+	test('URI#with, changes', () => {
 		assert.equal(URI.parse('before:some/file/path').with({ scheme: 'after' }).toString(), 'after:some/file/path');
 		assert.equal(URI.from({ scheme: 's' }).with({ scheme: 'http', path: '/api/files/test.me', query: 't=1234' }).toString(), 'http:/api/files/test.me?t=1234');
 		assert.equal(URI.from({ scheme: 's' }).with({ scheme: 'http', authority: '', path: '/api/files/test.me', query: 't=1234', fragment: '' }).toString(), 'http:/api/files/test.me?t=1234');
@@ -111,7 +111,7 @@ suite('URI', () => {
 		assert.equal(URI.from({ scheme: 's' }).with({ scheme: 'boo', authority: '', path: '/api/files/test.me', query: 't=1234', fragment: '' }).toString(), 'boo:/api/files/test.me?t=1234');
 	});
 
-	test('with, remove components #8465', () => {
+	test('URI#with, remove components #8465', () => {
 		assert.equal(URI.parse('scheme://authority/path').with({ authority: '' }).toString(), 'scheme:/path');
 		assert.equal(URI.parse('scheme:/path').with({ authority: 'authority' }).with({ authority: '' }).toString(), 'scheme:/path');
 		assert.equal(URI.parse('scheme:/path').with({ authority: 'authority' }).with({ authority: null }).toString(), 'scheme:/path');
@@ -121,7 +121,7 @@ suite('URI', () => {
 		assert.equal(URI.parse('scheme:/path').with({ authority: null }).toString(), 'scheme:/path');
 	});
 
-	test('with, validation', () => {
+	test('URI#with, validation', () => {
 		let uri = URI.parse('foo:bar/path');
 		assert.throws(() => uri.with({ scheme: 'fai:l' }));
 		assert.throws(() => uri.with({ scheme: 'fäil' }));
@@ -310,50 +310,52 @@ suite('URI', () => {
 		assert.equal(value.toString(), 'file:///a.file');
 	});
 
+
+	function assertToString(input: string | URI, expected: string) {
+		if (typeof input === 'string') {
+			input = URI.parse(input);
+		}
+		const actual = input.toString();
+		assert.equal(actual, expected.toString());
+	}
+
 	test('URI.toString, only scheme and query', () => {
-		const value = URI.parse('stuff:?qüery');
-		assert.equal(value.toString(), 'stuff:?q%C3%BCery');
+		assertToString('stuff:?qüery', 'stuff:?q%C3%BCery');
 	});
 
 	test('URI#toString, upper-case percent espaces', () => {
-		const value = URI.parse('file://sh%c3%a4res/path');
-		assert.equal(value.toString(), 'file://sh%C3%A4res/path');
+		assertToString('file://sh%c3%a4res/path', 'file://sh%C3%A4res/path');
+		assertToString('file://sh%c3%z4res/path', 'file://sh%C3%z4res/path');
+		assertToString('file:///sh%a0res/path', 'file:///sh%A0res/path'); // also upper-cased invalid sequence
 	});
 
 	test('URI#toString, lower-case windows drive letter', () => {
-		assert.equal(URI.parse('untitled:c:/Users/jrieken/Code/abc.txt').toString(), 'untitled:c%3A/Users/jrieken/Code/abc.txt');
-		assert.equal(URI.parse('untitled:C:/Users/jrieken/Code/abc.txt').toString(), 'untitled:c%3A/Users/jrieken/Code/abc.txt');
+		assertToString('untitled:c:/Users/jrieken/Code/abc.txt', 'untitled:c%3A/Users/jrieken/Code/abc.txt');
+		assertToString('untitled:C:/Users/jrieken/Code/abc.txt', 'untitled:c%3A/Users/jrieken/Code/abc.txt');
 	});
 
 	test('URI#toString, escape all the bits', () => {
-
 		const value = URI.file('/Users/jrieken/Code/_samples/18500/Mödel + Other Thîngß/model.js');
-		assert.equal(value.toString(), 'file:///Users/jrieken/Code/_samples/18500/M%C3%B6del%20+%20Other%20Th%C3%AEng%C3%9F/model.js');
+		assertToString(value, 'file:///Users/jrieken/Code/_samples/18500/M%C3%B6del%20+%20Other%20Th%C3%AEng%C3%9F/model.js');
 	});
 
 	test('URI#toString, don\'t encode port', () => {
 		let value = URI.parse('http://localhost:8080/far');
-		assert.equal(value.toString(), 'http://localhost:8080/far');
+		assertToString(value, 'http://localhost:8080/far');
 
 		value = URI.from({ scheme: 'http', authority: 'löcalhost:8080', path: '/far', query: undefined, fragment: undefined });
-		assert.equal(value.toString(), 'http://l%C3%B6calhost:8080/far');
+		assertToString(value, 'http://l%C3%B6calhost:8080/far');
 	});
 
 	test('URI#toString, user information in authority', () => {
-		let value = URI.parse('http://foo:bar@localhost/far');
-		assert.equal(value.toString(), 'http://foo:bar@localhost/far');
-
-		value = URI.parse('http://foo@localhost/far');
-		assert.equal(value.toString(), 'http://foo@localhost/far');
-
-		value = URI.parse('http://foo:bAr@localhost:8080/far');
-		assert.equal(value.toString(), 'http://foo:bAr@localhost:8080/far');
-
-		value = URI.parse('http://foo@localhost:8080/far');
-		assert.equal(value.toString(), 'http://foo@localhost:8080/far');
-
-		value = URI.from({ scheme: 'http', authority: 'föö:bör@löcalhost:8080', path: '/far', query: undefined, fragment: undefined });
-		assert.equal(value.toString(), 'http://f%C3%B6%C3%B6:b%C3%B6r@l%C3%B6calhost:8080/far');
+		assertToString('http://foo:bar@localhost/far', 'http://foo:bar@localhost/far');
+		assertToString('http://foo@localhost/far', 'http://foo@localhost/far');
+		assertToString('http://foo:bAr@localhost:8080/far', 'http://foo:bAr@localhost:8080/far');
+		assertToString('http://foo@localhost:8080/far', 'http://foo@localhost:8080/far');
+		assertToString(
+			URI.from({ scheme: 'http', authority: 'föö:bör@löcalhost:8080', path: '/far', query: undefined, fragment: undefined }),
+			'http://f%C3%B6%C3%B6:b%C3%B6r@l%C3%B6calhost:8080/far'
+		);
 	});
 
 	test('correctFileUriToFilePath2', () => {