From e3d1212ecd8818fdc63283f1bac8409f3ac954cd Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Mon, 30 Sep 2024 14:39:35 -0400 Subject: [PATCH] Ensure `addActivityItemStream()` doesn't lose `Output` vs `Error` stream specialization (#4848) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses https://github.com/posit-dev/positron/issues/4519 In `addActivityItemStream()`, either a `ActivityItemOutputStream` or `ActivityItemErrorStream` always goes in, but it was possible for a "generic" `ActivityItemStream` to come out here when we make one from the `remainderText` after the last newline: https://github.com/posit-dev/positron/blob/e51332d5baa59b72f5e8b28861bbb751f25452ea/src/vs/workbench/services/positronConsole/browser/classes/activityItemStream.ts#L118-L136 That generic `ActivityItemStream` would get set here: https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts#L110-L116 And then pushed onto `this._activityItems` https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts#L138-L139 The problem is that our Console code doesn't know how to deal with a generic `ActivityItemStream`. It must be a specialized `ActivityItemOutputStream` or `ActivityItemErrorStream`! https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/contrib/positronConsole/browser/components/runtimeActivity.tsx#L46-L49 If a generic `ActivityItemStream` slipped through, the output would never appear in the Console, making it look "dropped", as reported in https://github.com/posit-dev/positron/issues/4519. --- I like how we have a generic `ActivityItemStream` class that we specialize for the output/error types, so I've fixed this bug by using [polymorphic `this`](https://www.typescriptlang.org/docs/handbook/advanced-types.html#polymorphic-this-types) to allow `addActivityItemStream()` to take and return objects of the correct specialized type. Ideally we'd simply be able to use `new this.constructor()` in place of `new ActivityItemStream()` to use the polymorphic constructor when we need to create new objects, but due to some [well known weirdness](https://github.com/microsoft/TypeScript/issues/3841) in typescript, you have to manually cast it to the constructor type, so `newActivityItemStream()` wraps that up. I've also made the `ActivityItemStream` constructor `protected` to avoid us using it by accident in the codebase. We should _always_ be creating `ActivityItemOutputStream` or `ActivityItemErrorStream` instead. Before 😢 https://github.com/user-attachments/assets/b1854d9e-94eb-4952-aead-bebcf5c5d6a2 After 🥳 https://github.com/user-attachments/assets/4a76974c-6862-4e84-a296-620717ddb8da --- .../browser/classes/activityItemStream.ts | 59 ++++++++++++++++--- 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/src/vs/workbench/services/positronConsole/browser/classes/activityItemStream.ts b/src/vs/workbench/services/positronConsole/browser/classes/activityItemStream.ts index b007869b444..c23962ef3ec 100644 --- a/src/vs/workbench/services/positronConsole/browser/classes/activityItemStream.ts +++ b/src/vs/workbench/services/positronConsole/browser/classes/activityItemStream.ts @@ -19,7 +19,7 @@ export class ActivityItemStream { /** * Gets the ActivityItemStream array. */ - private activityItemStreams: ActivityItemStream[] = []; + private activityItemStreams: this[] = []; /** * Gets the ANSIOutput that is processing this ActivityItemStream. @@ -47,12 +47,17 @@ export class ActivityItemStream { /** * Constructor. + * + * Never to be called directly. + * Internally, use `newActivityItemStream()` instead. + * Externally, use `ActivityItemOutputStream` or `ActivityItemErrorStream` constructors instead. + * * @param id The identifier. * @param parentId The parent identifier. * @param when The date. * @param text The text. */ - constructor( + protected constructor( readonly id: string, readonly parentId: string, readonly when: Date, @@ -70,9 +75,7 @@ export class ActivityItemStream { * @param activityItemStream The ActivityItemStream to add. * @returns The remainder ActivityItemStream, or undefined. */ - public addActivityItemStream( - activityItemStream: ActivityItemStream - ): ActivityItemStream | undefined { + public addActivityItemStream(activityItemStream: this): this | undefined { // If this ActivityItemStream is terminated, copy its styles to the ActivityItemStream being // added and return it as the remainder ActivityItemStream to be processed. if (this.terminated) { @@ -95,7 +98,7 @@ export class ActivityItemStream { const remainderText = activityItemStream.text.substring(newlineIndex + 1); // Add an ActivityItemStream with the text containing the newline. - this.activityItemStreams.push(new ActivityItemStream( + this.activityItemStreams.push(this.newActivityItemStream( activityItemStream.id, activityItemStream.parentId, activityItemStream.when, @@ -116,7 +119,7 @@ export class ActivityItemStream { } // Create the remainder ActivityItemStream. - activityItemStream = new ActivityItemStream( + activityItemStream = this.newActivityItemStream( activityItemStream.id, activityItemStream.parentId, activityItemStream.when, @@ -153,15 +156,53 @@ export class ActivityItemStream { } } + /** + * Polymorphic constructor for internal creation of new `ActivityItemStream`s + * + * Uses polymorphic `this` to actually return extension class types, like + * `ActivityItemOutputStream` and `ActivityItemErrorStream`. + * + * Note that we have to manually cast `this.constructor()` to the right type, as otherwise + * it is just a generic `Function`. + * https://github.com/microsoft/TypeScript/issues/3841 + * https://stackoverflow.com/questions/64638771/how-can-i-create-a-new-instance-of-a-class-using-this-from-within-method + * + * @param id The identifier. + * @param parentId The parent identifier. + * @param when The date. + * @param text The text. + * @returns A newly constructed activity item stream of type `this`. + */ + private newActivityItemStream( + id: string, + parentId: string, + when: Date, + text: string + ): this { + const constructor = ( + this.constructor as + new (id: string, parentId: string, when: Date, text: string) => this + ); + return new constructor(id, parentId, when, text); + } + //#endregion Private Methods } /** * ActivityItemOutputStream class. */ -export class ActivityItemOutputStream extends ActivityItemStream { } +export class ActivityItemOutputStream extends ActivityItemStream { + constructor(id: string, parentId: string, when: Date, text: string) { + super(id, parentId, when, text); + } +} /** * ActivityItemErrorStream class. */ -export class ActivityItemErrorStream extends ActivityItemStream { } +export class ActivityItemErrorStream extends ActivityItemStream { + constructor(id: string, parentId: string, when: Date, text: string) { + super(id, parentId, when, text); + } +}