Multi-root workspace support and Fix TextMate grammar bugs#276
Multi-root workspace support and Fix TextMate grammar bugs#276mattmasson wants to merge 6 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes several Power Query TextMate grammar tokenization issues and adds a dedicated grammar test suite, while also extending the symbol-library system to better support multi-root workspaces and detect library symbol collisions.
Changes:
- Fix TextMate grammar rules for dotted identifiers, comparison operators (
<=,>=), null-coalescing (??), and semicolon punctuators. - Add a TextMate grammar test harness and a broad set of grammar tokenization tests under
server/src/test/grammar/. - Add multi-root aware symbol registration (namespacing by workspace folder) plus server-side overlap/overwrite logging and new overlap-detection utilities.
Reviewed changes
Copilot reviewed 30 out of 34 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| syntaxes/powerquery.tmLanguage.json | Fixes identifier/operator/punctuation regexes and adds new scopes for ?? and ;. |
| server/tsconfig.json | Adds ts-node-scoped skipLibCheck to work around type issues in dev tooling. |
| server/src/test/standardLibrary.test.ts | Adds unit tests for new external-library overlap detection helpers. |
| server/src/test/grammar/types.test.ts | Adds grammar coverage for primitive types and modifiers. |
| server/src/test/grammar/operators.test.ts | Adds grammar coverage for operators/punctuators including ?? and ;. |
| server/src/test/grammar/literals.test.ts | Adds grammar coverage for numeric/string/null literals and multiline strings. |
| server/src/test/grammar/keywords.test.ts | Adds grammar coverage for keywords and #keyword forms. |
| server/src/test/grammar/identifiers.test.ts | Adds grammar coverage for dotted/quoted/inclusive/intrinsic identifiers. |
| server/src/test/grammar/grammarTestHelper.ts | Introduces TextMate + Oniguruma harness for tokenizing lines/streams in tests. |
| server/src/test/grammar/expressions.test.ts | Adds grammar coverage for common M expression patterns and real-world snippets. |
| server/src/test/grammar/comments.test.ts | Adds grammar coverage for line/block comments and edge cases. |
| server/src/server.ts | Enables workspace folder support; adds logging around external library registration/overlaps. |
| server/src/library/externalLibraryUtils.ts | Adds APIs to list registered modules and detect cross-module symbol overlaps. |
| server/package.json | Adds vscode-textmate and vscode-oniguruma devDependencies for grammar tests. |
| server/package-lock.json | Locks new server devDependencies and incidental transitive updates. |
| server/.mocharc.json | Expands Mocha discovery to include compiled JS grammar tests under lib/. |
| scripts/package-lock.json | Updates transitive lockfile entries (brace-expansion, etc.). |
| package.json | Changes additionalSymbolsDirectories setting scope to resource for per-folder config. |
| package-lock.json | Updates transitive lockfile entries (brace-expansion, etc.). |
| client/src/test/suite/multiRootWorkspace.test.ts | Adds VS Code UI tests exercising multi-root workspace symbol isolation behavior. |
| client/src/test/suite/librarySymbolManager.test.ts | Updates/extends tests for folder-namespaced module keys. |
| client/src/test/multiRootFixture/workspaceB/symbols/ConnectorB.json | Adds workspace B symbol fixture for multi-root tests. |
| client/src/test/multiRootFixture/workspaceB/sample.pq | Adds workspace B sample PQ file for the fixture workspace. |
| client/src/test/multiRootFixture/workspaceB/.vscode/settings.json | Fixture settings for workspace B. |
| client/src/test/multiRootFixture/workspaceA/symbols/ConnectorA.json | Adds workspace A symbol fixture for multi-root tests. |
| client/src/test/multiRootFixture/workspaceA/sample.pq | Adds workspace A sample PQ file for the fixture workspace. |
| client/src/test/multiRootFixture/workspaceA/.vscode/settings.json | Fixture settings for workspace A. |
| client/src/test/multiRootFixture/test.code-workspace | Adds multi-root .code-workspace fixture referencing workspace A/B. |
| client/src/librarySymbolManager.ts | Implements per-folder symbol registration/removal via namespaced module keys. |
| client/src/extension.ts | Refreshes symbol directories per workspace folder and reacts to folder add/remove events. |
| client/package-lock.json | Updates client lockfile (lodash/brace-expansion/picomatch, etc.). |
| client/.vscode-test.js | Splits UI tests into single-root vs multi-root configurations. |
| .github/copilot-instructions.md | Adds updated repository instructions (build/test/architecture/conventions). |
| .github/.copilot.instructions.md | Removes legacy Copilot instructions file. |
Files not reviewed (3)
- client/package-lock.json: Language not supported
- scripts/package-lock.json: Language not supported
- server/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
configureAllFolderSymbolDirectories can register symbols under the global folder key when no workspace folders are present, but those global registrations are never cleared if/when workspace folders are later added. That can leave stale global modules registered on the server alongside per-folder modules (and can cause false “overlap” warnings / duplicated symbols). Consider clearing the global registrations when workspaceFolders becomes non-empty (e.g., call librarySymbolManager.removeSymbolsForFolder(GlobalFolderKey) before registering per-folder symbols, or call removeAllSymbols() and re-register).
| // Clear any previously-registered global or stale folder symbols before | |
| // rebuilding the current workspace-folder registrations. | |
| await librarySymbolManager.removeAllSymbols(); |
| workspaceFolders: { | ||
| supported: false, | ||
| supported: true, | ||
| changeNotifications: true, |
There was a problem hiding this comment.
The server advertises workspace.workspaceFolders.changeNotifications: true but there’s no handler for workspace-folder change notifications (e.g. connection.workspace.onDidChangeWorkspaceFolders). If the client sends these notifications, they’ll be ignored and the capability is misleading. Either implement the notification handler (and update any folder-scoped state as needed) or set changeNotifications to false/omit it.
| changeNotifications: true, | |
| changeNotifications: false, |
| for (const [moduleName, symbols] of externalLibraryByName) { | ||
| for (const symbol of symbols) { | ||
| const modules: string[] = symbolToModules.get(symbol.name) ?? []; | ||
| modules.push(moduleName); | ||
| symbolToModules.set(symbol.name, modules); | ||
| } | ||
| } | ||
|
|
||
| const overlaps: Map<string, ReadonlyArray<string>> = new Map(); | ||
|
|
||
| for (const [symbolName, modules] of symbolToModules) { | ||
| if (modules.length > 1) { | ||
| overlaps.set(symbolName, modules); | ||
| } |
There was a problem hiding this comment.
getOverlappingSymbols can report false overlaps if a single module contains the same symbol name more than once (the module name will be pushed multiple times, making modules.length > 1 even though there’s only one module involved). Consider tracking modules per symbol with a Set (and/or de-duplicating the modules array) before deciding an overlap exists.
| "extensions": ["ts"], | ||
| "spec": "src/test/**/*.test.ts" | ||
| "extensions": ["ts", "js"], | ||
| "spec": ["src/test/*.test.ts", "lib/test/grammar/**/*.test.js"] |
There was a problem hiding this comment.
Mocha is now configured to run the grammar tests from lib/test/grammar/**/*.test.js, which means they won’t run unless the server package has been built (the default server test script only runs mocha). If these tests are expected to run in CI by default, consider either (1) running the TypeScript sources under src/test/grammar/**/*.test.ts (adjusting imports so they work with ts-node/register), or (2) updating the test script/pipeline to build before running mocha.
| "spec": ["src/test/*.test.ts", "lib/test/grammar/**/*.test.js"] | |
| "spec": ["src/test/*.test.ts", "src/test/grammar/**/*.test.ts"] |
| afterEach(() => { | ||
| ExternalLibraryUtils.removeLibraries(ExternalLibraryUtils.getRegisteredModuleNames() as string[]); | ||
| }); |
There was a problem hiding this comment.
The as string[] cast is unnecessary here: getRegisteredModuleNames() already returns ReadonlyArray<string>, which is compatible with removeLibraries’ parameter type. Dropping the cast keeps the intent clearer and avoids widening away readonly-ness.
There was a problem hiding this comment.
I think this is correct?
| "exclude": ["node_modules"], | ||
| "ts-node": { | ||
| "compilerOptions": { | ||
| "skipLibCheck": true |
There was a problem hiding this comment.
yeah - it's needed for the new unit tests. it looks like ts-node uses a different version of typescript than we are, and the settings are different. there was some copilot suggestions on things we could move to, but that could be done in a follow up.
| afterEach(() => { | ||
| ExternalLibraryUtils.removeLibraries(ExternalLibraryUtils.getRegisteredModuleNames() as string[]); | ||
| }); |
There was a problem hiding this comment.
I think this is correct?
| @@ -0,0 +1,74 @@ | |||
| # Copilot Instructions | |||
There was a problem hiding this comment.
Was there a typo, or did the naming convention change (i.e. the missing leading dot)
There was a problem hiding this comment.
I think previously you'd configure the path(s) to instructions using settings, and then they switched it to be convention based by default. i created the original file - this one was created with the agency /init switch.
mattmasson
changed the title
|
could also use an existing TextMate grammar test tool |
Grammar Fixes
Validated
syntaxes/powerquery.tmLanguage.jsonagainst the official M language specification and fixed 4 bugs:1. Dotted identifiers not matched beyond 2 characters after the dot
The
Identifierregex was missing a*quantifier on the continuation character class in the dotted-identifier part. This causedTable.AddColumnto be split into"Table"(scoped) +".AddColumn"(unscoped) instead of being recognized as a single identifier. Per the M spec, dotted identifiers likeTable.AddColumnare a singleregular-identifier.Before:
\.[start-char][continuation-char]— only matched 2 chars after dot (e.g..AB)After:
\.[start-char][continuation-char]*— matches any length (e.g..AddColumn)2.
<=and>=operators split into two tokensThe operator regex alternation was
(<>|<|>|<=|>=)— since<appears before<=, the regex engine matched<first, leaving=as a separate assignment token. Same for>=.Before:
(<>|<|>|<=|>=)→<=tokenized as<+=After:
(<>|<=|>=|<|>)— longest-match-first ordering3. Missing
??null-coalescing operatorThe
??operator is defined in the M spec as anoperator-or-punctuatorbut only single?was matched. Added(\\?\\?)before(\\?)and a newkeyword.operator.nullcoalescing.powerqueryscope.4. Missing
;semicolon punctuatorSemicolons are used in section documents (
section Foo; shared x = 1;) and are defined in the M spec as anoperator-or-punctuator, but had no grammar rule. Added aspunctuation.terminator.powerquery.Grammar Test Suite
Added a comprehensive test suite under
server/src/test/grammar/usingvscode-textmate+vscode-oniguruma(the standard approach for testing TextMate grammars outside VS Code) integrated with the existing mocha/chai test infrastructure.166 new tests across 7 test files:
keywords.test.ts#keywordforms (#binary,#date, etc.)identifiers.test.tsTable.AddColumn), quoted (#"name"), inclusive (@), implicit (_), intrinsic (#shared)literals.test.ts#infinity,#nan,true/false,nulloperators.test.ts=,<>,<=,>=,=>,??,..,...) and punctuators (,,;,(),{},[])comments.test.ts//) and block (/* */) comments, edge casestypes.test.tsoptional/nullablemodifiersexpressions.test.tslet/in,if/then/else,each,try/catch, functions, records, sectionsOther Changes
vscode-textmateandvscode-onigurumaas server devDependenciesserver/.mocharc.jsonto include compiled grammar testsskipLibChecktots-nodeconfig inserver/tsconfig.json(needed forvscode-onigurumaWebAssembly types)