fix: construct errors are rendered in a messy way

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

@@ -1,5 +1,5 @@
 import { EOL } from 'os';
-import type { Construct } from 'constructs';
+import type { Construct, IConstruct } from 'constructs';
 import { BucketGrants } from './bucket-grants';
 import { BucketPolicy } from './bucket-policy';
 import type { IBucketNotificationDestination } from './destination';
@@ -2125,7 +2125,6 @@ export class Bucket extends BucketBase {
     if (!bucketName) {
       throw new ValidationError('BucketNameRequired', 'Bucket name is required', scope);
     }
-    Bucket.validateBucketName(bucketName, true);
 
     const oldEndpoint = `s3-website-${region}.${urlSuffix}`;
     const newEndpoint = `s3-website.${region}.${urlSuffix}`;
@@ -2140,7 +2139,7 @@ export class Bucket extends BucketBase {
 
     const websiteDomain = `${bucketName}.${staticDomainEndpoint}`;
 
-    return new ReferencedBucket(scope, id, {
+    const ret = new ReferencedBucket(scope, id, {
       account: attrs.account,
       region: attrs.region,
       bucketName: bucketName!,
@@ -2156,6 +2155,8 @@ export class Bucket extends BucketBase {
       disallowPublicAccess: false,
       notificationsHandlerRole: attrs.notificationsHandlerRole,
     });
+    Bucket.validateBucketNameScoped(ret, bucketName, true);
+    return ret;
   }
 
   /**
@@ -2224,11 +2225,22 @@ export class Bucket extends BucketBase {
    * @param allowLegacyBucketNaming allow legacy bucket naming style, default is false.
    */
   public static validateBucketName(physicalName: string, allowLegacyBucketNaming: boolean = false): void {
+    const errors = Bucket._validateBucketName(physicalName, allowLegacyBucketNaming);
+    if (errors.length > 0) {
+      // Since this is public and can be called statically, we have no object instance, so we throw an unscoped error.
+      throw new UnscopedValidationError('InvalidBucketNameValue', `Invalid S3 bucket name (value: ${physicalName})${EOL}${errors.join(EOL)}`);
+    }
+  }
+
+  /**
+   * Return any errors against the bucket name
+   */
+  private static _validateBucketName(physicalName: string, allowLegacyBucketNaming: boolean = false): string[] {
     const bucketName = physicalName;
     if (!bucketName || Token.isUnresolved(bucketName)) {
       // the name is a late-bound value, not a defined string,
       // so skip validation
-      return;
+      return [];
     }
 
     const errors: string[] = [];
@@ -2273,8 +2285,16 @@ export class Bucket extends BucketBase {
       errors.push('Bucket name must not resemble an IP address');
     }
 
+    return errors;
+  }
+
+  /**
+   * Like 'validateBucketName', but has an instance to throw a scoped ValidationError against
+   */
+  private static validateBucketNameScoped(scope: IConstruct, physicalName: string, allowLegacyBucketNaming: boolean = false): void {
+    const errors = Bucket._validateBucketName(physicalName, allowLegacyBucketNaming);
     if (errors.length > 0) {
-      throw new UnscopedValidationError('InvalidBucketNameValue', `Invalid S3 bucket name (value: ${bucketName})${EOL}${errors.join(EOL)}`);
+      throw new ValidationError('InvalidBucketNameValue', `Invalid S3 bucket name (value: ${physicalName})${EOL}${errors.join(EOL)}`, scope);
     }
   }
 
@@ -2327,7 +2347,7 @@ export class Bucket extends BucketBase {
     const { bucketEncryption, encryptionKey } = this.parseEncryption(props);
     this.encryptionKey = encryptionKey;
 
-    Bucket.validateBucketName(this.physicalName);
+    Bucket.validateBucketNameScoped(this, this.physicalName);
 
     let publicAccessBlockConfig: BlockPublicAccessOptions | undefined = props.blockPublicAccess;
     if (props.blockPublicAccess && FeatureFlags.of(this).isEnabled(cxapi.S3_PUBLIC_ACCESS_BLOCKED_BY_DEFAULT)) {

packages/aws-cdk-lib/core/lib/errors.ts

@@ -1,5 +1,6 @@
 import type { IConstruct } from 'constructs';
 import { constructInfoFromConstruct } from './private/runtime-info';
+import { captureCallStack, renderCallStackJustMyCode } from './stack-trace';
 import type { AssertionError } from '../../assertions/lib/private/error';
 import type { CloudAssemblyError } from '../../cx-api/lib/private/error';
 
@@ -120,61 +121,31 @@ abstract class ConstructError extends Error {
 
   constructor(msg: string, scope?: IConstruct, name?: string) {
     super(msg);
+
+    const ctr = this.constructor;
+
     Object.setPrototypeOf(this, ConstructError.prototype);
     Object.defineProperty(this, CONSTRUCT_ERROR_SYMBOL, { value: true });
 
-    this.name = name ?? new.target.name;
+    this.name = name ?? ctr.name;
     this.#time = new Date().toISOString();
     this.#constructPath = scope?.node.path;
 
     if (scope) {
-      Error.captureStackTrace(this, scope.constructor);
       try {
-        this.#constructInfo = scope ? constructInfoFromConstruct(scope) : undefined;
+        this.#constructInfo = constructInfoFromConstruct(scope);
       } catch (_) {
         // we don't want to fail if construct info is not available
       }
     }
 
-    const stack = [
-      `${this.name}: ${this.message}`,
-    ];
-
-    if (this.constructInfo) {
-      stack.push(`in ${this.constructInfo?.fqn} at [${this.constructPath}]`);
-    } else {
-      stack.push(`in [${this.constructPath}]`);
-    }
+    // The "stack" field in Node.js includes the error description. If it doesn't, Node will fall back to an
+    // ugly way of rendering the error.
+    this.stack = `${this.name}: ${msg}\n${renderCallStackJustMyCode(captureCallStack(ctr)).join('\n')}`;
 
-    if (this.stack) {
-      stack.push(this.stack);
-    }
-
-    this.stack = this.constructStack(this.stack);
-  }
-
-  /**
-   * Helper message to clean-up the stack and amend with construct information.
-   */
-  private constructStack(prev?: string) {
-    const indent = ' '.repeat(4);
-
-    const stack = [
-      `${this.name}: ${this.message}`,
-    ];
-
-    if (this.constructInfo) {
-      stack.push(`${indent}at path [${this.constructPath}] in ${this.constructInfo?.fqn}`);
-    } else {
-      stack.push(`${indent}at path [${this.constructPath}]`);
-    }
-
-    if (prev) {
-      stack.push('');
-      stack.push(...prev.split('\n').slice(1));
+    if (scope) {
+      this.stack += `\nRelates to construct:\n${renderConstructRootPath(scope)}`;
     }
-
-    return stack.join('\n');
   }
 }
 
@@ -258,3 +229,34 @@ export class ExecutionError extends ConstructError {
     Object.defineProperty(this, EXECUTION_ERROR_SYMBOL, { value: true });
   }
 }
+
+export function renderConstructRootPath(construct: IConstruct) {
+  const rootPath = [];
+
+  let cur: IConstruct | undefined = construct;
+  while (cur !== undefined) {
+    rootPath.push(cur);
+    cur = cur.node.scope;
+  }
+  rootPath.reverse();
+
+  const ret = new Array<string>();
+  for (let i = 0; i < rootPath.length; i++) {
+    const constructId = rootPath[i].node.id || '<.>';
+
+    let suffix = '';
+    try {
+      const constructInfo = constructInfoFromConstruct(rootPath[i]);
+      suffix = ` (${constructInfo?.fqn})`;
+    } catch (_) {
+      // we don't want to fail if construct info is not available
+    }
+
+    const branch = ' └─ ';
+    const indent = i > 0 ? ' '.repeat(branch.length * (i - 1)) + branch : '';
+
+    ret.push(`    ${indent}${constructId}${suffix}`);
+  }
+
+  return ret.join('\n');
+}

packages/aws-cdk-lib/core/lib/stack-trace.ts

@@ -27,16 +27,166 @@ export function captureStackTrace(
     return ['stack traces disabled'];
   }
 
-  const object: { stack?: string } = {};
   const previousLimit = Error.stackTraceLimit;
   try {
     Error.stackTraceLimit = limit;
-    Error.captureStackTrace(object, below);
+    return renderCallStackJustMyCode(captureCallStack(below), false);
   } finally {
     Error.stackTraceLimit = previousLimit;
   }
-  if (!object.stack) {
-    return [];
+}
+
+/**
+ * Capture a call stack using `Error.captureStackTrace`
+ *
+ * Modern Nodes have a `util.getCallSites()` API but:
+ *
+ * - it's heavily unstable; and
+ * - source map support works by hijacking `Error.prepareStackTrace`.
+ *
+ * It's easier for us to render a regular stacktrace as a string, have source map support
+ * do the right thing, and then pick it apart, than to try and reconstruct it.
+ */
+export function captureCallStack(upTo: Function | undefined): CallSite[] {
+  const obj: { stack: string } = {} as any;
+  Error.captureStackTrace(obj, upTo);
+  let trace = parseErrorStack(obj.stack);
+  if (trace.length === 0) {
+    // Protect against a common mistake: if upTo is not in the call stack, `captureStackTrace` will return an empty array.
+    // If that happens, do it again without an `upTo` function.
+    Error.captureStackTrace(obj);
+    trace = parseErrorStack(obj.stack);
   }
-  return object.stack.split('\n').slice(1).map(s => s.replace(/^\s*at\s+/, ''));
+  return trace;
+}
+
+/**
+ * Parse the `error.stack` string into constituent components
+ *
+ * The string looks like this:
+ *
+ * ```
+ * Error: Some Error message
+ * Potentially spread over multiple lines
+ *     at <function> (<file>:<line>:<col>)
+ *     at <function> (<file>:<line>:<col>)
+ *     at <class>.<function> (<file>:<line>:<col>)
+ *     at Object.<anonymous> (<file>:<line>:<col>)
+ *     at <function> [as somethingElse] (<file>:<line>:<col>)
+ *     at new <constructor> (<file>:<line>:<col>)
+ *     at <file>:<line>:<col>
+ * ```
+ *
+ * `<file>` can be `node:internal/modules/whatever`.
+ */
+export function parseErrorStack(stack: string): CallSite[] {
+  const lines = stack.split('\n');
+
+  const framePrefix = '    at ';
+
+  return lines
+    .filter(line => line.startsWith(framePrefix))
+    .map(line => {
+      line = line.slice(framePrefix.length);
+
+      let fileName;
+      let functionName;
+      let sourceLocation;
+
+      // line = <function> (<source>) | <source>
+      const paren = line.indexOf('(');
+      if (paren) {
+        functionName = line.slice(0, paren - 1);
+        line = line.slice(paren + 1, -1);
+      } else {
+        functionName = '<entry>';
+      }
+
+      // Make this easier to read
+      if (functionName === 'Object.<anonymous>') {
+        functionName = '<anonymous>';
+      }
+      let asI = functionName.indexOf(' [as ');
+      if (asI > -1) {
+        functionName = functionName.slice(0, asI);
+      }
+
+      // line = <file>:<line>:<col>, but file can contain : as well.
+      // Grab at most 2 groups of only digits from the end of the string for source location
+      const m = line.match(/(:[0-9]+){0,2}$/);
+
+      fileName = m ? line.slice(0, -m[0].length) : line;
+      sourceLocation = m ? m[0].slice(1) : '';
+
+      return {
+        fileName,
+        functionName,
+        sourceLocation,
+      };
+    });
+}
+
+/**
+ * Renders an array of CallSites nicely, focusing on the user application code
+ *
+ * We detect "Not My Code" using the following heuristics:
+ *
+ * - If there is '/node_modules/' in the file path, we assume the call stack is a library and we skip it.
+ * - If there is 'node:' in the file path, we assume it is NodeJS internals and we skip it.
+ */
+export function renderCallStackJustMyCode(stack: CallSite[], indent = true): string[] {
+  const moduleRe = /(\/|\\)node_modules(\/|\\)([^/\\]+)/;
+
+  const lines = [];
+  let skipped = new Array<{ functionName?: string; fileName: string }>();
+
+  let i = 0;
+  while (i < stack.length) {
+    const frame = stack[i++];
+
+    const pat = frame.fileName.match(moduleRe);
+    if (pat) {
+      while (i < stack.length && stack[i].fileName.includes(pat[0])) {
+        i++;
+      }
+      // The last stack frame has the function that user code call into.
+      skip({ functionName: stack[i - 1].functionName, fileName: pat[3] });
+    } else if (frame.fileName.includes('node:')) {
+      skip({ fileName: 'node internals' });
+      while (i < stack.length && stack[i].fileName.includes('node:')) {
+        i++;
+      }
+    } else {
+      reportSkipped(true);
+      const prefix = indent ? '    at ' : '';
+      lines.push(`${prefix}${frame.functionName} (${frame.fileName}:${frame.sourceLocation})`);
+    }
+  }
+  reportSkipped(false);
+  return lines;
+
+  function skip(what: typeof skipped[number]) {
+    if (!skipped.find(x => x.fileName === what.fileName && x.functionName === what.functionName)) {
+      skipped.push(what);
+    }
+  }
+
+  function reportSkipped(includeFunction: boolean) {
+    if (skipped.length > 0) {
+      const rendered = skipped.map((s, j) =>
+        // Only render the function name of the last frame before user code
+        j === skipped.length - 1 && s.functionName && includeFunction ?
+          `${s.functionName} in ${s.fileName}` : s.fileName);
+
+      const prefix = indent ? '    ' : '';
+      lines.push(`${prefix}...${rendered.join(', ')}...`);
+    }
+    skipped = [];
+  }
+}
+
+interface CallSite {
+  functionName: string;
+  fileName: string;
+  sourceLocation: string;
 }