-
Notifications
You must be signed in to change notification settings - Fork 14
Add more opportunities for cancellation for validate() #260
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
Draft
mattmasson
wants to merge
46
commits into
master
Choose a base branch
from
dev/moreAsyncValidate
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
981177f
update action config
67fb09a
update node and typescript
ff221af
add undefined check
c0fc7c2
fix mocha
ecef8cd
fix launch config
a3ce0ee
enable mocha tester in vscode
77fe063
0.11.0
d2c6085
simplify
cebbf1d
try reporting test results
6668ab8
simplify test summary
83f5100
checkpoint
c6eb414
checkpoint
4af63c8
fix promise implementation
5de58e3
update test settings
65983a9
checkpoint
ea6f6c4
Merge branch 'master' of https://github.com/microsoft/powerquery-lang…
d85e13f
regenerate lock file
ace79eb
0.11.1
1bc60b2
fix lint and async issues
243a6d9
remove unused
aac60a6
remove throwIfCancelled() from synchronous code paths
f9c1ff0
improve logic
1864238
Merge branch 'master' of https://github.com/microsoft/powerquery-lang…
f567c83
use expensive validation
9bfa8d1
instructions will be moved
18fe93b
refactor
c5890e5
Merge branch 'master' of https://github.com/microsoft/powerquery-lang…
274c3d0
cleanup
2f6de7e
don't reuse build window
40f8265
add yield helper
84d5335
remove conditional check
b8dbb6a
try this
b438cc5
use common code
89559b2
add timeout
fbedbb5
move cancellation token to test utils
14d2c24
redo tests
0412ece
large doc only
2e4a5b8
fix async cancellation
2a06ecd
change timeouts
f798a92
improve
278dc3b
adjust comment
0779958
cleanup comment
996ebab
test cleanup
291192a
add todo
8da8df5
remove the throwIfCancelled statements that were added to invokeExpre…
126aff9
Merge branch 'master' of https://github.com/microsoft/powerquery-lang…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,9 @@ | |
"group": { | ||
"kind": "build", | ||
"isDefault": true | ||
}, | ||
"presentation": { | ||
"clear": true | ||
} | ||
} | ||
] | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { ICancellationToken } from "@microsoft/powerquery-parser"; | ||
import { setImmediate } from "timers"; | ||
|
||
/** | ||
* Sequential processing with cancellation support - often better than Promise.all | ||
* for cancellation scenarios because we can stop between each operation. | ||
* Also yields control before each operation to allow cancellation tokens to take effect. | ||
*/ | ||
export async function processSequentiallyWithCancellation<T, R>( | ||
items: T[], | ||
processor: (item: T) => Promise<R>, | ||
cancellationToken?: ICancellationToken, | ||
): Promise<R[]> { | ||
const results: R[] = []; | ||
|
||
for (const item of items) { | ||
// Yield control to allow async cancellation tokens to take effect | ||
// eslint-disable-next-line no-await-in-loop | ||
await yieldForCancellation(cancellationToken); | ||
|
||
// eslint-disable-next-line no-await-in-loop | ||
const result: R = await processor(item); | ||
results.push(result); | ||
} | ||
|
||
return results; | ||
} | ||
|
||
export async function yieldForCancellation(cancellationToken?: ICancellationToken): Promise<void> { | ||
if (cancellationToken) { | ||
// First yield to microtasks (handles synchronous cancellation) | ||
await Promise.resolve(); | ||
cancellationToken.throwIfCancelled(); | ||
|
||
// Additional yield for setImmediate-based cancellation tokens | ||
// This ensures we yield to the timer queue where setImmediate callbacks execute | ||
await new Promise<void>((resolve: () => void) => setImmediate(resolve)); | ||
cancellationToken.throwIfCancelled(); | ||
} | ||
|
||
return Promise.resolve(); | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,15 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
import { CommonError, Result, ResultUtils } from "@microsoft/powerquery-parser"; | ||
import { NodeIdMap, ParseError, ParseState } from "@microsoft/powerquery-parser/lib/powerquery-parser/parser"; | ||
import { Diagnostic } from "vscode-languageserver-types"; | ||
import { TextDocument } from "vscode-languageserver-textdocument"; | ||
import { Trace } from "@microsoft/powerquery-parser/lib/powerquery-parser/common/trace"; | ||
|
||
import * as PromiseUtils from "../promiseUtils"; | ||
|
||
import { Analysis, AnalysisSettings, AnalysisUtils } from "../analysis"; | ||
import { CommonError, Result, ResultUtils } from "@microsoft/powerquery-parser"; | ||
import { TypeCache } from "../inspection"; | ||
import { validateDuplicateIdentifiers } from "./validateDuplicateIdentifiers"; | ||
import { validateFunctionExpression } from "./validateFunctionExpression"; | ||
|
@@ -35,7 +37,10 @@ export function validate( | |
initialCorrelationId: trace.id, | ||
}; | ||
|
||
validationSettings.cancellationToken?.throwIfCancelled(); | ||
|
||
const analysis: Analysis = AnalysisUtils.analysis(textDocument, analysisSettings); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: remove whitespace? |
||
const parseState: ParseState | undefined = ResultUtils.assertOk(await analysis.getParseState()); | ||
const parseError: ParseError.ParseError | undefined = ResultUtils.assertOk(await analysis.getParseError()); | ||
|
||
|
@@ -58,49 +63,56 @@ export function validate( | |
return result; | ||
} | ||
|
||
let functionExpressionDiagnostics: Diagnostic[]; | ||
let invokeExpressionDiagnostics: Diagnostic[]; | ||
let unknownIdentifiersDiagnostics: Diagnostic[]; | ||
|
||
const nodeIdMapCollection: NodeIdMap.Collection = parseState.contextState.nodeIdMapCollection; | ||
const typeCache: TypeCache = analysis.getTypeCache(); | ||
|
||
if (validationSettings.checkInvokeExpressions && nodeIdMapCollection) { | ||
functionExpressionDiagnostics = validateFunctionExpression(validationSettings, nodeIdMapCollection); | ||
// Define validation operations to run sequentially | ||
const validationOperations: (() => Promise<Diagnostic[]>)[] = [ | ||
// Parse validation (if there are parse errors) | ||
async (): Promise<Diagnostic[]> => await validateParse(parseError, updatedSettings), | ||
]; | ||
|
||
// Add conditional validations based on settings | ||
if (validationSettings.checkForDuplicateIdentifiers && nodeIdMapCollection) { | ||
validationOperations.push( | ||
async (): Promise<Diagnostic[]> => | ||
await validateDuplicateIdentifiers( | ||
textDocument, | ||
nodeIdMapCollection, | ||
updatedSettings, | ||
validationSettings.cancellationToken, | ||
), | ||
); | ||
} | ||
|
||
invokeExpressionDiagnostics = await validateInvokeExpression( | ||
validationSettings, | ||
nodeIdMapCollection, | ||
typeCache, | ||
if (validationSettings.checkInvokeExpressions && nodeIdMapCollection) { | ||
validationOperations.push( | ||
async (): Promise<Diagnostic[]> => | ||
await validateFunctionExpression(validationSettings, nodeIdMapCollection), | ||
async (): Promise<Diagnostic[]> => | ||
await validateInvokeExpression(validationSettings, nodeIdMapCollection, typeCache), | ||
); | ||
} else { | ||
functionExpressionDiagnostics = []; | ||
invokeExpressionDiagnostics = []; | ||
} | ||
|
||
if (validationSettings.checkUnknownIdentifiers && nodeIdMapCollection) { | ||
unknownIdentifiersDiagnostics = await validateUnknownIdentifiers( | ||
validationSettings, | ||
nodeIdMapCollection, | ||
typeCache, | ||
validationOperations.push( | ||
async (): Promise<Diagnostic[]> => | ||
await validateUnknownIdentifiers(validationSettings, nodeIdMapCollection, typeCache), | ||
); | ||
} else { | ||
unknownIdentifiersDiagnostics = []; | ||
} | ||
|
||
// Execute all validation operations sequentially with cancellation support | ||
const allDiagnostics: Diagnostic[][] = await PromiseUtils.processSequentiallyWithCancellation( | ||
validationOperations, | ||
(operation: () => Promise<Diagnostic[]>) => operation(), | ||
validationSettings.cancellationToken, | ||
); | ||
|
||
// Flatten all diagnostics into a single array | ||
const flattenedDiagnostics: Diagnostic[] = allDiagnostics.flat(); | ||
|
||
const result: ValidateOk = { | ||
diagnostics: [ | ||
...validateDuplicateIdentifiers( | ||
textDocument, | ||
nodeIdMapCollection, | ||
updatedSettings, | ||
validationSettings.cancellationToken, | ||
), | ||
...(await validateParse(parseError, updatedSettings)), | ||
...functionExpressionDiagnostics, | ||
...invokeExpressionDiagnostics, | ||
...unknownIdentifiersDiagnostics, | ||
], | ||
diagnostics: flattenedDiagnostics, | ||
hasSyntaxError: parseError !== undefined, | ||
}; | ||
|
||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we remove whitespace?