Merge pull request #703 from lukecotter/fix-goto-symbol

feat: go to symbol robustness and fuzzy arg matching

Author: lcottercertinia

lana/src/display/OpenFileInPackage.ts

@@ -1,7 +1,7 @@
 /*
  * Copyright (c) 2020 Certinia Inc. All rights reserved.
  */
-import { sep } from 'path';
+import { basename, sep } from 'path';
 import {
   Position,
   Selection,
@@ -22,10 +22,10 @@ export class OpenFileInPackage {
       return;
     }
 
-    const parts = symbolName.split('.');
-    const fileName = parts[0]?.trim();
+    const parts = symbolName.slice(0, symbolName.indexOf('('));
 
-    const paths = await context.findSymbol(fileName as string);
+    // findSymbol displays an error message if not found, so no need to duplicate here but it would probably be better here.
+    const paths = await context.findSymbol(parts);
     if (!paths.length) {
       return;
     }
@@ -59,16 +59,17 @@ export class OpenFileInPackage {
 
     const parsedRoot = parseApex(document.getText());
 
-    const symbolLocation = getMethodLine(parsedRoot, parts);
+    const symbolLocation = getMethodLine(parsedRoot, symbolName);
 
     if (!symbolLocation.isExactMatch) {
       context.display.showErrorMessage(
-        `Symbol '${symbolLocation.missingSymbol}' could not be found in file '${fileName}'`,
+        `Symbol '${symbolLocation.missingSymbol}' could not be found in file '${basename(path)}'`,
       );
     }
     const zeroIndexedLineNumber = symbolLocation.line - 1;
+    const character = symbolLocation.character ?? 0;
 
-    const pos = new Position(zeroIndexedLineNumber, 0);
+    const pos = new Position(zeroIndexedLineNumber, character);
 
     const options: TextDocumentShowOptions = {
       preserveFocus: false,

lana/src/salesforce/ApexParser/ApexSymbolLocator.ts

@@ -8,9 +8,15 @@ import {
   CommonTokenStream,
 } from '@apexdevtools/apex-parser';
 import { CharStreams } from 'antlr4ts';
-import { ApexVisitor, type ApexMethodNode, type ApexNode } from './ApexVisitor';
+import {
+  ApexVisitor,
+  type ApexConstructorNode,
+  type ApexMethodNode,
+  type ApexNode,
+} from './ApexVisitor';
 
 export type SymbolLocation = {
+  character: number;
   line: number;
   isExactMatch: boolean;
   missingSymbol?: string;
@@ -25,61 +31,143 @@ export function parseApex(apexCode: string): ApexNode {
   return new ApexVisitor().visit(parser.compilationUnit());
 }
 
-export function getMethodLine(rootNode: ApexNode, symbols: string[]): SymbolLocation {
-  const result: SymbolLocation = { line: 1, isExactMatch: true };
+export function getMethodLine(rootNode: ApexNode, fullyQualifiedSymbol: string): SymbolLocation {
+  const result: SymbolLocation = { character: 0, line: 1, isExactMatch: false };
 
-  if (symbols[0] === rootNode.name) {
-    symbols = symbols.slice(1);
+  if (fullyQualifiedSymbol.indexOf('(') === -1) {
+    return result;
   }
 
-  if (!symbols.length) {
-    return result;
+  // NOTE: The symbol may contain namespaces as symbols from the debug log are fully qualified e.g myns.MyClass.InnerClass.method(args)
+  // We are attempting rudamentary handling of the case where symbols contain namespace but the parsed class does not but no guarantees it work in all cases.
+
+  // There are two possible symbol types method and constructor, args are optional
+  // MyClass.method(args) - method,  MyClass.InnerClass.method(args) - method
+  // MyClass(args) - constuctor, MyClass.InnerClass(args) - constuctor
+
+  // Find the Namespace of the supplied symbol, if there is one.
+  //  strip all whitespace to make comparisons easier
+  let symbolToFind = normalizeText(fullyQualifiedSymbol);
+  const outerClassNode = rootNode.children?.[0];
+  let outerClassName = normalizeText(outerClassNode?.name ?? '');
+  const endNsIndex = symbolToFind.indexOf(outerClassName);
+  const namespace = endNsIndex > 0 ? symbolToFind.slice(0, endNsIndex - 1) : '';
+  if (namespace) {
+    outerClassName = namespace + '.' + outerClassName;
+    // Remove the leading namespace as most likely the source code will not have it, for symbols in this file.
+    symbolToFind = symbolToFind.replace(namespace + '.', '');
   }
 
+  // This is the index of the first '(' which indicates method args or constructor args.
+  const methodArgsIndex = symbolToFind.indexOf('(');
+  // We can't tell the difference between InnerClass constructor and outer class method call.
+  // As such className could either be the class name or the method name, we need to check.
+  const className = symbolToFind.slice(0, methodArgsIndex);
   let currentRoot: ApexNode | undefined = rootNode;
+  // Keep iterating until we find the last symbol that is a class.
+  // The next symbol might be a method or might be invalid.
+  for (const symbol of className.split('.')) {
+    const nextRoot = findClassNode(currentRoot, symbol, namespace);
+    if (!nextRoot) {
+      break;
+    }
+
+    currentRoot = nextRoot;
+  }
+
+  if (currentRoot) {
+    result.line = currentRoot.line ?? 1;
+    result.character = currentRoot.idCharacter ?? 0;
+  }
 
-  for (const symbol of symbols) {
-    if (isClassSymbol(symbol)) {
-      currentRoot = findClassNode(currentRoot, symbol);
-
-      if (!currentRoot) {
-        result.isExactMatch = false;
-        result.missingSymbol = symbol;
-        break;
-      }
-    } else {
-      const methodNode = findMethodNode(currentRoot, symbol);
-
-      if (!methodNode) {
-        result.line = currentRoot.line ?? 1;
-        result.isExactMatch = false;
-        result.missingSymbol = symbol;
-        break;
-      }
+  // This is the method name before the args list, this may actually be a class name though so we need to check.
+  // e.g for MyClass.InnerClass(args) we get InnerClass(args) but is this a method of InnerClass constructor?
+  const qualifiedMethodName = symbolToFind.slice(className.lastIndexOf('.') + 1);
+  if (qualifiedMethodName && currentRoot) {
+    let methodNode: ApexMethodNode | ApexConstructorNode | undefined = findMethodNode(
+      currentRoot,
+      qualifiedMethodName,
+      outerClassName,
+    );
+    if (!methodNode) {
+      methodNode = findConstructorNode(currentRoot, qualifiedMethodName, outerClassName);
+    }
 
+    if (methodNode) {
       result.line = methodNode.line;
+      result.character = methodNode.idCharacter;
+      result.isExactMatch = true;
+      return result;
     }
   }
 
+  result.line = currentRoot.line ?? 1;
+  result.isExactMatch = false;
+  // keep the original case for error messages.
+  result.missingSymbol = fullyQualifiedSymbol.slice(className.lastIndexOf('.') + 1);
   return result;
 }
 
-function isClassSymbol(symbol: string): boolean {
-  return !symbol.includes('(');
+function findClassNode(root: ApexNode, symbol: string, namespace: string): ApexNode | undefined {
+  const symbolWithoutNamespace = symbol.replaceAll(namespace + '.', '');
+  return root.children?.find((child) => {
+    if (child.nature === 'Class') {
+      const normalizedChildName = normalizeText(child.name ?? '');
+      return normalizedChildName === symbol || normalizedChildName === symbolWithoutNamespace;
+    }
+
+    return false;
+  });
 }
 
-function findClassNode(root: ApexNode, symbol: string): ApexNode | undefined {
-  return root.children?.find((child) => child.name === symbol && child.nature === 'Class');
+function findMethodNode(
+  root: ApexNode,
+  symbol: string,
+  outerClassName: string,
+): ApexMethodNode | undefined {
+  const [methodName, params = ''] = symbol.slice(0, -1).split('(');
+  // Try again but with the class name removed from args list. args from the debug log are fully qualified but they are not necessarily in the file,
+  // as we only need to qualify for external types to the file.
+  // (MyClass.ObjectArg) vs (ObjectArg) where MyClass is current class.
+  const paramsWithoutClassName = params.replaceAll(outerClassName + '.', '');
+
+  return root.children?.find((child) => {
+    if (child.nature === 'Method' && normalizeText(child.name ?? '') === methodName) {
+      const methodChild = child as ApexMethodNode;
+      const methodParams = normalizeText(methodChild.params);
+      return (
+        params === undefined || methodParams === params || methodParams === paramsWithoutClassName
+      );
+    }
+    return false;
+  }) as ApexMethodNode;
 }
 
-function findMethodNode(root: ApexNode, symbol: string): ApexMethodNode | undefined {
-  const [methodName, params] = symbol.split('(');
-  const paramStr = params?.replace(')', '').trim();
+function findConstructorNode(
+  root: ApexNode,
+  symbol: string,
+  outerClassName: string,
+): ApexConstructorNode | undefined {
+  const [constructorName, params = ''] = symbol.slice(0, -1).split('(');
+  // Try again but with the class name removed from args list. args from the debug log are fully qualified but they are not necessarily in the file,
+  // as we only need to qualify for external types to the file.
+  // (MyClass.ObjectArg) vs (ObjectArg) where MyClass is current class.
+  const paramsWithoutClassName = params.replaceAll(outerClassName + '.', '');
+
+  return root.children?.find((child) => {
+    if (child.nature === 'Constructor' && normalizeText(child.name ?? '') === constructorName) {
+      const constructorChild = child as ApexConstructorNode;
+      const constructorParams = normalizeText(constructorChild.params);
+      return (
+        params === undefined ||
+        constructorParams === params ||
+        constructorParams === paramsWithoutClassName
+      );
+    }
+    return false;
+  }) as ApexConstructorNode;
+}
 
-  return root.children?.find(
-    (child) =>
-      child.name === methodName &&
-      child.nature === 'Method' &&
-      (paramStr === undefined || (child as ApexMethodNode).params === paramStr),
-  ) as ApexMethodNode;
+function normalizeText(text: string): string {
+  return text?.replaceAll(' ', '').toLowerCase();
 }

lana/src/salesforce/ApexParser/ApexVisitor.ts

@@ -4,25 +4,68 @@
 import type {
   ApexParserVisitor,
   ClassDeclarationContext,
+  ConstructorDeclarationContext,
   FormalParametersContext,
   MethodDeclarationContext,
 } from '@apexdevtools/apex-parser';
 import type { ErrorNode, ParseTree, RuleNode, TerminalNode } from 'antlr4ts/tree';
 
-type ApexNature = 'Class' | 'Method';
+type ApexNature = 'Constructor' | 'Class' | 'Method';
 
+/**
+ * Represents a node in the Apex syntax tree.
+ * Can be either a class or method declaration with optional child nodes.
+ */
 export interface ApexNode {
+  /** The type of Apex construct (Class or Method) */
   nature?: ApexNature;
+  /** The name of the class or method */
   name?: string;
+  /** Child nodes (nested classes or methods) */
   children?: ApexNode[];
+  /** Line number where the node is declared */
   line?: number;
+  /** Character position of the identifier on the line */
+  idCharacter?: number;
 }
 
-export type ApexMethodNode = ApexNode & {
-  nature: 'Method';
+/**
+ * Represents a class declaration node in the Apex syntax tree.
+ * All properties are required (non-optional) to ensure complete class metadata.
+ */
+export interface ApexClassNode extends ApexNode {
+  /** Indicates this node represents a class declaration */
+  nature: 'Class';
+  /** Line number where the class is declared */
+  line: number;
+  /** Character position of the class identifier on the line */
+  idCharacter: number;
+}
+
+export interface ApexParamNode extends ApexNode {
+  /** Indicates this node represents a method declaration */
+  nature: 'Method' | 'Constructor';
+  /** Comma-separated list of parameter types for the method */
   params: string;
+  /** Line number where the method is declared */
   line: number;
-};
+  /** Character position of the method identifier on the line */
+  idCharacter: number;
+}
+
+/**
+ * Represents a method declaration node in the Apex syntax tree.
+ * All properties are required (non-optional) to ensure complete method metadata.
+ */
+export interface ApexMethodNode extends ApexParamNode {
+  /** Indicates this node represents a method declaration */
+  nature: 'Method';
+}
+
+export interface ApexConstructorNode extends ApexParamNode {
+  /** Indicates this node represents a method declaration */
+  nature: 'Constructor';
+}
 
 type VisitableApex = ParseTree & {
   accept<Result>(visitor: ApexParserVisitor<Result>): Result;
@@ -49,22 +92,45 @@ export class ApexVisitor implements ApexParserVisitor<ApexNode> {
     return { children };
   }
 
-  visitClassDeclaration(ctx: ClassDeclarationContext): ApexNode {
+  visitClassDeclaration(ctx: ClassDeclarationContext): ApexClassNode {
+    const { start } = ctx;
+    const ident = ctx.id();
+
     return {
       nature: 'Class',
-      name: ctx.id().Identifier()?.toString() ?? '',
+      name: ident.text ?? '',
       children: ctx.children?.length ? this.visitChildren(ctx).children : [],
-      line: ctx.start.line,
+      line: start.line,
+      idCharacter: ident.start.charPositionInLine ?? 0,
+    };
+  }
+
+  visitConstructorDeclaration(ctx: ConstructorDeclarationContext): ApexConstructorNode {
+    const { start } = ctx;
+    const idContexts = ctx.qualifiedName().id();
+    const constructorName = idContexts[idContexts.length - 1];
+
+    return {
+      nature: 'Constructor',
+      name: constructorName?.text ?? '',
+      children: ctx.children?.length ? this.visitChildren(ctx).children : [],
+      params: this.getParameters(ctx.formalParameters()),
+      line: start.line,
+      idCharacter: start.charPositionInLine ?? 0,
     };
   }
 
   visitMethodDeclaration(ctx: MethodDeclarationContext): ApexMethodNode {
+    const { start } = ctx;
+    const ident = ctx.id();
+
     return {
       nature: 'Method',
-      name: ctx.id().Identifier()?.toString() ?? '',
+      name: ident.text ?? '',
       children: ctx.children?.length ? this.visitChildren(ctx).children : [],
       params: this.getParameters(ctx.formalParameters()),
-      line: ctx.start.line,
+      line: start.line,
+      idCharacter: ident.start.charPositionInLine ?? 0,
     };
   }
 
@@ -78,7 +144,7 @@ export class ApexVisitor implements ApexParserVisitor<ApexNode> {
 
   private getParameters(ctx: FormalParametersContext): string {
     const paramsList = ctx.formalParameterList()?.formalParameter();
-    return paramsList?.map((param) => param.typeRef().typeName(0)?.text).join(', ') ?? '';
+    return paramsList?.map((param) => param.typeRef().text).join(',') ?? '';
   }
 
   private forNode(node: ApexNode, anonHandler: (n: ApexNode) => void) {