feat: add child combinator ">" (and fix a specificity bug) (#233)

* feat: fix 2 bugs and add child combinator ">"

Bug: Just because a trie element exists for a more specific scope doesn‘t mean its parent scopes will match, so we need to collect the trie elements with less specific scopes too.

Bug: If the number of scope names in both rules‘ scope paths are not
equal, the parent scope names won‘t be compared at all. Instead, the rule with
the longest scope path is preferred. This goes against the TextMate
manual (https://macromates.com/manual/en/scope_selectors). In
particular, the following line in “Ranking Matches”:
> Rules 1 and 2 applied again to the scope selector when removing the deepest element (in the case of a tie)

Feature: Add support for the child combinator (the `>` operator). This
allows for styling a parent-child relationship specifically.

* fix: proceed to next scope after successful match

* fix: increment parent indexes after comparison

* chore: add comments and some small improvements

* test: add 3 new cases

- One for the new child combinator
- One for bug 1 ("Theme resolving falls back to less specific rules")
- One for bug 2 ("Theme resolving should give deeper scopes higher specificity")

* chore(revert): undo bug 1 fix

After trying to reproduce the alleged bug, I realized it‘s not a bug. When a ThemeTrieElement is created, it inherits the `_rulesWithParentScopes` array of its parent element, which is guaranteed to be populated due to the lexicographical sorting of the theme rules in `resolveParsedThemeRules`.

* test: remove test case for non-existent bug

…and update the other bug‘s test case to actually fail on the main branch

src/tests/themes.test.ts

@@ -606,6 +606,63 @@ test('Theme resolving rules with parent scopes', () => {
 	assertThemeEqual(actual, expected);
 });
 
+test('Theme resolving a rule with child combinator', () => {
+	let theme = Theme.createFromRawTheme({
+		settings: [
+			{ settings: { foreground: '#100000' } },
+			{ scope: 'b a', settings: { foreground: '#200000' } },
+			{ scope: 'b > a', settings: { foreground: '#300000' } },
+			{ scope: 'c > b > a', settings: { foreground: '#400000' } },
+			{ scope: 'a', settings: { foreground: '#500000' } },
+		]
+	});
+
+	const colorMap = theme.getColorMap();
+	const match = (...path: string[]) => {
+		const result = theme.match(ScopeStack.from(...path));
+		if (!result) {
+			return null;
+		}
+		return colorMap[result.foregroundId];
+	};
+
+	assert.equal(match('b', 'a'), '#300000', 'b a');
+	assert.equal(match('b', 'c', 'a'), '#200000', 'b c a');
+	assert.equal(match('c', 'b', 'a'), '#400000', 'c b a');
+	assert.equal(match('c', 'b', 'd', 'a'), '#200000', 'c b d a');
+});
+
+test('Theme resolving should give deeper scopes higher specificity (#233)', () => {
+	let theme = Theme.createFromRawTheme({
+		settings: [
+			{ settings: { foreground: '#100000' } },
+			{ scope: 'y.z a.b', settings: { foreground: '#200000' } },
+			{ scope: 'x y a.b', settings: { foreground: '#300000' } },
+		]
+	});
+
+	const colorMap = theme.getColorMap();
+	const defaults = theme.getDefaults();
+
+	const match = (...path: string[]) => {
+		const result = theme.match(ScopeStack.from(...path));
+		if (!result || result.foregroundId === 0) {
+			return null;
+		}
+		return colorMap[result.foregroundId];
+	};
+
+	// Sanity check
+	assert.equal(match('x', 'a.b'), null, 'x a.b');
+	assert.equal(match('y', 'a.b'), null, 'y a.b');
+	assert.equal(match('y.z', 'a'), null, 'y.z a');
+	assert.equal(match('x', 'y', 'a.b'), '#300000', 'x y a.b');
+
+	// Even though the "x y a.b" rule has more scopes in its path, the "y.z a.b" rule has
+	// a deeper match, so it should take precedence.
+	assert.equal(match('x', 'y.z', 'a.b'), '#200000', 'y.z a.b');
+});
+
 test('Theme resolving issue #38: ignores rules with invalid colors', () => {
 	let actual = parseTheme({
 		settings: [{

src/theme.ts

@@ -161,26 +161,46 @@ export class ScopeStack {
 	}
 }
 
-function _scopePathMatchesParentScopes(scopePath: ScopeStack | null, parentScopes: ScopeName[] | null): boolean {
-	if (parentScopes === null) {
+function _scopePathMatchesParentScopes(scopePath: ScopeStack | null, parentScopes: readonly ScopeName[]): boolean {
+	if (parentScopes.length === 0) {
 		return true;
 	}
 
-	let index = 0;
-	let scopePattern = parentScopes[index];
+	// Starting with the deepest parent scope, look for a match in the scope path.
+	for (let index = 0; index < parentScopes.length; index++) {
+		let scopePattern = parentScopes[index];
+		let scopeMustMatch = false;
 
-	while (scopePath) {
-		if (_matchesScope(scopePath.scopeName, scopePattern)) {
-			index++;
-			if (index === parentScopes.length) {
-				return true;
+		// Check for a child combinator (a parent-child relationship)
+		if (scopePattern === '>') {
+			if (index === parentScopes.length - 1) {
+				// Invalid use of child combinator
+				return false;
 			}
-			scopePattern = parentScopes[index];
+			scopePattern = parentScopes[++index];
+			scopeMustMatch = true;
+		}
+
+		while (scopePath) {
+			if (_matchesScope(scopePath.scopeName, scopePattern)) {
+				break;
+			}
+			if (scopeMustMatch) {
+				// If a child combinator was used, the parent scope must match.
+				return false;
+			}
+			scopePath = scopePath.parent;
+		}
+
+		if (!scopePath) {
+			// No more potential matches
+			return false;
 		}
 		scopePath = scopePath.parent;
 	}
 
-	return false;
+	// All parent scopes were matched.
+	return true;
 }
 
 function _matchesScope(scopeName: ScopeName, scopePattern: ScopeName): boolean {
@@ -427,17 +447,19 @@ export class ColorMap {
 	}
 }
 
+const emptyParentScopes= Object.freeze(<ScopeName[]>[]);
+
 export class ThemeTrieElementRule {
 
 	scopeDepth: number;
-	parentScopes: ScopeName[] | null;
+	parentScopes: readonly ScopeName[];
 	fontStyle: number;
 	foreground: number;
 	background: number;
 
-	constructor(scopeDepth: number, parentScopes: ScopeName[] | null, fontStyle: number, foreground: number, background: number) {
+	constructor(scopeDepth: number, parentScopes: readonly ScopeName[] | null, fontStyle: number, foreground: number, background: number) {
 		this.scopeDepth = scopeDepth;
-		this.parentScopes = parentScopes;
+		this.parentScopes = parentScopes || emptyParentScopes;
 		this.fontStyle = fontStyle;
 		this.foreground = foreground;
 		this.background = background;
@@ -489,55 +511,77 @@ export class ThemeTrieElement {
 		this._rulesWithParentScopes = rulesWithParentScopes;
 	}
 
-	private static _sortBySpecificity(arr: ThemeTrieElementRule[]): ThemeTrieElementRule[] {
-		if (arr.length === 1) {
-			return arr;
-		}
-		arr.sort(this._cmpBySpecificity);
-		return arr;
-	}
-
 	private static _cmpBySpecificity(a: ThemeTrieElementRule, b: ThemeTrieElementRule): number {
-		if (a.scopeDepth === b.scopeDepth) {
-			const aParentScopes = a.parentScopes;
-			const bParentScopes = b.parentScopes;
-			let aParentScopesLen = aParentScopes === null ? 0 : aParentScopes.length;
-			let bParentScopesLen = bParentScopes === null ? 0 : bParentScopes.length;
-			if (aParentScopesLen === bParentScopesLen) {
-				for (let i = 0; i < aParentScopesLen; i++) {
-					const aLen = aParentScopes![i].length;
-					const bLen = bParentScopes![i].length;
-					if (aLen !== bLen) {
-						return bLen - aLen;
-					}
-				}
-			}
-			return bParentScopesLen - aParentScopesLen;
+		// First, compare the scope depths of both rules. The “scope depth” of a rule is
+		// the number of segments (delimited by dots) in the rule's deepest scope name
+		// (i.e. the final scope name in the scope path delimited by spaces).
+		if (a.scopeDepth !== b.scopeDepth) {
+			return b.scopeDepth - a.scopeDepth;
 		}
-		return b.scopeDepth - a.scopeDepth;
+
+		// Traverse the parent scopes depth-first, comparing the specificity of both
+		// rules' parent scopes, which matches the behavior described by ”Ranking Matches”
+		// in TextMate 1.5's manual: https://macromates.com/manual/en/scope_selectors
+		// Start at index 0 for both rules, since the parent scopes were reversed
+		// beforehand (i.e. index 0 is the deepest parent scope).
+		let aParentIndex = 0;
+		let bParentIndex = 0;
+
+		while (true) {
+			// Child combinators don't affect specificity.
+			if (a.parentScopes[aParentIndex] === '>') {
+				aParentIndex++;
+			}
+			if (b.parentScopes[bParentIndex] === '>') {
+				bParentIndex++;
+			}
+
+			// This is a scope-by-scope comparison, so we need to stop once a rule runs
+			// out of parent scopes.
+			if (aParentIndex >= a.parentScopes.length || bParentIndex >= b.parentScopes.length) {
+				break;
+			}
+
+			// When sorting by scope name specificity, it's safe to treat a longer parent
+			// scope as more specific. If both rules' parent scopes match a given scope
+			// path, the longer parent scope will always be more specific.
+			const parentScopeLengthDiff =
+				b.parentScopes[bParentIndex].length - a.parentScopes[aParentIndex].length;
+
+			if (parentScopeLengthDiff !== 0) {
+				return parentScopeLengthDiff;
+			}
+
+			aParentIndex++;
+			bParentIndex++;
+		}
+
+		// If a depth-first, scope-by-scope comparison resulted in a tie, the rule with
+		// more parent scopes is considered more specific.
+		return b.parentScopes.length - a.parentScopes.length;
 	}
 
 	public match(scope: ScopeName): ThemeTrieElementRule[] {
-		if (scope === '') {
-			return ThemeTrieElement._sortBySpecificity((<ThemeTrieElementRule[]>[]).concat(this._mainRule).concat(this._rulesWithParentScopes));
+		if (scope !== '') {
+			let dotIndex = scope.indexOf('.')
+			let head: string
+			let tail: string
+			if (dotIndex === -1) {
+				head = scope
+				tail = ''
+			} else {
+				head = scope.substring(0, dotIndex)
+				tail = scope.substring(dotIndex + 1)
+			}
+
+			if (this._children.hasOwnProperty(head)) {
+				return this._children[head].match(tail);
+			}
 		}
 
-		let dotIndex = scope.indexOf('.');
-		let head: string;
-		let tail: string;
-		if (dotIndex === -1) {
-			head = scope;
-			tail = '';
-		} else {
-			head = scope.substring(0, dotIndex);
-			tail = scope.substring(dotIndex + 1);
-		}
-
-		if (this._children.hasOwnProperty(head)) {
-			return this._children[head].match(tail);
-		}
-
-		return ThemeTrieElement._sortBySpecificity((<ThemeTrieElementRule[]>[]).concat(this._mainRule).concat(this._rulesWithParentScopes));
+		const rules = this._rulesWithParentScopes.concat(this._mainRule);
+		rules.sort(ThemeTrieElement._cmpBySpecificity);
+		return rules;
 	}
 
 	public insert(scopeDepth: number, scope: ScopeName, parentScopes: ScopeName[] | null, fontStyle: number, foreground: number, background: number): void {

src/utils.ts

@@ -105,7 +105,7 @@ export function strcmp(a: string, b: string): number {
 	return 0;
 }
 
-export function strArrCmp(a: string[] | null, b: string[] | null): number {
+export function strArrCmp(a: readonly string[] | null, b: readonly string[] | null): number {
 	if (a === null && b === null) {
 		return 0;
 	}