inline-completion-debt

src/vs/base/common/observableImpl/autorun.ts

@@ -3,32 +3,24 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
+import { BugIndicatingError } from 'vs/base/common/errors';
 import { DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifecycle';
-import { IReader, IObservable, IObserver } from 'vs/base/common/observableImpl/base';
+import { IReader, IObservable, IObserver, IChangeContext } from 'vs/base/common/observableImpl/base';
 import { getLogger } from 'vs/base/common/observableImpl/logging';
 
 export function autorun(debugName: string, fn: (reader: IReader) => void): IDisposable {
-	return new AutorunObserver(debugName, fn, undefined);
+	return new AutorunObserver(debugName, fn, undefined, undefined);
 }
 
-interface IChangeContext {
-	readonly changedObservable: IObservable<any, any>;
-	readonly change: unknown;
-
-	didChange<T, TChange>(observable: IObservable<T, TChange>): this is { change: TChange };
-}
-
-export function autorunHandleChanges(
+export function autorunHandleChanges<TChangeSummary>(
 	debugName: string,
 	options: {
-		/**
-		 * Returns if this change should cause a re-run of the autorun.
-		*/
-		handleChange: (context: IChangeContext) => boolean;
+		createEmptyChangeSummary?: () => TChangeSummary;
+		handleChange: (context: IChangeContext, changeSummary: TChangeSummary) => boolean;
 	},
-	fn: (reader: IReader) => void
+	fn: (reader: IReader, changeSummary: TChangeSummary) => void
 ): IDisposable {
-	return new AutorunObserver(debugName, fn, options.handleChange);
+	return new AutorunObserver(debugName, fn, options.createEmptyChangeSummary, options.handleChange);
 }
 
 export function autorunWithStore(
@@ -63,18 +55,21 @@ const enum AutorunState {
 	upToDate = 3,
 }
 
-export class AutorunObserver implements IObserver, IReader, IDisposable {
+export class AutorunObserver<TChangeSummary = any> implements IObserver, IReader, IDisposable {
 	private state = AutorunState.stale;
 	private updateCount = 0;
 	private disposed = false;
 	private dependencies = new Set<IObservable<any>>();
 	private dependenciesToBeRemoved = new Set<IObservable<any>>();
+	private changeSummary: TChangeSummary | undefined;
 
 	constructor(
 		public readonly debugName: string,
-		private readonly runFn: (reader: IReader) => void,
-		private readonly _handleChange: ((context: IChangeContext) => boolean) | undefined
+		private readonly runFn: (reader: IReader, changeSummary: TChangeSummary) => void,
+		private readonly createChangeSummary: (() => TChangeSummary) | undefined,
+		private readonly _handleChange: ((context: IChangeContext, summary: TChangeSummary) => boolean) | undefined,
 	) {
+		this.changeSummary = this.createChangeSummary?.();
 		getLogger()?.handleAutorunCreated(this);
 		this._runIfNeeded();
 	}
@@ -101,7 +96,9 @@ export class AutorunObserver implements IObserver, IReader, IDisposable {
 		getLogger()?.handleAutorunTriggered(this);
 
 		try {
-			this.runFn(this);
+			const changeSummary = this.changeSummary!;
+			this.changeSummary = this.createChangeSummary?.();
+			this.runFn(this, changeSummary);
 		} finally {
 			// We don't want our observed observables to think that they are (not even temporarily) not being observed.
 			// Thus, we only unsubscribe from observables that are definitely not read anymore.
@@ -142,6 +139,10 @@ export class AutorunObserver implements IObserver, IReader, IDisposable {
 			} while (this.state !== AutorunState.upToDate);
 		}
 		this.updateCount--;
+
+		if (this.updateCount < 0) {
+			throw new BugIndicatingError();
+		}
 	}
 
 	public handlePossibleChange(observable: IObservable<any>): void {
@@ -156,7 +157,7 @@ export class AutorunObserver implements IObserver, IReader, IDisposable {
 				changedObservable: observable,
 				change,
 				didChange: o => o === observable as any,
-			}) : true;
+			}, this.changeSummary!) : true;
 			if (shouldReact) {
 				this.state = AutorunState.stale;
 			}

src/vs/base/common/observableImpl/base.ts

@@ -171,7 +171,6 @@ export abstract class ConvenientObservable<T, TChange> implements IObservable<T,
 export abstract class BaseObservable<T, TChange = void> extends ConvenientObservable<T, TChange> {
 	protected readonly observers = new Set<IObserver>();
 
-	/** @sealed */
 	public addObserver(observer: IObserver): void {
 		const len = this.observers.size;
 		this.observers.add(observer);
@@ -180,7 +179,6 @@ export abstract class BaseObservable<T, TChange = void> extends ConvenientObserv
 		}
 	}
 
-	/** @sealed */
 	public removeObserver(observer: IObserver): void {
 		const deleted = this.observers.delete(observer);
 		if (deleted && this.observers.size === 0) {
@@ -203,6 +201,14 @@ export function transaction(fn: (tx: ITransaction) => void, getDebugName?: () =>
 	}
 }
 
+export function subtransaction(tx: ITransaction | undefined, fn: (tx: ITransaction) => void, getDebugName?: () => string): void {
+	if (!tx) {
+		transaction(fn, getDebugName);
+	} else {
+		fn(tx);
+	}
+}
+
 export class TransactionImpl implements ITransaction {
 	private updatingObservers: { observer: IObserver; observable: IObservable<any> }[] | null = [];
 
@@ -313,3 +319,18 @@ export class DisposableObservableValue<T extends IDisposable | undefined, TChang
 		this._value?.dispose();
 	}
 }
+
+export interface IChangeContext {
+	readonly changedObservable: IObservable<any, any>;
+	readonly change: unknown;
+
+	didChange<T, TChange>(observable: IObservable<T, TChange>): this is { change: TChange };
+}
+
+export interface IChangeTracker {
+	/**
+	 * Returns if this change should cause an invalidation.
+	 * Can record the changes to just process deltas.
+	*/
+	handleChange(context: IChangeContext): boolean;
+}

src/vs/base/common/observableImpl/derived.ts

@@ -4,11 +4,21 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { BugIndicatingError } from 'vs/base/common/errors';
-import { IReader, IObservable, BaseObservable, IObserver, _setDerived } from 'vs/base/common/observableImpl/base';
+import { IReader, IObservable, BaseObservable, IObserver, _setDerived, IChangeContext } from 'vs/base/common/observableImpl/base';
 import { getLogger } from 'vs/base/common/observableImpl/logging';
 
 export function derived<T>(debugName: string | (() => string), computeFn: (reader: IReader) => T): IObservable<T> {
-	return new Derived(debugName, computeFn);
+	return new Derived(debugName, computeFn, undefined, undefined);
+}
+
+export function derivedHandleChanges<T, TChangeSummary>(
+	debugName: string | (() => string),
+	options: {
+		createEmptyChangeSummary: () => TChangeSummary;
+		handleChange: (context: IChangeContext, changeSummary: TChangeSummary) => boolean;
+	},
+	computeFn: (reader: IReader, changeSummary: TChangeSummary) => T): IObservable<T> {
+	return new Derived(debugName, computeFn, options.createEmptyChangeSummary, options.handleChange);
 }
 
 _setDerived(derived);
@@ -35,23 +45,26 @@ const enum DerivedState {
 	upToDate = 3,
 }
 
-export class Derived<T> extends BaseObservable<T, void> implements IReader, IObserver {
+export class Derived<T, TChangeSummary = any> extends BaseObservable<T, void> implements IReader, IObserver {
 	private state = DerivedState.initial;
 	private value: T | undefined = undefined;
 	private updateCount = 0;
 	private dependencies = new Set<IObservable<any>>();
 	private dependenciesToBeRemoved = new Set<IObservable<any>>();
+	private changeSummary: TChangeSummary | undefined = undefined;
 
 	public override get debugName(): string {
 		return typeof this._debugName === 'function' ? this._debugName() : this._debugName;
 	}
 
 	constructor(
 		private readonly _debugName: string | (() => string),
-		private readonly computeFn: (reader: IReader) => T
+		private readonly computeFn: (reader: IReader, changeSummary: TChangeSummary) => T,
+		private readonly createChangeSummary: (() => TChangeSummary) | undefined,
+		private readonly _handleChange: ((context: IChangeContext, summary: TChangeSummary) => boolean) | undefined,
 	) {
 		super();
-
+		this.changeSummary = this.createChangeSummary?.();
 		getLogger()?.handleDerivedCreated(this);
 	}
 
@@ -72,7 +85,7 @@ export class Derived<T> extends BaseObservable<T, void> implements IReader, IObs
 		if (this.observers.size === 0) {
 			// Without observers, we don't know when to clean up stuff.
 			// Thus, we don't cache anything to prevent memory leaks.
-			const result = this.computeFn(this);
+			const result = this.computeFn(this, this.createChangeSummary?.()!);
 			// Clear new dependencies
 			this.onLastObserverRemoved();
 			return result;
@@ -113,9 +126,11 @@ export class Derived<T> extends BaseObservable<T, void> implements IReader, IObs
 		const oldValue = this.value;
 		this.state = DerivedState.upToDate;
 
+		const changeSummary = this.changeSummary!;
+		this.changeSummary = this.createChangeSummary?.();
 		try {
 			/** might call {@link handleChange} indirectly, which could invalidate us */
-			this.value = this.computeFn(this);
+			this.value = this.computeFn(this, changeSummary);
 		} finally {
 			// We don't want our observed observables to think that they are (not even temporarily) not being observed.
 			// Thus, we only unsubscribe from observables that are definitely not read anymore.
@@ -146,7 +161,7 @@ export class Derived<T> extends BaseObservable<T, void> implements IReader, IObs
 	}
 
 	// IObserver Implementation
-	public beginUpdate(): void {
+	public beginUpdate<T>(_observable: IObservable<T>): void {
 		this.updateCount++;
 		const propagateBeginUpdate = this.updateCount === 1;
 		if (this.state === DerivedState.upToDate) {
@@ -165,7 +180,7 @@ export class Derived<T> extends BaseObservable<T, void> implements IReader, IObs
 		}
 	}
 
-	public endUpdate(): void {
+	public endUpdate<T>(_observable: IObservable<T>): void {
 		this.updateCount--;
 		if (this.updateCount === 0) {
 			// End update could change the observer list.
@@ -189,9 +204,19 @@ export class Derived<T> extends BaseObservable<T, void> implements IReader, IObs
 		}
 	}
 
-	public handleChange<T, TChange>(observable: IObservable<T, TChange>, _change: TChange): void {
+	public handleChange<T, TChange>(observable: IObservable<T, TChange>, change: TChange): void {
 		const isUpToDate = this.state === DerivedState.upToDate;
-		if ((this.state === DerivedState.dependenciesMightHaveChanged || isUpToDate) && this.dependencies.has(observable)) {
+		let shouldReact = true;
+
+		if (this._handleChange && this.dependencies.has(observable)) {
+			shouldReact = this._handleChange({
+				changedObservable: observable,
+				change,
+				didChange: o => o === observable as any,
+			}, this.changeSummary!);
+		}
+
+		if (shouldReact && (this.state === DerivedState.dependenciesMightHaveChanged || isUpToDate) && this.dependencies.has(observable)) {
 			this.state = DerivedState.stale;
 			if (isUpToDate) {
 				for (const r of this.observers) {
@@ -212,4 +237,18 @@ export class Derived<T> extends BaseObservable<T, void> implements IReader, IObs
 		this.dependenciesToBeRemoved.delete(observable);
 		return value;
 	}
+
+	public override addObserver(observer: IObserver): void {
+		if (!this.observers.has(observer) && this.updateCount > 0) {
+			observer.beginUpdate(this);
+		}
+		super.addObserver(observer);
+	}
+
+	public override removeObserver(observer: IObserver): void {
+		if (this.observers.has(observer) && this.updateCount > 0) {
+			observer.endUpdate(this);
+		}
+		super.removeObserver(observer);
+	}
 }

src/vs/base/common/observableImpl/utils.ts

@@ -276,28 +276,36 @@ export function wasEventTriggeredRecently(event: Event<any>, timeoutMs: number,
 }
 
 /**
- * This ensures the observable cache is kept up-to-date, even if there are no subscribers.
- * This is useful when the observables `get` method is used, but not its `read` method.
+ * This ensures the observable is being observed.
+ * Observed observables (such as {@link derived}s) can maintain a cache, as they receive invalidation events.
+ * Unobserved observables are forced to recompute their value from scratch every time they are read.
  *
- * (Usually, when no one is actually observing the observable, getting its value will
- * compute it from scratch, as the cache cannot be trusted:
- * Because no one is actually observing its value, keeping the cache up-to-date would be too expensive)
+ * @param observable the observable to keep alive
+ * @param forceRecompute if true, the observable will be eagerly recomputed after it changed.
+ * Use this if recomputing the observables causes side-effects.
 */
-export function keepAlive(observable: IObservable<any>): IDisposable {
-	const o = new KeepAliveObserver();
+export function keepAlive(observable: IObservable<any>, forceRecompute?: boolean): IDisposable {
+	const o = new KeepAliveObserver(forceRecompute ?? false);
 	observable.addObserver(o);
 	return toDisposable(() => {
 		observable.removeObserver(o);
 	});
 }
 
 class KeepAliveObserver implements IObserver {
+	private counter = 0;
+
+	constructor(private readonly forceRecompute: boolean) { }
+
 	beginUpdate<T>(observable: IObservable<T, void>): void {
-		// NO OP
+		this.counter++;
 	}
 
 	endUpdate<T>(observable: IObservable<T, void>): void {
-		// NO OP
+		this.counter--;
+		if (this.counter === 0 && this.forceRecompute) {
+			observable.reportChanges();
+		}
 	}
 
 	handlePossibleChange<T>(observable: IObservable<T, unknown>): void {

src/vs/base/test/common/observable.test.ts

@@ -926,6 +926,42 @@ suite('observables', () => {
 			'myAutorun(myDerived3: 1 + 0)',
 		]);
 	});
+
+	test('bug: Add observable in endUpdate', () => {
+		const myObservable1 = observableValue('myObservable1', 0);
+		const myObservable2 = observableValue('myObservable2', 0);
+
+		const myDerived1 = derived('myDerived1', reader => {
+			return myObservable1.read(reader);
+		});
+
+		const myDerived2 = derived('myDerived2', reader => {
+			return myObservable2.read(reader);
+		});
+
+		const myDerivedA1 = derived('myDerivedA1', reader => {
+			const d1 = myDerived1.read(reader);
+			if (d1 === 1) {
+				// This adds an observer while myDerived is still in update mode.
+				// When myDerived exits update mode, the observer shouldn't receive
+				// more endUpdate than beginUpdate calls.
+				myDerived2.read(reader);
+			}
+		});
+
+		autorun('myAutorun1', reader => {
+			myDerivedA1.read(reader);
+		});
+
+		autorun('myAutorun2', reader => {
+			myDerived2.read(reader);
+		});
+
+		transaction(tx => {
+			myObservable1.set(1, tx);
+			myObservable2.set(1, tx);
+		});
+	});
 });
 
 export class LoggingObserver implements IObserver {