Add more opportunities for cancellation for validate() #260
Conversation
…uage-services into dev/moreAsyncValidate
…uage-services into dev/moreAsyncValidate
There was a problem hiding this comment.
Pull Request Overview
This PR adds enhanced cancellation support to the validate() function by introducing more opportunities for cancellation throughout the validation process. The key change is implementing sequential processing with cancellation checks rather than using Promise.all(), allowing validation operations to be cancelled more responsively.
- Implements sequential processing for validation operations with cancellation support
- Adds cancellation checks at multiple points throughout the validation pipeline
- Introduces a new
processSequentiallyWithCancellationutility function for better async cancellation patterns
Reviewed Changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/powerquery-language-services/promiseUtils.ts | New utility module providing sequential processing with cancellation support |
| src/powerquery-language-services/validate/validate.ts | Refactored to use sequential validation operations with cancellation checks |
| src/powerquery-language-services/validate/validateInvokeExpression.ts | Updated to use sequential processing and added cancellation checks |
| src/powerquery-language-services/validate/validateDuplicateIdentifiers.ts | Converted to async with sequential processing and cancellation support |
| src/powerquery-language-services/validate/validateFunctionExpression.ts | Made async and added cancellation checks with yielding |
| src/powerquery-language-services/inspection/invokeExpression/invokeExpression.ts | Added cancellation checks at key inspection points |
| src/test/validation/asyncValidation.test.ts | Comprehensive test suite for validation cancellation behavior |
| src/test/utils/promiseUtils.test.ts | Unit tests for the new promise utility functions |
| src/test/testUtils/validationTestUtils.ts | Helper functions for testing validation cancellation scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/powerquery-language-services/validate/validateDuplicateIdentifiers.ts
Show resolved
Hide resolved
src/powerquery-language-services/validate/validateDuplicateIdentifiers.ts
Outdated
Show resolved
Hide resolved
| readonly library: Library.ILibrary; | ||
| readonly source: string; | ||
| // Keep cancellationToken here for backward compatibility | ||
| readonly cancellationToken: PQP.ICancellationToken | undefined; |
There was a problem hiding this comment.
@JordanBoltonMN can I move cancellationToken up to InspectionSettings and rmove it from here? Or do we need to keep it here for back compat?
There was a problem hiding this comment.
ValidationSettings extends InspectionSettings, so we can safely promote properties from ValidationSettings to InspectionSettings.
|
needs version bump |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 8 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…uage-services into dev/moreAsyncValidate
…uage-services into dev/moreAsyncValidate
| readonly library: Library.ILibrary; | ||
| readonly source: string; | ||
| // Keep cancellationToken here for backward compatibility | ||
| readonly cancellationToken: PQP.ICancellationToken | undefined; |
There was a problem hiding this comment.
ValidationSettings extends InspectionSettings, so we can safely promote properties from ValidationSettings to InspectionSettings.
| validationSettings.cancellationToken?.throwIfCancelled(); | ||
|
|
||
| const analysis: Analysis = AnalysisUtils.analysis(textDocument, analysisSettings); | ||
|
|
There was a problem hiding this comment.
nit: remove whitespace?
| import { Trace } from "@microsoft/powerquery-parser/lib/powerquery-parser/common/trace"; | ||
|
|
||
| import * as PromiseUtils from "../promiseUtils"; | ||
|
|
There was a problem hiding this comment.
nit: can we remove whitespace?
| import { Trace } from "@microsoft/powerquery-parser/lib/powerquery-parser/common/trace"; | ||
|
|
||
| import * as PromiseUtils from "../promiseUtils"; | ||
|
|
| cancellationToken, | ||
| ), | ||
| // Create an array of validation functions to process sequentially | ||
| const validationFunctions: Array<() => Promise<ReadonlyArray<Diagnostic>>> = [ |
There was a problem hiding this comment.
nit: ReadonlyArray<T>
| import { Trace } from "@microsoft/powerquery-parser/lib/powerquery-parser/common/trace"; | ||
|
|
||
| import * as PromiseUtils from "../promiseUtils"; | ||
|
|
| import type { Range } from "vscode-languageserver-textdocument"; | ||
|
|
||
| import * as PromiseUtils from "../promiseUtils"; | ||
|
|
| @@ -0,0 +1,111 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
nit: mark every property that can be as readonly
| trackProcessed: true, | ||
| }); | ||
|
|
||
| await expectCancellationAfterTimeout( |
There was a problem hiding this comment.
Can you convert this to the params style arguments? As an outside reader (especially if I'm reading online and can't drill down into the function definition), it makes it more immediately apparent what those arguments are for.
| * Creates common test setup for string processing tests | ||
| */ | ||
| function testSetup(options?: { | ||
| items?: string[]; |
Resolves #253.