From 140950dd8e4f98ab33897247c888c89eea7caf00 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sun, 5 Jan 2020 12:46:42 -0700 Subject: [PATCH] Fixed bug in speculative evaluation mode where cached types from older speculative sessions were still being used in newer speculative sessions, giving the wrong answer. --- server/src/analyzer/typeEvaluator.ts | 83 +++++++++++++++++++--------- 1 file changed, 57 insertions(+), 26 deletions(-) diff --git a/server/src/analyzer/typeEvaluator.ts b/server/src/analyzer/typeEvaluator.ts index 42a2f5e10..2542f647e 100644 --- a/server/src/analyzer/typeEvaluator.ts +++ b/server/src/analyzer/typeEvaluator.ts @@ -270,12 +270,18 @@ interface CodeFlowAnalyzer { type CachedType = Type | PartialType; +// Each time we enter speculative mode, we increment +// the speculative mode ID so we can distinguish between +// types that were cached in a previous speculative mode. +let nextSpeculativeModeId = 1; +const invalidSpeculativeModeId = 0; + const partialTypeCategory = -1; interface PartialType { category: -1; type: Type | undefined; isIncomplete?: boolean; - isSpeculative?: boolean; + speculativeModeId: number; } interface FlowNodeTypeResult { @@ -309,7 +315,6 @@ interface ReturnTypeInferenceContext { const maxReturnTypeInferenceStackSize = 3; export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { - let isSpeculativeMode = false; const symbolResolutionStack: SymbolResolutionStackEntry[] = []; const isReachableRecursionMap = new Map(); const functionRecursionMap = new Map(); @@ -317,6 +322,8 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { const codeFlowAnalyzerCache = new Map(); const typeCache = new Map(); + let speculativeModeId = invalidSpeculativeModeId; + const returnTypeInferenceContextStack: ReturnTypeInferenceContext[] = []; let returnTypeInferenceTypeCache: Map | undefined; @@ -343,14 +350,31 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { } if (cachedType.category === partialTypeCategory) { - return isSpeculativeMode ? (cachedType as PartialType).type : undefined; + if (!isSpeculativeMode()) { + return undefined; + } + + const partialType = cachedType as PartialType; + + // If the cached type was written as part of an older + // speculative mode, we will ignore it. If it was generated + // as part of a newer speculative mode, it is OK to use. + if (partialType.speculativeModeId < speculativeModeId) { + return undefined; + } + + return partialType.type; } return cachedType; } function writeTypeCache(node: ParseNode, type: Type) { - const cachedType = isSpeculativeMode ? { category: partialTypeCategory, type } : type; + const cachedType = !isSpeculativeMode() ? type : { + category: partialTypeCategory, + speculativeModeId, + type + }; // Should we use a temporary cache associated with a contextual // analysis of a function, contextualized based on call-site argument types? @@ -1419,7 +1443,7 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { } function addWarning(message: string, node: ParseNode) { - if (!isSpeculativeMode) { + if (!isSpeculativeMode()) { const fileInfo = getFileInfo(node); return fileInfo.diagnosticSink.addWarningWithTextRange(message, node); } @@ -1428,7 +1452,7 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { } function addError(message: string, node: ParseNode) { - if (!isSpeculativeMode) { + if (!isSpeculativeMode()) { const fileInfo = getFileInfo(node); return fileInfo.diagnosticSink.addErrorWithTextRange(message, node); } @@ -1905,7 +1929,7 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { } function setSymbolAccessed(fileInfo: AnalyzerFileInfo, symbol: Symbol) { - if (!isSpeculativeMode) { + if (!isSpeculativeMode()) { fileInfo.accessedSymbolMap.set(symbol.id, true); } } @@ -3247,15 +3271,18 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { let validOverload: FunctionType | undefined; - // Temporarily disable diagnostic output. - useSpeculativeMode(() => { - for (const overload of callType.overloads) { + for (const overload of callType.overloads) { + // Temporarily disable diagnostic output. + useSpeculativeMode(() => { if (validateCallArguments(errorNode, argList, overload, new Map())) { validOverload = overload; - break; } + }); + + if (validOverload) { + break; } - }); + } return validOverload; } @@ -3699,7 +3726,7 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { // Run through all the args that were not validated and evaluate their types // to ensure that we haven't missed any (due to arg/param mismatches). This will // ensure that referenced symbols are not reported as unaccessed. - if (!isSpeculativeMode) { + if (!isSpeculativeMode()) { argList.forEach(arg => { if (arg.valueExpression) { if (!validateArgTypeParams.some(validatedArg => validatedArg.argument === arg)) { @@ -3726,7 +3753,7 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { const exprType = getTypeOfExpression(argParam.argument.valueExpression, expectedType); argType = exprType.type; - if (argParam.argument && argParam.argument.name && !isSpeculativeMode) { + if (argParam.argument && argParam.argument.name && !isSpeculativeMode()) { writeTypeCache(argParam.argument.name, argType); } } else { @@ -7237,10 +7264,10 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { // object. For non-speculative and complete types, we'll store // the type directly. const entry: PartialType | Type | undefined = - (isSpeculativeMode || isIncomplete) ? { + (isSpeculativeMode() || isIncomplete) ? { category: partialTypeCategory, type, - isSpeculative: isSpeculativeMode, + speculativeModeId, isIncomplete } : type; @@ -7270,8 +7297,10 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { } const partialType = cachedEntry as PartialType; - if (partialType.isSpeculative && !isSpeculativeMode) { - return undefined; + if (partialType.speculativeModeId !== invalidSpeculativeModeId) { + if (!isSpeculativeMode()) { + return undefined; + } } return { @@ -8031,19 +8060,21 @@ export function createTypeEvaluator(importLookup: ImportLookup): TypeEvaluator { // any caching of types, under the assumption that we're // performing speculative evaluations. function useSpeculativeMode(callback: () => void) { - const prevSpeculativeMode = isSpeculativeMode; - isSpeculativeMode = true; + const prevSpeculativeModeId = speculativeModeId; + speculativeModeId = nextSpeculativeModeId++; callback(); - isSpeculativeMode = prevSpeculativeMode; + speculativeModeId = prevSpeculativeModeId; + } + + function isSpeculativeMode() { + return speculativeModeId !== invalidSpeculativeModeId; } function disableSpeculativeMode(callback: () => void) { - const prevSpeculativeMode = isSpeculativeMode; - isSpeculativeMode = false; - + const prevSpeculativeModeId = speculativeModeId; + speculativeModeId = invalidSpeculativeModeId; callback(); - - isSpeculativeMode = prevSpeculativeMode; + speculativeModeId = prevSpeculativeModeId; } function getFileInfo(node: ParseNode): AnalyzerFileInfo {