Merge pull request #94 from SSlinky/dev

Bug fix scope cleaning

Author: SSlinky

server/src/capabilities/capabilities.ts

@@ -17,7 +17,7 @@ import { FoldingRange, FoldingRangeKind } from '../capabilities/folding';
 import { SemanticToken, SemanticTokenModifiers, SemanticTokenTypes } from '../capabilities/semanticTokens';
 import { BaseRuleSyntaxElement, BaseIdentifyableSyntaxElement, BaseSyntaxElement, Context, HasSemanticTokenCapability } from '../project/elements/base';
 import { AmbiguousNameDiagnostic, BaseDiagnostic, DuplicateDeclarationDiagnostic, ShadowDeclarationDiagnostic, SubOrFunctionNotDefinedDiagnostic, UnusedDiagnostic, VariableNotDefinedDiagnostic } from './diagnostics';
-import { isPositionInsideRange, isRangeInsideRange } from '../utils/helpers';
+import { isPositionInsideRange, isRangeInsideRange, rangeEquals } from '../utils/helpers';
 
 
 abstract class BaseCapability {
@@ -266,6 +266,10 @@ export class ScopeItemCapability {
 		return result === '' ? 'NONE' : result;
 	}
 
+	get range(): Range | undefined {
+		return this.element?.context.range;
+	}
+
 	// Item Properties
 	locationUri?: string;
 	isPublicScope?: boolean;
@@ -287,24 +291,7 @@ export class ScopeItemCapability {
 		if (this.type === ScopeType.REFERENCE) {
 			// Link to declaration if it exists.
 			this.resolveLinks();
-			if (!this.link) {
-				// TODO:
-				// References to variables should get a diagnostic if they aren't declared.
-				//  -- No option explicit: Hint with code action to declare.
-				//						   GET before declared gets a warning.
-				//  -- Option explicit: Error with code action to declare.
-				//  -- Subsequent explicit declaration should raise duplicate declaration (current bahaviour).
-				// 	-- All declarations with no GET references get a warning.
-				// References to function or sub calls should raise an error if they aren't declared.
-				//	-- Must always throw even when option explicit not present.
-				//	-- Nothing required on first reference as declaration may come later.
-				const severity = this.isOptionExplicitScope
-					? DiagnosticSeverity.Error
-					: DiagnosticSeverity.Hint;
-				const _ = this.assignmentType & AssignmentType.CALL
-					? this.pushDiagnostic(SubOrFunctionNotDefinedDiagnostic, this, this.name)
-					: this.pushDiagnostic(VariableNotDefinedDiagnostic, this, this.name, severity);
-			}
+			this.validateLink();
 		} else {
 			// Diagnostic checks on declarations.
 			const ancestors = this.getParentChain();
@@ -546,6 +533,27 @@ export class ScopeItemCapability {
 		this.linkThisToItem(foundDeclarations[0]);
 	}
 
+	private validateLink() {
+		if (!this.link) {
+			// TODO:
+			// References to variables should get a diagnostic if they aren't declared.
+			//  -- No option explicit: Hint with code action to declare.
+			//						   GET before declared gets a warning.
+			//  -- Option explicit: Error with code action to declare.
+			//  -- Subsequent explicit declaration should raise duplicate declaration (current bahaviour).
+			// 	-- All declarations with no GET references get a warning.
+			// References to function or sub calls should raise an error if they aren't declared.
+			//	-- Must always throw even when option explicit not present.
+			//	-- Nothing required on first reference as declaration may come later.
+			const severity = this.isOptionExplicitScope
+				? DiagnosticSeverity.Error
+				: DiagnosticSeverity.Hint;
+			const _ = this.assignmentType & AssignmentType.CALL
+				? this.pushDiagnostic(SubOrFunctionNotDefinedDiagnostic, this, this.name)
+				: this.pushDiagnostic(VariableNotDefinedDiagnostic, this, this.name, severity);
+		}
+	}
+
 	private linkThisToItem(linkItem?: ScopeItemCapability): void {
 		if (!linkItem) {
 			return;
@@ -787,28 +795,23 @@ export class ScopeItemCapability {
 	 * Recursively removes all scopes with the passed in uri and
 	 * within the range bounds, including where it is linked.
 	 */
-	invalidate(uri: string, range: Range): void {
-		const isInvalidScope = (scope: ScopeItemCapability) =>
-			scope.locationUri === uri
-			&& scope.element?.context.range
-			&& isRangeInsideRange(scope.element.context.range, range);
-
+	invalidate(uri: string, range?: Range): void {
 		const cleanScopes = (scopes?: ScopeItemCapability[]) => {
 			if (scopes === undefined) {
 				return undefined;
 			}
 
 			const result: ScopeItemCapability[] = [];
 			scopes.forEach(scope => {
-				if (isInvalidScope(scope)) {
+				if (scope.isLocatedAt(uri, range)) {
 					Services.logger.debug(`Invalidating ${scope.name}`);
 
 					// Clean the backlinks on the linked item if we have one.
 					if (scope.link) scope.link.backlinks = cleanScopes(
 						scope.link.backlinks);
 
 					// Clean the invaludated scope.
-					scope.invalidate(uri, range);
+					scope.invalidate(uri, scope.range);
 
 					return;
 				}
@@ -849,10 +852,24 @@ export class ScopeItemCapability {
 
 		// Do a basic clean on backlinks that doesn't trigger recursion.
 		if (this.backlinks) {
-			this.backlinks = this.backlinks.filter(scope => !isInvalidScope(scope));
+			this.backlinks = this.backlinks
+				.filter(scope => !scope.isLocatedAt(uri, range));
 		}
 	}
 
+	/** Returns true if the uri matches and, if passed, range is fully inside range. */
+	isLocatedAt(uri: string, range?: Range): boolean {
+		if (uri !== this.locationUri) {
+			return false;
+		}
+
+		if (!range) {
+			return true;
+		}
+
+		return isRangeInsideRange(this.range, range);
+	}
+
 	/** Returns true for public and false for private */
 	private getVisibility(item: ScopeItemCapability): boolean {
 		// Classes and modules are always public.
@@ -900,7 +917,7 @@ export class ScopeItemCapability {
 
 			// Check all items for whether they have a name overlap or a scope overlap.
 			scope?.maps.forEach(map => map.forEach(items => items.forEach(item => {
-				const elementRange = item.element?.context.range;
+				const elementRange = item.range;
 				const identifierRange = item.element?.identifierCapability?.range;
 				if (identifierRange && isPositionInsideRange(position, identifierRange)) {
 					// Position is inside the identifier, push to results.
@@ -972,7 +989,7 @@ export class ScopeItemCapability {
 			link.locationUri,
 			link.element.context.range,
 			link.element.identifierCapability.range,
-			this.element?.context.range
+			this.range
 		);
 	}
 
@@ -1045,7 +1062,7 @@ export class ScopeItemCapability {
 		};
 
 		const m = new Map<string, ScopeItemCapability>();
-		results.forEach(s => m.set(rangeString(s.element?.context.range), s));
+		results.forEach(s => m.set(rangeString(s.range), s));
 		return Array.from(m.values());
 	}
 }

server/src/project/workspace.ts

@@ -153,8 +153,7 @@ export class Workspace implements IWorkspace {
 
 		if (previousDocument) {
 			Services.projectScope.invalidate(
-				previousDocument.uri,
-				previousDocument.range
+				previousDocument.uri
 			);
 		}
 

server/src/utils/helpers.ts

@@ -94,7 +94,12 @@ export function isPositionInsideRange(position: Position, range: Range): boolean
  * @param inner The range to test as enveloped.
  * @param outer The range to test as enveloping.
  */
-export function isRangeInsideRange(inner: Range, outer: Range): boolean {
+export function isRangeInsideRange(inner?: Range, outer?: Range): boolean {
+	// Test we have ranges.
+	if (!inner || !outer) {
+		return false;
+	}
+
 	// Test characters on single-line ranges.
 	const isSingleLine = inner.start.line === inner.end.line
 		&& outer.start.line === outer.end.line
@@ -107,4 +112,12 @@ export function isRangeInsideRange(inner: Range, outer: Range): boolean {
 	// Test lines on multi-line ranges.
 	return inner.start.line >= outer.start.line
 		&& inner.end.line <= outer.end.line;
+}
+
+export function rangeEquals(r1?: Range, r2?: Range): boolean {
+	return !!r1 && !!r2
+		&& r1.start.line === r2.start.line
+		&& r1.start.character === r2.start.character
+		&& r1.end.line === r2.end.line
+		&& r1.end.character === r2.end.character;
 }