Skip to content

Commit 6179333

Browse files
JordanBoltonMNJordan BoltonCopilot
authored
Avoid discarded primary-type speculation (#423)
* Avoid discarded primary-type speculation and bump to 3.1.0 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d5e5f8a-36d8-47aa-8526-9275e8355e2c * Explain why primary-type prediction safely skips speculation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d5e5f8a-36d8-47aa-8526-9275e8355e2c * Refine primary-type prediction documentation and test placement Move prediction coverage into parser behavior tests, remove the README addition, document token-to-AST mappings, and bump the package to 3.2.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d5e5f8a-36d8-47aa-8526-9275e8355e2c * Remove package version changes from parser speculation fix Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bfa84c62-b343-4449-a0d6-ecfdecea16f1 --------- Co-authored-by: Jordan Bolton <jobolton@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d5e5f8a-36d8-47aa-8526-9275e8355e2c Copilot-Session: bfa84c62-b343-4449-a0d6-ecfdecea16f1
1 parent 48af2f6 commit 6179333

2 files changed

Lines changed: 273 additions & 1 deletion

File tree

‎src/powerquery-parser/parser/parsers/naiveParseSteps.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1971,6 +1971,26 @@ export async function readType(
19711971

19721972
state.cancellationToken?.throwIfCancelled();
19731973

1974+
// canStartPrimaryType covers every prefix accepted by tryReadPrimaryType. If false, that
1975+
// reader only creates a PrimitiveType context, rejects the token without consuming it,
1976+
// and rolls the context back. readType discards that diagnostic and calls the same
1977+
// primary-expression reader below, so skipping the attempt loses no valid type or diagnostic.
1978+
// Keep speculation without an enclosing context or at EOF, where its root/cached-token
1979+
// bookkeeping is observable. Cancellation checks remain above and in the fallback;
1980+
// traces and checkpoint hooks for the discarded attempt are intentionally omitted.
1981+
if (
1982+
state.currentContextNode !== undefined &&
1983+
state.tokenIndex < state.lexerSnapshot.tokens.length &&
1984+
!canStartPrimaryType(state)
1985+
) {
1986+
trace.exit({
1987+
[NaiveTraceConstant.TokenIndex]: state.tokenIndex,
1988+
[TraceConstant.IsError]: true,
1989+
});
1990+
1991+
return parser.readPrimaryExpression(state, parser, trace.id);
1992+
}
1993+
19741994
const triedReadPrimaryType: TriedReadPrimaryType = await tryReadPrimaryType(state, parser, trace.id);
19751995

19761996
if (ResultUtils.isOk(triedReadPrimaryType)) {
@@ -2397,6 +2417,30 @@ export async function readFunctionType(
23972417
return functionType;
23982418
}
23992419

2420+
function canStartPrimaryType(state: ParseState): boolean {
2421+
const token: Token.Token | undefined = state.lexerSnapshot.tokens[state.tokenIndex];
2422+
2423+
// No token means no primary-type prefix. The token checks correspond to:
2424+
// LeftBracket: Ast.RecordType, such as [x = number].
2425+
// LeftBrace: Ast.ListType, such as {number}.
2426+
// KeywordType: Ast.PrimitiveType with primitive kind Type, not Ast.TypePrimaryType.
2427+
// NullLiteral: Ast.PrimitiveType with primitive kind Null, not Ast.LiteralExpression.
2428+
// Identifier "nullable": Ast.NullableType, such as nullable number.
2429+
// Other primitive-type identifiers: Ast.PrimitiveType (number, text, etc.).
2430+
// "table" and "function" also admit Ast.TableType and Ast.FunctionType;
2431+
// tryReadPrimaryType uses subsequent tokens to distinguish these from primitive types.
2432+
return (
2433+
token !== undefined &&
2434+
(token.kind === TokenKind.LeftBracket ||
2435+
token.kind === TokenKind.LeftBrace ||
2436+
token.kind === TokenKind.KeywordType ||
2437+
token.kind === TokenKind.NullLiteral ||
2438+
(token.kind === TokenKind.Identifier &&
2439+
(token.data === Constant.LanguageConstant.Nullable ||
2440+
ConstantUtils.isPrimitiveTypeConstant(token.data))))
2441+
);
2442+
}
2443+
24002444
async function tryReadPrimaryType(
24012445
state: ParseState,
24022446
parser: Parser,

‎src/test/libraryTest/parser/parseBehavior.test.ts‎

Lines changed: 229 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,23 @@
33

44
import "mocha";
55
import { expect } from "chai";
6+
import { rejects } from "assert";
67

78
import * as ParserTestUtils from "./parserTestUtils";
8-
import { Assert, DefaultSettings, Language, Task, TaskUtils } from "../../../powerquery-parser";
9+
import {
10+
Assert,
11+
CommonError,
12+
CounterCancellationToken,
13+
DefaultSettings,
14+
Language,
15+
NoOpCancellationToken,
16+
Parser,
17+
Task,
18+
TaskUtils,
19+
} from "../../../powerquery-parser";
20+
import { Trace, TraceConstant, TraceManager } from "../../../powerquery-parser/common/trace";
921
import { AssertTestUtils } from "../../testUtils";
22+
import { Ast } from "../../../powerquery-parser/language";
1023
import { NodeKind } from "../../../powerquery-parser/language/ast/ast";
1124
import { ParseBehavior } from "../../../powerquery-parser/parser/parseBehavior";
1225
import { ParseError } from "../../../powerquery-parser/parser";
@@ -205,3 +218,218 @@ in
205218
).to.deep.equal(["text", "number"]);
206219
});
207220
});
221+
222+
// Strict primary-type reads still perform speculation. Only an unconsumed primitive-type
223+
// failure is discarded here, reproducing readType's fallback without using its prediction.
224+
async function readTypeWithoutPrediction(
225+
state: Parser.ParseState,
226+
parser: Parser.Parser,
227+
correlationId: number | undefined,
228+
): Promise<Ast.TType> {
229+
const tokenIndex: number = state.tokenIndex;
230+
231+
try {
232+
return await parser.readPrimaryType(state, parser, correlationId);
233+
} catch (error: unknown) {
234+
if (
235+
state.tokenIndex !== tokenIndex ||
236+
!(
237+
error instanceof Parser.ParseError.InvalidPrimitiveTypeError ||
238+
error instanceof Parser.ParseError.ExpectedAnyTokenKindError
239+
)
240+
) {
241+
throw error;
242+
}
243+
244+
return parser.readPrimaryExpression(state, parser, correlationId);
245+
}
246+
}
247+
248+
class RecordingTraceManager extends TraceManager {
249+
public readonly events: { trace: Trace; message: string }[] = [];
250+
251+
public emit(trace: Trace, message: string): void {
252+
this.events.push({ trace, message });
253+
}
254+
}
255+
256+
function newState(overrides?: Partial<Parser.ParseState>): Parser.ParseState {
257+
const lex: Task.TriedLexTask = TaskUtils.tryLex(DefaultSettings, "unknown");
258+
TaskUtils.assertIsLexStageOk(lex);
259+
const state: Parser.ParseState = Parser.ParseStateUtils.newState(lex.lexerSnapshot, overrides);
260+
Parser.ParseStateUtils.startContext(state, Ast.NodeKind.ListType);
261+
262+
return state;
263+
}
264+
265+
describe("Parser primary-type prediction", () => {
266+
for (const parser of [Parser.CombinatorialParserV2, Parser.RecursiveDescentParser]) {
267+
for (const text of [
268+
"let a = type unknown in a",
269+
"let a = type (1 + 2) in a",
270+
"let a = type @unknown in a",
271+
"let a = type 1 in a",
272+
"let a = type in a",
273+
"let a = type table [x=",
274+
"let a = type function (x as",
275+
"let a = type function (x as number) as ",
276+
"let a = type [x = number] in a",
277+
"let a = type nullable number in a",
278+
"(x as number) as nullable number => x",
279+
]) {
280+
it(`preserves full parse results for ${text}`, async () => {
281+
const actual: Task.TriedLexParseTask = await TaskUtils.tryLexParse(
282+
{ ...DefaultSettings, parser },
283+
text,
284+
);
285+
286+
const expected: Task.TriedLexParseTask = await TaskUtils.tryLexParse(
287+
{ ...DefaultSettings, parser: { ...parser, readType: readTypeWithoutPrediction } },
288+
text,
289+
);
290+
291+
expect(actual).to.deep.equal(expected);
292+
293+
expect("nodeIdMapCollection" in actual ? actual.nodeIdMapCollection : undefined).to.deep.equal(
294+
"nodeIdMapCollection" in expected ? expected.nodeIdMapCollection : undefined,
295+
);
296+
297+
expect("parseState" in actual ? actual.parseState : undefined).to.deep.equal(
298+
"parseState" in expected ? expected.parseState : undefined,
299+
);
300+
});
301+
}
302+
303+
it("skips speculation with a cancellation token and custom parser hooks", async () => {
304+
const state: Parser.ParseState = newState({ cancellationToken: NoOpCancellationToken });
305+
let primaryReads: number = 0;
306+
307+
const customParser: Parser.Parser = {
308+
...parser,
309+
checkpoint: (): never => {
310+
throw new Error("The skipped attempt must not create a checkpoint");
311+
},
312+
restoreCheckpoint: (): never => {
313+
throw new Error("The skipped attempt must not restore a checkpoint");
314+
},
315+
readPrimaryExpression: async (
316+
state: Parser.ParseState,
317+
parser: Parser.Parser,
318+
correlationId: number | undefined,
319+
): Promise<Ast.TPrimaryExpression> => {
320+
primaryReads += 1;
321+
322+
return await Parser.NaiveParseSteps.readPrimaryExpression(state, parser, correlationId);
323+
},
324+
};
325+
326+
const result: Ast.TType = await customParser.readType(state, customParser, undefined);
327+
expect(result.kind).to.equal(Ast.NodeKind.IdentifierExpression);
328+
expect(primaryReads).to.equal(1);
329+
});
330+
331+
it("traces only executed reads with matching entries, exits, and correlations", async () => {
332+
const traceManager: RecordingTraceManager = new RecordingTraceManager();
333+
const state: Parser.ParseState = newState({ traceManager });
334+
await parser.readType(state, parser, undefined);
335+
336+
const entries: Map<number, Trace> = new Map();
337+
const exits: Set<number> = new Set();
338+
339+
for (const { trace, message } of traceManager.events) {
340+
if (message === TraceConstant.Entry) {
341+
expect(entries.has(trace.id)).to.equal(false);
342+
343+
if (trace.correlationId !== undefined) {
344+
expect(entries.has(trace.correlationId)).to.equal(true);
345+
}
346+
347+
entries.set(trace.id, trace);
348+
} else if (message === TraceConstant.Exit) {
349+
expect(entries.has(trace.id)).to.equal(true);
350+
expect(exits.has(trace.id)).to.equal(false);
351+
exits.add(trace.id);
352+
}
353+
}
354+
355+
const tasks: string[] = [...entries.values()].map((trace: Trace) => trace.task);
356+
expect(tasks).to.include("readType");
357+
expect(tasks).to.include("readPrimaryExpression");
358+
expect(tasks).not.to.include("tryReadPrimaryType");
359+
expect(tasks).not.to.include("tryReadPrimitiveType");
360+
expect(exits.size).to.equal(entries.size);
361+
});
362+
363+
it("honors cancellation before prediction", async () => {
364+
const state: Parser.ParseState = newState({ cancellationToken: new CounterCancellationToken(1) });
365+
const tokenIndex: number = state.tokenIndex;
366+
const idCounter: number = state.contextState.idCounter;
367+
await rejects(parser.readType(state, parser, undefined), CommonError.CancellationError);
368+
expect(state.tokenIndex).to.equal(tokenIndex);
369+
expect(state.contextState.idCounter).to.equal(idCounter);
370+
});
371+
372+
it("honors cancellation when entering the primary-expression fallback", async () => {
373+
const state: Parser.ParseState = newState({ cancellationToken: new CounterCancellationToken(2) });
374+
let enteredFallback: boolean = false;
375+
376+
const customParser: Parser.Parser = {
377+
...parser,
378+
readPrimaryExpression: async (
379+
state: Parser.ParseState,
380+
parser: Parser.Parser,
381+
correlationId: number | undefined,
382+
): Promise<Ast.TPrimaryExpression> => {
383+
enteredFallback = true;
384+
385+
return await Parser.NaiveParseSteps.readPrimaryExpression(state, parser, correlationId);
386+
},
387+
};
388+
389+
await rejects(customParser.readType(state, customParser, undefined), CommonError.CancellationError);
390+
expect(enteredFallback).to.equal(true);
391+
expect(state.tokenIndex).to.equal(0);
392+
});
393+
394+
it("retains strict primitive-type diagnostics and their checkpoint hooks", async () => {
395+
let checkpoints: number = 0;
396+
397+
const customParser: Parser.Parser = {
398+
...parser,
399+
checkpoint: (state: Parser.ParseState): Promise<Parser.ParseStateCheckpoint> => {
400+
checkpoints += 1;
401+
402+
return parser.checkpoint(state);
403+
},
404+
};
405+
406+
await rejects(
407+
customParser.readPrimaryType(newState(), customParser, undefined),
408+
Parser.ParseError.InvalidPrimitiveTypeError,
409+
);
410+
411+
expect(checkpoints).to.be.greaterThan(0);
412+
});
413+
414+
it("retains EOF rollback and clears the stale cached token", async () => {
415+
const state: Parser.ParseState = newState();
416+
state.tokenIndex = state.lexerSnapshot.tokens.length;
417+
state.currentTokenKind = undefined;
418+
expect(state.currentToken).not.to.equal(undefined);
419+
let checkpoints: number = 0;
420+
421+
const customParser: Parser.Parser = {
422+
...parser,
423+
checkpoint: (state: Parser.ParseState): Promise<Parser.ParseStateCheckpoint> => {
424+
checkpoints += 1;
425+
426+
return parser.checkpoint(state);
427+
},
428+
};
429+
430+
await rejects(customParser.readType(state, customParser, undefined));
431+
expect(checkpoints).to.be.greaterThan(0);
432+
expect(state.currentToken).to.equal(undefined);
433+
});
434+
}
435+
});

0 commit comments

Comments
 (0)