Skip to content

Query hot reloading during development #1803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { Disposable } from "@cursorless/common";
import { Disposable, showError } from "@cursorless/common";
import { pull } from "lodash";
import {
IterationScopeChangeEventCallback,
IterationScopeRangeConfig,
ScopeChangeEventCallback,
ScopeRangeConfig,
ScopeRanges,
} from "..";
import { Debouncer } from "../core/Debouncer";
import { LanguageDefinitions } from "../languages/LanguageDefinitions";
import { ide } from "../singletons/ide.singleton";
import { ScopeRangeProvider } from "./ScopeRangeProvider";

Expand All @@ -19,7 +21,10 @@ export class ScopeRangeWatcher {
private debouncer = new Debouncer(() => this.onChange());
private listeners: (() => void)[] = [];

constructor(private scopeRangeProvider: ScopeRangeProvider) {
constructor(
languageDefinitions: LanguageDefinitions,
private scopeRangeProvider: ScopeRangeProvider,
) {
this.disposables.push(
// An Event which fires when the array of visible editors has changed.
ide().onDidChangeVisibleTextEditors(this.debouncer.run),
Expand All @@ -32,6 +37,7 @@ export class ScopeRangeWatcher {
// dirty-state changes.
ide().onDidChangeTextDocument(this.debouncer.run),
ide().onDidChangeTextEditorVisibleRanges(this.debouncer.run),
languageDefinitions.onDidChangeDefinition(this.debouncer.run),
this.debouncer,
);

Expand All @@ -54,10 +60,31 @@ export class ScopeRangeWatcher {
): Disposable {
const fn = () => {
ide().visibleTextEditors.forEach((editor) => {
callback(
editor,
this.scopeRangeProvider.provideScopeRanges(editor, config),
);
let scopeRanges: ScopeRanges[];
try {
scopeRanges = this.scopeRangeProvider.provideScopeRanges(
editor,
config,
);
} catch (err) {
showError(
ide().messages,
"ScopeRangeWatcher.provide",
(err as Error).message,
);
// If there was a problem getting scopes for an editor, we show an
// error and clear any scopes we might have shown last time. This is
// especially important during development, but also seems like the
// robust thing to do generally.
scopeRanges = [];

if (ide().runMode === "test") {
// Fail hard if we're in test mode; otherwise recover
throw err;
}
}

callback(editor, scopeRanges);
});
};

Expand Down
10 changes: 7 additions & 3 deletions packages/cursorless-engine/src/cursorlessEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ export function createCursorlessEngine(
const debug = new Debug(treeSitter);

const rangeUpdater = new RangeUpdater();
ide.disposeOnExit(rangeUpdater);

const snippets = new Snippets();
snippets.init();
Expand All @@ -52,7 +51,9 @@ export function createCursorlessEngine(

const testCaseRecorder = new TestCaseRecorder(hatTokenMap, storedTargets);

const languageDefinitions = new LanguageDefinitions(treeSitter);
const languageDefinitions = new LanguageDefinitions(fileSystem, treeSitter);

ide.disposeOnExit(rangeUpdater, languageDefinitions, hatTokenMap, debug);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops; looks like we forgot to dispose a bunch of stuff 😅


return {
commandApi: {
Expand Down Expand Up @@ -108,7 +109,10 @@ function createScopeProvider(
),
);

const rangeWatcher = new ScopeRangeWatcher(rangeProvider);
const rangeWatcher = new ScopeRangeWatcher(
languageDefinitions,
rangeProvider,
);
const supportChecker = new ScopeSupportChecker(scopeHandlerFactory);

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,10 @@ export class LanguageDefinition {
*/
static create(
treeSitter: TreeSitter,
queryDir: string,
languageId: string,
): LanguageDefinition | undefined {
const languageQueryPath = join(
ide().assetsRoot,
"queries",
`${languageId}.scm`,
);
const languageQueryPath = join(queryDir, `${languageId}.scm`);

if (!existsSync(languageQueryPath)) {
return undefined;
Expand Down
46 changes: 43 additions & 3 deletions packages/cursorless-engine/src/languages/LanguageDefinitions.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
import { Range, TextDocument } from "@cursorless/common";
import {
Disposable,
FileSystem,
Notifier,
Range,
TextDocument,
getCursorlessRepoRoot,
} from "@cursorless/common";
import { join } from "path";
import { SyntaxNode } from "web-tree-sitter";
import { TreeSitter } from "..";
import { ide } from "../singletons/ide.singleton";
import { LanguageDefinition } from "./LanguageDefinition";

/**
Expand All @@ -14,6 +23,8 @@ const LANGUAGE_UNDEFINED = Symbol("LANGUAGE_UNDEFINED");
* constructing them as necessary
*/
export class LanguageDefinitions {
private notifier: Notifier = new Notifier();

/**
* Maps from language id to {@link LanguageDefinition} or
* {@link LANGUAGE_UNDEFINED} if language doesn't have new-style definitions.
Expand All @@ -28,8 +39,31 @@ export class LanguageDefinitions {
string,
LanguageDefinition | typeof LANGUAGE_UNDEFINED
> = new Map();
private queryDir: string;
private disposables: Disposable[] = [];

constructor(
fileSystem: FileSystem,
private treeSitter: TreeSitter,
) {
// Use the repo root as the root for development mode, so that we can
// we can make hot-reloading work for the queries
this.queryDir = join(
ide().runMode === "development"
? getCursorlessRepoRoot()
: ide().assetsRoot,
"queries",
);

constructor(private treeSitter: TreeSitter) {}
if (ide().runMode === "development") {
this.disposables.push(
fileSystem.watchDir(this.queryDir, () => {
this.languageDefinitions.clear();
this.notifier.notifyListeners();
}),
);
}
}

/**
* Get a language definition for the given language id, if the language
Expand All @@ -44,7 +78,7 @@ export class LanguageDefinitions {

if (definition == null) {
definition =
LanguageDefinition.create(this.treeSitter, languageId) ??
LanguageDefinition.create(this.treeSitter, this.queryDir, languageId) ??
LANGUAGE_UNDEFINED;

this.languageDefinitions.set(languageId, definition);
Expand All @@ -59,4 +93,10 @@ export class LanguageDefinitions {
public getNodeAtLocation(document: TextDocument, range: Range): SyntaxNode {
return this.treeSitter.getNodeAtLocation(document, range);
}

onDidChangeDefinition = this.notifier.registerListener;

dispose() {
this.disposables.forEach((disposable) => disposable.dispose());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
Position,
TextDocument,
TextEditor,
showError,
} from "@cursorless/common";
import { uniqWith } from "lodash";
import { TreeSitterQuery } from "../../../../languages/TreeSitterQuery";
Expand All @@ -15,6 +16,7 @@ import {
ScopeIteratorRequirements,
} from "../scopeHandler.types";
import { mergeAdjacentBy } from "./mergeAdjacentBy";
import { ide } from "../../../../singletons/ide.singleton";

/** Base scope handler to use for both tree-sitter scopes and their iteration scopes */
export abstract class BaseTreeSitterScopeHandler extends BaseScopeHandler {
Expand Down Expand Up @@ -65,9 +67,18 @@ export abstract class BaseTreeSitterScopeHandler extends BaseScopeHandler {
targets.length > 1 &&
!equivalentScopes.every((scope) => scope.allowMultiple)
) {
throw Error(
"Please use #allow-multiple! predicate in your query to allow multiple matches for this scope type",
const message =
"Please use #allow-multiple! predicate in your query to allow multiple matches for this scope type";

showError(
ide().messages,
"BaseTreeSitterScopeHandler.allow-multiple",
message,
);

if (ide().runMode === "test") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Development is much easier if you can see the scopes that allow-multiple is complaining about. If you fail, you can't see anything so it's really hard to debug

throw Error(message);
}
}

return targets;
Expand Down