From 93a4ef878d3100e346944adc8d6cc85957471dd2 Mon Sep 17 00:00:00 2001 From: PylanceBot <99766470+PylanceBot@users.noreply.github.com> Date: Wed, 1 Mar 2023 15:51:08 -0800 Subject: [PATCH] [PylanceBot] Pull Pylance with Pyright 1.1.296 (#4716) Co-authored-by: Bill Schnurr Co-authored-by: HeeJae Chang Co-authored-by: Erik De Bonte Co-authored-by: Rich Chiodo --- .github/workflows/codeql.yml | 2 +- .../pyright-internal/src/analyzer/binder.ts | 85 +---- .../src/analyzer/declaration.ts | 6 + .../src/analyzer/parseTreeUtils.ts | 80 +++++ .../pyright-internal/src/analyzer/program.ts | 309 ++++++++++++++++-- .../src/common/collectionUtils.ts | 8 + .../src/common/positionUtils.ts | 16 + .../src/languageService/completionProvider.ts | 92 +++--- .../src/languageService/hoverProvider.ts | 7 + .../src/languageService/importAdder.ts | 7 +- .../src/languageService/indentationUtils.ts | 36 +- .../languageService/renameModuleProvider.ts | 24 +- .../completions.errorNodes.fourslash.ts | 27 ++ .../tests/fourslash/hover.slots.fourslash.ts | 15 + .../src/tests/importAdder.test.ts | 14 + .../tests/indentationUtils.reindent.test.ts | 27 ++ .../src/tests/moveSymbol.importAdder.test.ts | 185 +++++++++-- .../src/tests/moveSymbol.insertion.test.ts | 114 +++++++ .../src/tests/renameModuleTestUtils.ts | 16 +- 19 files changed, 856 insertions(+), 214 deletions(-) create mode 100644 packages/pyright-internal/src/tests/fourslash/completions.errorNodes.fourslash.ts create mode 100644 packages/pyright-internal/src/tests/fourslash/hover.slots.fourslash.ts diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 75e7b311d..0595d4341 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -32,7 +32,7 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v2 + uses: actions/checkout@v3 # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL diff --git a/packages/pyright-internal/src/analyzer/binder.ts b/packages/pyright-internal/src/analyzer/binder.ts index 546f7a191..64c45fd1f 100644 --- a/packages/pyright-internal/src/analyzer/binder.ts +++ b/packages/pyright-internal/src/analyzer/binder.ts @@ -114,6 +114,7 @@ import { ParameterDeclaration, TypeAliasDeclaration, TypeParameterDeclaration, + UnresolvedModuleMarker, VariableDeclaration, } from './declaration'; import { extractParameterDocumentation } from './docStringUtils'; @@ -2472,7 +2473,7 @@ export class Binder extends ParseTreeWalker { if (importInfo && importInfo.isImportFound && !importInfo.isNativeLib && importInfo.resolvedPaths.length > 0) { pathOfLastSubmodule = importInfo.resolvedPaths[importInfo.resolvedPaths.length - 1]; } else { - pathOfLastSubmodule = '*** unresolved ***'; + pathOfLastSubmodule = UnresolvedModuleMarker; } const isResolved = @@ -2535,7 +2536,7 @@ export class Binder extends ParseTreeWalker { const loaderActionPath = importInfo && i < importInfo.resolvedPaths.length ? importInfo.resolvedPaths[i] - : '*** unresolved ***'; + : UnresolvedModuleMarker; // Allocate a new loader action. loaderActions = { @@ -3789,87 +3790,13 @@ export class Binder extends ParseTreeWalker { } private _getVariableDocString(node: ExpressionNode): string | undefined { - // Walk up the parse tree to find an assignment expression. - let curNode: ParseNode | undefined = node; - let annotationNode: TypeAnnotationNode | undefined; - - while (curNode) { - if (curNode.nodeType === ParseNodeType.Assignment) { - break; - } - - if (curNode.nodeType === ParseNodeType.TypeAnnotation && !annotationNode) { - annotationNode = curNode; - } - - curNode = curNode.parent; - } - - if (curNode?.nodeType !== ParseNodeType.Assignment) { - // Allow a simple annotation statement to have a docstring even - // though PEP 258 doesn't mention this case. This PEP pre-dated - // PEP 526, so it didn't contemplate this situation. - if (annotationNode) { - curNode = annotationNode; - } else { - return undefined; - } - } - - const parentNode = curNode.parent; - if (parentNode?.nodeType !== ParseNodeType.StatementList) { - return undefined; - } - - const suiteOrModule = parentNode.parent; - if ( - !suiteOrModule || - (suiteOrModule.nodeType !== ParseNodeType.Module && suiteOrModule.nodeType !== ParseNodeType.Suite) - ) { - return undefined; - } - - const assignmentIndex = suiteOrModule.statements.findIndex((node) => node === parentNode); - if (assignmentIndex < 0 || assignmentIndex === suiteOrModule.statements.length - 1) { - return undefined; - } - - const nextStatement = suiteOrModule.statements[assignmentIndex + 1]; - - if (nextStatement.nodeType !== ParseNodeType.StatementList || !ParseTreeUtils.isDocString(nextStatement)) { - return undefined; - } - - // See if the assignment is within one of the contexts specified in PEP 258. - let isValidContext = false; - if (parentNode?.parent?.nodeType === ParseNodeType.Module) { - // If we're at the top level of a module, the attribute docstring is valid. - isValidContext = true; - } else if ( - parentNode?.parent?.nodeType === ParseNodeType.Suite && - parentNode?.parent?.parent?.nodeType === ParseNodeType.Class - ) { - // If we're at the top level of a class, the attribute docstring is valid. - isValidContext = true; - } else { - const func = ParseTreeUtils.getEnclosingFunction(parentNode); - - // If we're within an __init__ method, the attribute docstring is valid. - if ( - func && - func.name.value === '__init__' && - ParseTreeUtils.getEnclosingClass(func, /* stopAtFunction */ true) - ) { - isValidContext = true; - } - } - - if (!isValidContext) { + const docNode = ParseTreeUtils.getVariableDocStringNode(node); + if (!docNode) { return undefined; } // A docstring can consist of multiple joined strings in a single expression. - const strings = (nextStatement.statements[0] as StringListNode).strings; + const strings = docNode.strings; if (strings.length === 1) { // Common case. return strings[0].value; diff --git a/packages/pyright-internal/src/analyzer/declaration.ts b/packages/pyright-internal/src/analyzer/declaration.ts index a15edfac1..133fe0ae7 100644 --- a/packages/pyright-internal/src/analyzer/declaration.ts +++ b/packages/pyright-internal/src/analyzer/declaration.ts @@ -31,6 +31,8 @@ import { YieldNode, } from '../parser/parseNodes'; +export const UnresolvedModuleMarker = '*** unresolved ***'; + export const enum DeclarationType { Intrinsic, Variable, @@ -290,3 +292,7 @@ export function isSpecialBuiltInClassDeclaration(decl: Declaration): decl is Spe export function isIntrinsicDeclaration(decl: Declaration): decl is IntrinsicDeclaration { return decl.type === DeclarationType.Intrinsic; } + +export function isUnresolvedAliasDeclaration(decl: Declaration): boolean { + return isAliasDeclaration(decl) && decl.path === UnresolvedModuleMarker; +} diff --git a/packages/pyright-internal/src/analyzer/parseTreeUtils.ts b/packages/pyright-internal/src/analyzer/parseTreeUtils.ts index 37769d0a2..052916c85 100644 --- a/packages/pyright-internal/src/analyzer/parseTreeUtils.ts +++ b/packages/pyright-internal/src/analyzer/parseTreeUtils.ts @@ -2554,3 +2554,83 @@ function _getEndPositionIfMultipleStatementsAreOnSameLine( return undefined; } + +export function getVariableDocStringNode(node: ExpressionNode): StringListNode | undefined { + // Walk up the parse tree to find an assignment expression. + let curNode: ParseNode | undefined = node; + let annotationNode: TypeAnnotationNode | undefined; + + while (curNode) { + if (curNode.nodeType === ParseNodeType.Assignment) { + break; + } + + if (curNode.nodeType === ParseNodeType.TypeAnnotation && !annotationNode) { + annotationNode = curNode; + } + + curNode = curNode.parent; + } + + if (curNode?.nodeType !== ParseNodeType.Assignment) { + // Allow a simple annotation statement to have a docstring even + // though PEP 258 doesn't mention this case. This PEP pre-dated + // PEP 526, so it didn't contemplate this situation. + if (annotationNode) { + curNode = annotationNode; + } else { + return undefined; + } + } + + const parentNode = curNode.parent; + if (parentNode?.nodeType !== ParseNodeType.StatementList) { + return undefined; + } + + const suiteOrModule = parentNode.parent; + if ( + !suiteOrModule || + (suiteOrModule.nodeType !== ParseNodeType.Module && suiteOrModule.nodeType !== ParseNodeType.Suite) + ) { + return undefined; + } + + const assignmentIndex = suiteOrModule.statements.findIndex((node) => node === parentNode); + if (assignmentIndex < 0 || assignmentIndex === suiteOrModule.statements.length - 1) { + return undefined; + } + + const nextStatement = suiteOrModule.statements[assignmentIndex + 1]; + + if (nextStatement.nodeType !== ParseNodeType.StatementList || !isDocString(nextStatement)) { + return undefined; + } + + // See if the assignment is within one of the contexts specified in PEP 258. + let isValidContext = false; + if (parentNode?.parent?.nodeType === ParseNodeType.Module) { + // If we're at the top level of a module, the attribute docstring is valid. + isValidContext = true; + } else if ( + parentNode?.parent?.nodeType === ParseNodeType.Suite && + parentNode?.parent?.parent?.nodeType === ParseNodeType.Class + ) { + // If we're at the top level of a class, the attribute docstring is valid. + isValidContext = true; + } else { + const func = getEnclosingFunction(parentNode); + + // If we're within an __init__ method, the attribute docstring is valid. + if (func && func.name.value === '__init__' && getEnclosingClass(func, /* stopAtFunction */ true)) { + isValidContext = true; + } + } + + if (!isValidContext) { + return undefined; + } + + // A docstring can consist of multiple joined strings in a single expression. + return nextStatement.statements[0] as StringListNode; +} diff --git a/packages/pyright-internal/src/analyzer/program.ts b/packages/pyright-internal/src/analyzer/program.ts index 901d1f30c..c06976273 100644 --- a/packages/pyright-internal/src/analyzer/program.ts +++ b/packages/pyright-internal/src/analyzer/program.ts @@ -19,13 +19,14 @@ import { MarkupKind, } from 'vscode-languageserver-types'; +import { Commands } from '../commands/commands'; import { OperationCanceledException, throwIfCancellationRequested } from '../common/cancellationUtils'; -import { appendArray } from '../common/collectionUtils'; +import { appendArray, arrayEquals } from '../common/collectionUtils'; import { ConfigOptions, ExecutionEnvironment, matchFileSpecs } from '../common/configOptions'; import { ConsoleInterface, StandardConsole } from '../common/console'; import { assert, assertNever } from '../common/debug'; -import { Diagnostic } from '../common/diagnostic'; -import { DiagnosticSink, FileDiagnostics } from '../common/diagnosticSink'; +import { Diagnostic, DiagnosticCategory } from '../common/diagnostic'; +import { FileDiagnostics } from '../common/diagnosticSink'; import { FileEditAction, FileEditActions, FileOperations, TextEditAction } from '../common/editAction'; import { Extensions } from '../common/extensibility'; import { LogTracker } from '../common/logTracker'; @@ -43,6 +44,7 @@ import { } from '../common/pathUtils'; import { convertPositionToOffset, convertRangeToTextRange, convertTextRangeToRange } from '../common/positionUtils'; import { computeCompletionSimilarity } from '../common/stringUtils'; +import { TextEditTracker } from '../common/textEditTracker'; import { DocumentRange, doesRangeContain, @@ -52,6 +54,7 @@ import { Range, TextRange, } from '../common/textRange'; +import { TextRangeCollection } from '../common/textRangeCollection'; import { Duration, timingStats } from '../common/timing'; import { applyTextEditsToString } from '../common/workspaceEditUtils'; import { @@ -73,7 +76,7 @@ import { DefinitionFilter } from '../languageService/definitionProvider'; import { DocumentSymbolCollector, DocumentSymbolCollectorUseCase } from '../languageService/documentSymbolCollector'; import { IndexOptions, IndexResults, WorkspaceSymbolCallback } from '../languageService/documentSymbolProvider'; import { HoverResults } from '../languageService/hoverProvider'; -import { ImportAdder } from '../languageService/importAdder'; +import { ImportAdder, ImportData } from '../languageService/importAdder'; import { getModuleStatementIndentation, reindentSpan } from '../languageService/indentationUtils'; import { getInsertionPointForSymbolUnderModule } from '../languageService/insertionPointUtils'; import { ReferenceCallback, ReferencesResult } from '../languageService/referencesProvider'; @@ -89,10 +92,17 @@ import { Declaration } from './declaration'; import { getNameFromDeclaration } from './declarationUtils'; import { ImportResolver } from './importResolver'; import { ImportResult, ImportType } from './importResult'; -import { findNodeByOffset, getDocString, isBlankLine } from './parseTreeUtils'; +import { + findNodeByOffset, + findNodeByPosition, + getDocString, + getDottedName, + getDottedNameWithGivenNodeAsLastName, + isBlankLine, +} from './parseTreeUtils'; import { Scope } from './scope'; import { getScopeForNode } from './scopeUtils'; -import { IPythonMode, parseFile, SourceFile } from './sourceFile'; +import { IPythonMode, SourceFile } from './sourceFile'; import { isUserCode } from './sourceFileInfoUtils'; import { isStubFile, SourceMapper } from './sourceMapper'; import { Symbol } from './symbol'; @@ -2072,6 +2082,19 @@ export class Program { return undefined; } + // We will try to + // 1. Find symbol to move. + // 2. Update all references to the symbol to new location. + // 3. Remove the existing symbol. + // 4. Insert the symbol to the destination module. + // 5. Insert imports required for the symbol moved to the destination module. + // 6. Remove import no longer needed from the original module. + // + // Here all changes are done to edits, no features in LS will apply changes to + // program directly. All modification is done through LSP by a edit request so + // things like undo or edit stacks UI works. + + // 1. Find symbol to move. const execEnv = this._configOptions.findExecEnvironment(filePath); const declarations = DocumentSymbolCollector.getDeclarationsForNode( node, @@ -2095,8 +2118,10 @@ export class Program { return undefined; } + // 2. Update affected references. this._processModuleReferences(renameModuleProvider, node.value, filePath); + // 3. Remove existing symbols. const sourceDecl = renameModuleProvider.declarations.find( (d) => d.node && getFileExtension(d.path) === sourceFileExt ); @@ -2134,6 +2159,7 @@ export class Program { const reindentResult = reindentSpan(parseResults, symbolRange, insertionIndentation); const fullRange = RenameModuleProvider.getSymbolFullStatementTextRange(parseResults, sourceDecl); + renameModuleProvider.textEditTracker.addEdit( filePath, convertTextRangeToRange( @@ -2143,20 +2169,10 @@ export class Program { '' ); + // 4. Add the symbol to the destination file. const fileOperations: FileOperations[] = []; let codeSnippetToInsert = reindentResult.text; if (newFileParseResults) { - // TODO: We need to "add import" statement for symbols defined in the destination file - // if we couldn't find insertion point where all constraints are met. - // For example, if the symbol we are moving is used before other symbols it references are declared. - importAdder.applyImportsTo( - collectedimports, - newFileParseResults, - options.importFormat, - renameModuleProvider.textEditTracker, - token - ); - const range = convertTextRangeToRange( { start: insertionPoint, length: 0 }, newFileParseResults.tokenizerOutput.lines @@ -2170,39 +2186,224 @@ export class Program { renameModuleProvider.textEditTracker.addEdit(newFilePath, range, codeSnippetToInsert); } else { fileOperations.push({ kind: 'create', filePath: newFilePath }); + renameModuleProvider.textEditTracker.addEdit(newFilePath, getEmptyRange(), codeSnippetToInsert); + } - const tempParseResults = parseFile( - this._configOptions, - newFilePath, - codeSnippetToInsert, - IPythonMode.None, - new DiagnosticSink() - ); + // 5. Insert imports required for the symbol moved to the destination module. + // + // Since step 5 and 6 can create nested edits, we clone the program and apply all changes to re-calculate + // edits we need to apply to the destination file. The same workflow as `fix all` but done in program level + // not service level. + const cloned = this.clone(); + + let edits = renameModuleProvider.getEdits(); + + const textAfterSymbolAdded = applyTextEditsToString( + edits.filter((v) => v.filePath === newFilePath), + newFileParseResults?.tokenizerOutput.lines ?? new TextRangeCollection([]), + newFileInfo?.sourceFile.getFileContent() ?? '' + ); + + _updateFileContent(cloned, newFilePath, textAfterSymbolAdded); + + const textAfterImportsAdded = _tryGetTextAfterImportsAdded( + cloned, + newFilePath, + collectedimports, + insertionPoint, + token + ); + + edits = _updateFileEditActions( + edits, + newFilePath, + newFileParseResults, + textAfterSymbolAdded, + textAfterImportsAdded + ); + + // 6. Remove imports no longer required from original module. + const textAfterSymbolRemoved = applyTextEditsToString( + edits.filter((v) => v.filePath === filePath), + parseResults.tokenizerOutput.lines, + fileInfo.sourceFile.getFileContent()! + ); + + _updateFileContent(cloned, filePath, textAfterSymbolRemoved); + + const textAfterUnusedImportsRemoved = _tryGetTextAfterUnusedImportsRemoved( + cloned, + filePath, + collectedimports, + 0, + token + ); + + edits = _updateFileEditActions( + edits, + filePath, + parseResults, + textAfterSymbolRemoved, + textAfterUnusedImportsRemoved + ); + + cloned.dispose(); + + return { + edits, + fileOperations, + }; + + function _updateFileEditActions( + edits: FileEditAction[], + filePath: string, + parseResults: ParseResults | undefined, + oldText: string, + newText: string | undefined + ) { + if (newText === undefined || oldText === newText) { + return edits; + } + + // There were nested edits. Replace whole file. + edits = edits.filter((v) => v.filePath !== filePath); + edits.push({ + filePath, + range: parseResults + ? convertTextRangeToRange(parseResults.parseTree, parseResults.tokenizerOutput.lines) + : getEmptyRange(), + replacementText: newText, + }); + + return edits; + } + + function _tryGetTextAfterImportsAdded( + cloned: Program, + filePath: string, + importData: ImportData, + insertionPoint: number, + token: CancellationToken + ) { + const sourceFile = cloned.getBoundSourceFile(filePath); + const parseResults = sourceFile?.getParseResults(); + if (!parseResults) { + return undefined; + } const insertAddEdits = importAdder.applyImports( - collectedimports, - newFilePath, - tempParseResults, + importData, + filePath, + parseResults, insertionPoint, options.importFormat, token ); - const updateContent = applyTextEditsToString( + return applyTextEditsToString( insertAddEdits, - tempParseResults.tokenizerOutput.lines, - codeSnippetToInsert + parseResults.tokenizerOutput.lines, + sourceFile!.getFileContent()! ); - - renameModuleProvider.textEditTracker.addEdit(newFilePath, getEmptyRange(), updateContent); } - return { - edits: renameModuleProvider.getEdits(), - fileOperations, - }; + function _tryGetTextAfterUnusedImportsRemoved( + cloned: Program, + filePath: string, + importData: ImportData, + attempt: number, + token: CancellationToken + ): string | undefined { + throwIfCancellationRequested(token); + + cloned.analyzeFile(filePath, token); + + const sourceFile = cloned.getBoundSourceFile(filePath); + const parseResults = sourceFile?.getParseResults(); + if (!parseResults) { + return undefined; + } + + const tracker = new TextEditTracker(); + for (const diagnostic of cloned + .getDiagnosticsForRange( + filePath, + convertTextRangeToRange(parseResults.parseTree, parseResults.tokenizerOutput.lines) + ) + .filter( + (d) => + d.category === DiagnosticCategory.UnusedCode && + d.getActions()?.some((a) => a.action === Commands.unusedImport) + )) { + const nameNode = findNodeByPosition( + parseResults.parseTree, + diagnostic.range.start, + parseResults.tokenizerOutput.lines + ); + + if (nameNode?.nodeType !== ParseNodeType.Name) { + continue; + } + + // decl is synthesized. there is no node associated with the decl. + // ex) import a or import a.b + const dottedName1 = + nameNode.parent?.nodeType === ParseNodeType.ModuleName ? nameNode.parent.nameParts : [nameNode]; + + for (const [decl, names] of importData.declarations) { + if (decl.node) { + if (TextRange.containsRange(decl.node, nameNode)) { + tracker.removeNodes({ node: nameNode, parseResults: parseResults }); + break; + } + } + + const dottedName2 = getDottedName(getDottedNameWithGivenNodeAsLastName(names[0])); + if (dottedName2 && arrayEquals(dottedName1, dottedName2, (e1, e2) => e1.value === e2.value)) { + tracker.removeNodes({ node: nameNode, parseResults: parseResults }); + break; + } + } + } + + const oldText = sourceFile!.getFileContent()!; + const newText = applyTextEditsToString( + tracker.getEdits(token).filter((v) => v.filePath === filePath), + parseResults.tokenizerOutput.lines, + oldText + ); + + // We will attempt to remove unused imports multiple times since removing 1 unused import + // could make another import unused. This is due to how we calculate which import is not used. + // ex) import os, os.path, os.path.xxx + // `os.path` and `os.path.xxx` will be marked as used due to `import os`. + // once `os` is removed `os.path` will be marked as unused and so on. + // We will attempt to remove those chained unused imports up to 10 chain. + if (attempt > 10 || oldText === newText) { + return newText; + } + + _updateFileContent(cloned, filePath, newText); + return _tryGetTextAfterUnusedImportsRemoved(cloned, filePath, importData, attempt + 1, token); + } }); + function _updateFileContent(cloned: Program, filePath: string, text: string) { + const info = cloned.getSourceFileInfo(filePath); + const version = info ? (info.sourceFile.getClientVersion() ?? 0) + 1 : 0; + const chainedFilePath = info ? info.chainedSourceFile?.sourceFile.getFilePath() : undefined; + const ipythonMode = info ? info.sourceFile.getIPythonMode() : IPythonMode.None; + const isTracked = info ? info.isTracked : true; + const realFilePath = info ? info.sourceFile.getRealFilePath() : filePath; + + cloned.setFileOpened(filePath, version, [{ text }], { + chainedFilePath, + ipythonMode, + isTracked, + realFilePath, + }); + } + function _getNumberOfBlankLinesToInsert(parseResults: ParseResults, position: Position) { // This basically try to add 2 blanks lines before previous line with text. if (position.line === 0 && position.character === 0) { @@ -2221,6 +2422,42 @@ export class Program { } } + clone() { + const program = new Program( + this._importResolver, + this._configOptions, + this._console, + new LogTracker(this._console, 'Cloned') + ); + + // Cloned program will use whatever user files the program currently has. + const userFiles = this.getUserFiles(); + program.setTrackedFiles(userFiles.map((i) => i.sourceFile.getFilePath())); + program.markAllFilesDirty(true); + + // Make sure we keep editor content (open file) which could be different than one in the file system. + for (const fileInfo of this.getOpened()) { + const version = fileInfo.sourceFile.getClientVersion(); + if (version === undefined) { + continue; + } + + program.setFileOpened( + fileInfo.sourceFile.getFilePath(), + version, + [{ text: fileInfo.sourceFile.getOpenFileContents()! }], + { + chainedFilePath: fileInfo.chainedSourceFile?.sourceFile.getFilePath(), + ipythonMode: fileInfo.sourceFile.getIPythonMode(), + isTracked: fileInfo.isTracked, + realFilePath: fileInfo.sourceFile.getRealFilePath(), + } + ); + } + + return program; + } + canRenameSymbolAtPosition( filePath: string, position: Position, diff --git a/packages/pyright-internal/src/common/collectionUtils.ts b/packages/pyright-internal/src/common/collectionUtils.ts index 8b6bfd3bc..a5e1acaba 100644 --- a/packages/pyright-internal/src/common/collectionUtils.ts +++ b/packages/pyright-internal/src/common/collectionUtils.ts @@ -386,3 +386,11 @@ export function addIfNotNull(arr: T[], t: T): T[] { arr.push(t); return arr; } + +export function arrayEquals(c1: T[], c2: T[], predicate: (e1: T, e2: T) => boolean) { + if (c1.length !== c2.length) { + return false; + } + + return c1.every((v, i) => predicate(v, c2[i])); +} diff --git a/packages/pyright-internal/src/common/positionUtils.ts b/packages/pyright-internal/src/common/positionUtils.ts index e83e0659c..a512a917a 100644 --- a/packages/pyright-internal/src/common/positionUtils.ts +++ b/packages/pyright-internal/src/common/positionUtils.ts @@ -8,6 +8,7 @@ * line/column positions. */ +import { TokenizerOutput } from '../parser/tokenizer'; import { assert } from './debug'; import { Position, Range, TextRange } from './textRange'; import { TextRangeCollection } from './textRangeCollection'; @@ -69,3 +70,18 @@ export function convertRangeToTextRange(range: Range, lines: TextRangeCollection export function convertTextRangeToRange(range: TextRange, lines: TextRangeCollection): Range { return convertOffsetsToRange(range.start, TextRange.getEnd(range), lines); } + +// Returns the position of the last character in a line (before the newline). +export function getLineEndPosition(tokenizerOutput: TokenizerOutput, line: number): Position { + const lines = tokenizerOutput.lines; + const lineRange = lines.getItemAt(line); + // Character should be at the end of the line but before the newline. + const char = + line < lines.count - 1 + ? lineRange.length - tokenizerOutput.predominantEndOfLineSequence.length + : lineRange.length; + return { + line, + character: char, + }; +} diff --git a/packages/pyright-internal/src/languageService/completionProvider.ts b/packages/pyright-internal/src/languageService/completionProvider.ts index 92f6753a1..5e6aa7a57 100644 --- a/packages/pyright-internal/src/languageService/completionProvider.ts +++ b/packages/pyright-internal/src/languageService/completionProvider.ts @@ -833,44 +833,60 @@ export class CompletionProvider { return this._createSingleKeywordCompletion('else'); } + case ErrorExpressionCategory.MissingMemberAccessName: case ErrorExpressionCategory.MissingExpression: { // Don't show completion after random dots. const tokenizerOutput = this._parseResults.tokenizerOutput; const offset = convertPositionToOffset(this._position, tokenizerOutput.lines); const index = ParseTreeUtils.getTokenIndexAtLeft(tokenizerOutput.tokens, offset!); const token = ParseTreeUtils.getTokenAtIndex(tokenizerOutput.tokens, index); - if (token?.type === TokenType.Dot || token?.type === TokenType.Ellipsis) { - break; + const prevToken = ParseTreeUtils.getTokenAtIndex(tokenizerOutput.tokens, index - 1); + + if (node.category === ErrorExpressionCategory.MissingExpression) { + // Skip dots on expressions. + if (token?.type === TokenType.Dot || token?.type === TokenType.Ellipsis) { + break; + } + + // ex) class MyType: + // def is_str(self): ... + // myType = MyType() + // + // In incomplete code such as "myType.is" <= "is" will be tokenized as keyword not identifier, + // so even if user's intention is writing "is_str", completion after "is" won't include "is_str" + // since parser won't see "is" as partially written member name instead it will see it as + // expression statement with missing expression after "is" keyword. + // In such case, use "MyType." to get completion. + if (token?.type !== TokenType.Keyword || TextRange.getEnd(token) !== offset) { + return this._getExpressionCompletions(node, priorWord, priorText, postText); + } + + if (prevToken?.type !== TokenType.Dot) { + return this._getExpressionCompletions(node, priorWord, priorText, postText); + } + + const previousOffset = TextRange.getEnd(prevToken); + const previousNode = ParseTreeUtils.findNodeByOffset(this._parseResults.parseTree, previousOffset); + if ( + previousNode?.nodeType !== ParseNodeType.Error || + previousNode.category !== ErrorExpressionCategory.MissingMemberAccessName + ) { + return this._getExpressionCompletions(node, priorWord, priorText, postText); + } else { + // Update node to previous node so we get the member access completions. + node = previousNode; + } + } else if (node.category === ErrorExpressionCategory.MissingMemberAccessName) { + // Skip double dots on member access. + if ( + (token?.type === TokenType.Dot || token?.type === TokenType.Ellipsis) && + (prevToken?.type === TokenType.Dot || prevToken?.type === TokenType.Ellipsis) + ) { + return undefined; + } } - // ex) class MyType: - // def is_str(self): ... - // myType = MyType() - // - // In incomplete code such as "myType.is" <= "is" will be tokenized as keyword not identifier, - // so even if user's intention is writing "is_str", completion after "is" won't include "is_str" - // since parser won't see "is" as partially written member name instead it will see it as - // expression statement with missing expression after "is" keyword. - // In such case, use "MyType." to get completion. - if (token?.type !== TokenType.Keyword || TextRange.getEnd(token) !== offset) { - return this._getExpressionCompletions(node, priorWord, priorText, postText); - } - - const previousToken = ParseTreeUtils.getTokenAtIndex(tokenizerOutput.tokens, index - 1); - if (previousToken?.type !== TokenType.Dot) { - return this._getExpressionCompletions(node, priorWord, priorText, postText); - } - - const previousOffset = TextRange.getEnd(previousToken); - const previousNode = ParseTreeUtils.findNodeByOffset(this._parseResults.parseTree, previousOffset); - if ( - previousNode?.nodeType !== ParseNodeType.Error || - previousNode.category !== ErrorExpressionCategory.MissingMemberAccessName - ) { - return this._getExpressionCompletions(node, priorWord, priorText, postText); - } - - return this._getMissingMemberAccessNameCompletions(previousNode, previousOffset, priorWord); + return this._getMissingMemberAccessNameCompletions(node, priorWord); } case ErrorExpressionCategory.MissingDecoratorCallName: { @@ -887,11 +903,6 @@ export class CompletionProvider { return completionResults; } - case ErrorExpressionCategory.MissingMemberAccessName: { - const offset = convertPositionToOffset(this._position, this._parseResults.tokenizerOutput.lines); - return this._getMissingMemberAccessNameCompletions(node, offset!, priorWord); - } - case ErrorExpressionCategory.MissingFunctionParameterList: { if (node.child && node.child.nodeType === ParseNodeType.Name) { if (node.decorators?.some((d) => this._isOverload(d))) { @@ -909,14 +920,7 @@ export class CompletionProvider { return undefined; } - private _getMissingMemberAccessNameCompletions(node: ErrorNode, offset: number, priorWord: string) { - const index = ParseTreeUtils.getTokenIndexAtLeft(this._parseResults.tokenizerOutput.tokens, offset) - 1; - const previousToken = ParseTreeUtils.getTokenAtIndex(this._parseResults.tokenizerOutput.tokens, index); - if (previousToken?.type === TokenType.Dot || previousToken?.type === TokenType.Ellipsis) { - // Don't allow multiple dot bring up completions. - return undefined; - } - + private _getMissingMemberAccessNameCompletions(node: ErrorNode, priorWord: string) { if (!node.child || !isExpressionNode(node.child)) { return undefined; } @@ -2777,7 +2781,7 @@ export class CompletionProvider { position: this._position, }; - if (detail?.funcParensDisabled) { + if (detail?.funcParensDisabled || !this._options.snippet) { completionItemData.funcParensDisabled = true; } diff --git a/packages/pyright-internal/src/languageService/hoverProvider.ts b/packages/pyright-internal/src/languageService/hoverProvider.ts index c2b4a9133..d198c9896 100644 --- a/packages/pyright-internal/src/languageService/hoverProvider.ts +++ b/packages/pyright-internal/src/languageService/hoverProvider.ts @@ -121,6 +121,13 @@ export class HoverProvider { let primaryDeclaration = declarations[0]; if (primaryDeclaration.type === DeclarationType.Alias && declarations.length > 1) { primaryDeclaration = declarations[1]; + } else if ( + primaryDeclaration.type === DeclarationType.Variable && + declarations.length > 1 && + primaryDeclaration.isDefinedBySlots + ) { + // Slots cannot have docstrings, so pick the secondary. + primaryDeclaration = declarations[1]; } this._addResultsForDeclaration( diff --git a/packages/pyright-internal/src/languageService/importAdder.ts b/packages/pyright-internal/src/languageService/importAdder.ts index 7391849d7..fe3935397 100644 --- a/packages/pyright-internal/src/languageService/importAdder.ts +++ b/packages/pyright-internal/src/languageService/importAdder.ts @@ -16,6 +16,7 @@ import { isClassDeclaration, isFunctionDeclaration, isParameterDeclaration, + isUnresolvedAliasDeclaration, isVariableDeclaration, ModuleLoaderActions, } from '../analyzer/declaration'; @@ -145,7 +146,7 @@ export class ImportAdder { const execEnv = this._configOptions.findExecEnvironment(filePath); for (const decl of result.declarations.keys() ?? []) { const importInfo = this._getImportInfo(decl, filePath); - if (!importInfo) { + if (!importInfo || isUnresolvedAliasDeclaration(decl)) { continue; } @@ -378,6 +379,10 @@ class NameCollector extends ParseTreeWalker { } override visitName(name: NameNode) { + if (!TextRange.containsRange(this._range, name)) { + return false; + } + throwIfCancellationRequested(this._token); // We process dotted name as a whole rather than diff --git a/packages/pyright-internal/src/languageService/indentationUtils.ts b/packages/pyright-internal/src/languageService/indentationUtils.ts index cc53aebf0..a51a0fde7 100644 --- a/packages/pyright-internal/src/languageService/indentationUtils.ts +++ b/packages/pyright-internal/src/languageService/indentationUtils.ts @@ -159,7 +159,7 @@ function _getIndentation( preferDedent: boolean ): { token?: Token; indentation: number } { const tokens = parseResults.tokenizerOutput.tokens; - const startingToken = findPreviousNonWhitespaceToken(tokens, offset); + const startingToken = findNonWhitespaceTokenAtOrBeforeOffset(tokens, offset); if (!startingToken) { return { indentation: 0, @@ -468,7 +468,7 @@ function _getIndentationForNextLine(parseResults: ParseResults, prevToken: Token // Found a non whitespace token before we returned. whitespaceOnly = false; } - token = findPreviousNonWhitespaceToken(parseResults.tokenizerOutput.tokens, token.start - 1); + token = findNonWhitespaceTokenAtOrBeforeOffset(parseResults.tokenizerOutput.tokens, token.start - 1); } // No parenthesis found @@ -495,7 +495,7 @@ function _getFirstNonBlankLineIndentationFromText(parseResults: ParseResults, cu } function _findStringToken(tokens: TextRangeCollection, index: number): Token | undefined { - const token = _findPreviousNonWhitespaceTokenFromIndex(tokens, index); + const token = _findNonWhitespaceTokenAtOrBeforeIndex(tokens, index); if (!token) { return undefined; } @@ -503,19 +503,19 @@ function _findStringToken(tokens: TextRangeCollection, index: number): To return token.type === TokenType.String ? token : undefined; } -export function findPreviousNonWhitespaceToken(tokens: TextRangeCollection, offset: number): Token | undefined { +export function findNonWhitespaceTokenAtOrBeforeOffset( + tokens: TextRangeCollection, + offset: number +): Token | undefined { const index = tokens.getItemAtPosition(offset); if (index < 0) { return undefined; } - return _findPreviousNonWhitespaceTokenFromIndex(tokens, index); + return _findNonWhitespaceTokenAtOrBeforeIndex(tokens, index); } -function _findPreviousNonWhitespaceTokenFromIndex( - tokens: TextRangeCollection, - index: number -): Token | undefined { +function _findNonWhitespaceTokenAtOrBeforeIndex(tokens: TextRangeCollection, index: number): Token | undefined { for (let i = index; i >= 0; i--) { const token = _getTokenAtIndex(tokens, i); if (!token) { @@ -710,7 +710,7 @@ function _convertTokenStreams(parseResults: ParseResults, span: TextRange) { let endIndex = Math.min(tokens.getItemAtPosition(TextRange.getEnd(span)), tokens.length - 1); const endToken = _getTokenAtIndex(tokens, endIndex)!; - if (TextRange.getEnd(span) < endToken.start) { + if (TextRange.getEnd(span) <= endToken.start) { // ex) |< = span end [endToken] endIndex--; } @@ -761,6 +761,12 @@ function _convertTokenStreams(parseResults: ParseResults, span: TextRange) { // Handle text in whitespace that is not part of token stream. let previousInfo = tokenInfoArray[0]; const additionalTokens: TokenInfo[] = []; + if (previousInfo.kind === 'comment') { + // ex) token [#] comment + const start = startIndex === 0 ? 0 : TextRange.getEnd(_getTokenAtIndex(tokens, startIndex - 1)!); + _addTokenInfoIfMatch(parseResults, start, previousInfo.start, Char.Hash, additionalTokens); + } + for (let i = 1; i < tokenInfoArray.length; i++) { const info = tokenInfoArray[i]; @@ -807,9 +813,13 @@ function _convertTokenStreams(parseResults: ParseResults, span: TextRange) { // It is the first token in the file. previousInfo.firstTokenOnLine = true; } else { - const previousToken = _findPreviousNonWhitespaceTokenFromIndex(tokens, startIndex - 1)!; - const previousEnd = convertOffsetToPosition(TextRange.getEnd(previousToken), lines); - previousInfo.firstTokenOnLine = previousEnd.line !== previousInfo.range.start.line; + const previousNonWhitespaceToken = _findNonWhitespaceTokenAtOrBeforeIndex(tokens, startIndex - 1); + if (previousNonWhitespaceToken) { + const previousEnd = convertOffsetToPosition(TextRange.getEnd(previousNonWhitespaceToken), lines); + previousInfo.firstTokenOnLine = previousEnd.line !== previousInfo.range.start.line; + } else { + previousInfo.firstTokenOnLine = true; + } } previousInfo.multilineDocComment = _isMultilineDocComment(parseResults, previousInfo); diff --git a/packages/pyright-internal/src/languageService/renameModuleProvider.ts b/packages/pyright-internal/src/languageService/renameModuleProvider.ts index 510f82838..33ee95e99 100644 --- a/packages/pyright-internal/src/languageService/renameModuleProvider.ts +++ b/packages/pyright-internal/src/languageService/renameModuleProvider.ts @@ -32,6 +32,7 @@ import { getDottedNameWithGivenNodeAsLastName, getFirstAncestorOrSelfOfKind, getFullStatementRange, + getVariableDocStringNode, isFromImportAlias, isFromImportModuleName, isFromImportName, @@ -226,16 +227,31 @@ export class RenameModuleProvider { static getSymbolTextRange(parseResults: ParseResults, decl: Declaration): TextRange { if (isVariableDeclaration(decl)) { - const range = getFullStatementRange(decl.node, parseResults); - return convertRangeToTextRange(range, parseResults.tokenizerOutput.lines) ?? decl.node; + const assignment = getFirstAncestorOrSelfOfKind(decl.node, ParseNodeType.Assignment) ?? decl.node; + const range = getFullStatementRange(assignment, parseResults); + const textRange = convertRangeToTextRange(range, parseResults.tokenizerOutput.lines) ?? assignment; + + if (decl.docString !== undefined) { + const docNode = getVariableDocStringNode(decl.node); + if (docNode) { + TextRange.extend(textRange, docNode); + } + } + + return textRange; } return decl.node; } static getSymbolFullStatementTextRange(parseResults: ParseResults, decl: Declaration): TextRange { - const range = getFullStatementRange(decl.node, parseResults, { includeTrailingBlankLines: true }); - return convertRangeToTextRange(range, parseResults.tokenizerOutput.lines) ?? decl.node; + const statementNode = isVariableDeclaration(decl) + ? getFirstAncestorOrSelfOfKind(decl.node, ParseNodeType.Assignment) ?? decl.node + : decl.node; + const range = getFullStatementRange(statementNode, parseResults, { + includeTrailingBlankLines: true, + }); + return convertRangeToTextRange(range, parseResults.tokenizerOutput.lines) ?? statementNode; } static getRenameModulePath(declarations: Declaration[]) { diff --git a/packages/pyright-internal/src/tests/fourslash/completions.errorNodes.fourslash.ts b/packages/pyright-internal/src/tests/fourslash/completions.errorNodes.fourslash.ts new file mode 100644 index 000000000..33def1708 --- /dev/null +++ b/packages/pyright-internal/src/tests/fourslash/completions.errorNodes.fourslash.ts @@ -0,0 +1,27 @@ +/// + +// @filename: test.py +//// import os + +//// class App(): +//// def __init(self): +//// self.instance_path = "\\foo" + +//// app = App() +//// try: +//// os.makedirs(app.in[|/*marker*/|]) + +//// except: +//// pass + +// @ts-ignore +await helper.verifyCompletion('included', 'markdown', { + marker: { + completions: [ + { + label: 'instance_path', + kind: Consts.CompletionItemKind.Variable, + }, + ], + }, +}); diff --git a/packages/pyright-internal/src/tests/fourslash/hover.slots.fourslash.ts b/packages/pyright-internal/src/tests/fourslash/hover.slots.fourslash.ts new file mode 100644 index 000000000..5cf9e5d99 --- /dev/null +++ b/packages/pyright-internal/src/tests/fourslash/hover.slots.fourslash.ts @@ -0,0 +1,15 @@ +/// + +// @filename: test.py +//// class Chat: +//// __slots__ = ("id",) +//// +//// def __init__(self): +//// self.id = 1234 +//// """The ID of the channel.""" +//// +//// y = Chat() +//// y.[|/*marker*/id|] +helper.verifyHover('markdown', { + marker: '```python\n(variable) id: int\n```\n---\nThe ID of the channel.', +}); diff --git a/packages/pyright-internal/src/tests/importAdder.test.ts b/packages/pyright-internal/src/tests/importAdder.test.ts index e55e2fee9..44e902c70 100644 --- a/packages/pyright-internal/src/tests/importAdder.test.ts +++ b/packages/pyright-internal/src/tests/importAdder.test.ts @@ -1355,6 +1355,20 @@ test('use relative import format - textEditTracker', () => { testImportMoveWithTracker(code, ImportFormat.Relative); }); +test('dont include token not contained in the span', () => { + const code = ` +// @filename: test1.py +//// import random +//// +//// [|/*src*/answer_word = random.choice(["a","b","c","d"]) +//// |]guess_word = "c" + +// @filename: nested/__init__.py +//// [|{|"r":"import random!n!!n!!n!"|}|][|/*dest*/|] + `; + testImportMove(code, ImportFormat.Absolute); +}); + function testImportMoveWithTracker(code: string, importFormat = ImportFormat.Absolute) { const state = parseAndGetTestState(code).state; diff --git a/packages/pyright-internal/src/tests/indentationUtils.reindent.test.ts b/packages/pyright-internal/src/tests/indentationUtils.reindent.test.ts index 3c135abb6..586b1a423 100644 --- a/packages/pyright-internal/src/tests/indentationUtils.reindent.test.ts +++ b/packages/pyright-internal/src/tests/indentationUtils.reindent.test.ts @@ -387,6 +387,33 @@ test('re-indentation tab on multiline text', () => { testIndentation(code, 2, expected); }); +test('dont include token not contained in the span', () => { + const code = ` +//// import random +//// +//// [|/*marker*/answer_word = random.choice(["a","b","c","d"]) +//// |]guess_word = "c" + `; + + const expected = `answer_word = random.choice(["a","b","c","d"])`; + + testIndentation(code, 0, expected); +}); + +test('handle comment before first token', () => { + const code = ` +//// [|/*marker*/# this function doesn't do much +//// def myfunc(a, b): +//// return a + b|] + `; + + const expected = `# this function doesn't do much +def myfunc(a, b): + return a + b`; + + testIndentation(code, 0, expected); +}); + function testIndentation(code: string, indentation: number, expected: string, indentFirstToken = true) { const state = parseAndGetTestState(code).state; const range = state.getRangeByMarkerName('marker')!; diff --git a/packages/pyright-internal/src/tests/moveSymbol.importAdder.test.ts b/packages/pyright-internal/src/tests/moveSymbol.importAdder.test.ts index 432c9b395..d5cd3979d 100644 --- a/packages/pyright-internal/src/tests/moveSymbol.importAdder.test.ts +++ b/packages/pyright-internal/src/tests/moveSymbol.importAdder.test.ts @@ -12,16 +12,16 @@ import { testMoveSymbolAtPosition } from './renameModuleTestUtils'; test('move imports used in the symbol', () => { const code = ` // @filename: test.py -//// from typing import List, Mapping +//// [|{|"r":"!n!class MyType:!n! pass!n!!n!"|}from typing import List, Mapping //// //// class MyType: //// pass //// -//// [|{|"r":""|}def [|/*marker*/foo|](a: str, b: List[int]) -> None: +//// def [|/*marker*/foo|](a: str, b: List[int]) -> None: //// c: Mapping[str, MyType] = { 'hello', MyType() }|] // @filename: moved.py -//// [|{|"r":"from typing import List, Mapping!n!!n!!n!"|}|][|{|"r":"from test import MyType!n!!n!!n!"|}|][|{|"r":"def foo(a: str, b: List[int]) -> None:!n! c: Mapping[str, MyType] = { 'hello', MyType() }", "name": "dest"|}|] +//// [|{|"r":"from test import MyType!n!!n!!n!from typing import List, Mapping!n!!n!!n!def foo(a: str, b: List[int]) -> None:!n! c: Mapping[str, MyType] = { 'hello', MyType() }", "name": "dest"|}|] `; testFromCode(code); @@ -30,16 +30,16 @@ test('move imports used in the symbol', () => { test('import with alias', () => { const code = ` // @filename: test.py -//// from typing import List as l, Mapping as m +//// [|{|"r":"!n!class MyType:!n! pass!n!!n!"|}from typing import List as l, Mapping as m //// //// class MyType: //// pass //// -//// [|{|"r":""|}def [|/*marker*/foo|](a: str, b: l[int]) -> None: +//// def [|/*marker*/foo|](a: str, b: l[int]) -> None: //// c: m[str, MyType] = { 'hello', MyType() }|] // @filename: moved.py -//// [|{|"r":"from typing import List as l, Mapping as m!n!!n!!n!"|}|][|{|"r":"from test import MyType!n!!n!!n!"|}|][|{|"r":"def foo(a: str, b: l[int]) -> None:!n! c: m[str, MyType] = { 'hello', MyType() }", "name": "dest"|}|] +//// [|{|"r":"from test import MyType!n!!n!!n!from typing import List as l, Mapping as m!n!!n!!n!def foo(a: str, b: l[int]) -> None:!n! c: m[str, MyType] = { 'hello', MyType() }", "name": "dest"|}|] `; testFromCode(code); @@ -48,12 +48,12 @@ test('import with alias', () => { test('with existing imports', () => { const code = ` // @filename: test.py -//// from typing import List, Mapping +//// [|{|"r":"!n!class MyType:!n! pass!n!!n!"|}from typing import List, Mapping //// //// class MyType: //// pass //// -//// [|{|"r":""|}def [|/*marker*/foo|](a: str, b: List[int]) -> None: +//// def [|/*marker*/foo|](a: str, b: List[int]) -> None: //// c: Mapping[str, MyType] = { 'hello', MyType() }|] // @filename: moved.py @@ -67,7 +67,7 @@ test('with existing imports', () => { test('merge with existing imports', () => { const code = ` // @filename: test.py -//// from typing import List, Mapping +//// [|{|"r":"!n!class MyType:!n! pass!n!!n!class MyType2(MyType):!n! pass!n!!n!"|}from typing import List, Mapping //// //// class MyType: //// pass @@ -75,13 +75,13 @@ test('merge with existing imports', () => { //// class MyType2(MyType): //// pass //// -//// [|{|"r":""|}def [|/*marker*/foo|](a: str, b: List[int]) -> None: +//// def [|/*marker*/foo|](a: str, b: List[int]) -> None: //// c: Mapping[str, MyType] = { 'hello', MyType2() }|] // @filename: moved.py -//// from typing import Mapping[|{|"r":"!n!from typing import List"|}|] -//// from test import MyType[|{|"r":"!n!from test import MyType2"|}|] -//// m = MyType()[|{|"r":"!n!!n!!n!def foo(a: str, b: List[int]) -> None:!n! c: Mapping[str, MyType] = { 'hello', MyType2() }", "name": "dest"|}|] +//// [|{|"r":"from typing import List, Mapping!n!from test import MyType, MyType2!n!m = MyType()!n!!n!!n!def foo(a: str, b: List[int]) -> None:!n! c: Mapping[str, MyType] = { 'hello', MyType2() }", "name": "dest"|}from typing import Mapping +//// from test import MyType +//// m = MyType()|] `; testFromCode(code); @@ -90,12 +90,12 @@ test('merge with existing imports', () => { test('merge with existing moving symbol imports', () => { const code = ` // @filename: test.py -//// from typing import List, Mapping +//// [|{|"r":"!n!class MyType:!n! pass!n!!n!"|}from typing import List, Mapping //// //// class MyType: //// pass //// -//// [|{|"r":""|}def [|/*marker*/foo|](a: str, b: List[int]) -> None: +//// def [|/*marker*/foo|](a: str, b: List[int]) -> None: //// c: Mapping[str, MyType] = { 'hello', MyType() }|] // @filename: moved.py @@ -111,19 +111,19 @@ test('merge with existing moving symbol imports', () => { test('merge with existing moving symbol imports and add new one', () => { const code = ` // @filename: test.py -//// from typing import List, Mapping +//// [|{|"r":"!n!class MyType:!n! pass!n!!n!"|}from typing import List, Mapping //// //// class MyType: //// pass //// -//// [|{|"r":""|}def [|/*marker*/foo|](a: str, b: List[int]) -> None: +//// def [|/*marker*/foo|](a: str, b: List[int]) -> None: //// c: Mapping[str, MyType] = { 'hello', MyType() }|] // @filename: moved.py -//// from typing import List, Mapping -//// [|{|"r":""|}from test import foo[|{|"r":"!n!from test import MyType"|}|][|{|"r":"!n!!n!!n!def foo(a: str, b: List[int]) -> None:!n! c: Mapping[str, MyType] = { 'hello', MyType() }", "name": "dest"|}|] -//// |] -//// foo() +//// [|{|"r":"from typing import List, Mapping!n!!n!from test import MyType!n!!n!!n!def foo(a: str, b: List[int]) -> None:!n! c: Mapping[str, MyType] = { 'hello', MyType() }!n!!n!foo()", "name": "dest"|}from typing import List, Mapping +//// from test import foo +//// +//// foo()|] `; testFromCode(code); @@ -132,9 +132,9 @@ test('merge with existing moving symbol imports and add new one', () => { test('symbol from destination file used', () => { const code = ` // @filename: test.py -//// from moved import MyType +//// [|{|"r":"!n!"|}from moved import MyType //// -//// [|{|"r":""|}def [|/*marker*/foo|](a: MyType) -> None: +//// def [|/*marker*/foo|](a: MyType) -> None: //// c: Mapping[str, MyType] = { 'hello', a }|] // @filename: moved.py @@ -149,9 +149,9 @@ test('symbol from destination file used', () => { test('insert after all symbols references', () => { const code = ` // @filename: test.py -//// from moved import MyType +//// [|{|"r":"!n!"|}from moved import MyType //// -//// [|{|"r":""|}def [|/*marker*/foo|](a: MyType) -> None: +//// def [|/*marker*/foo|](a: MyType) -> None: //// c: Mapping[str, MyType] = { 'hello', a }|] // @filename: moved.py @@ -169,9 +169,9 @@ test('insert after all symbols references', () => { test('insert after all symbols references 2', () => { const code = ` // @filename: test.py -//// from moved import MyType +//// [|{|"r":"!n!"|}from moved import MyType //// -//// [|{|"r":""|}def [|/*marker*/foo|](a: MyType) -> None: +//// def [|/*marker*/foo|](a: MyType) -> None: //// c: Mapping[str, MyType] = { 'hello', a }|] // @filename: moved.py @@ -188,9 +188,9 @@ test('insert after all symbols references 2', () => { test('symbol used before all symbol references', () => { const code = ` // @filename: test.py -//// from moved import MyType +//// [|{|"r":"!n!"|}from moved import MyType //// -//// [|{|"r":""|}def [|/*marker*/foo|](a: MyType) -> None: +//// def [|/*marker*/foo|](a: MyType) -> None: //// c: Mapping[str, MyType] = { 'hello', a }|] // @filename: moved.py @@ -205,13 +205,136 @@ test('symbol used before all symbol references', () => { testFromCode(code); }); -function testFromCode(code: string) { +test('symbol with import statements', () => { + const code = ` +// @filename: test.py +//// [|{|"r": "import sys!n!!n!"|}import os, os.path, sys +//// +//// def [|/*marker*/foo|](): +//// p = os.path.curdir +//// os.abort()|] + +// @filename: moved.py +//// [|{|"r": "import os!n!import os.path!n!!n!!n!def foo():!n! p = os.path.curdir!n! os.abort()", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('symbol with import statements with alias', () => { + const code = ` +// @filename: test.py +//// [|{|"r": "import sys!n!!n!"|}import os, os.path as path, sys +//// +//// def [|/*marker*/foo|](): +//// p = path.curdir +//// os.abort()|] + +// @filename: moved.py +//// [|{|"r": "import os!n!import os.path as path!n!!n!!n!def foo():!n! p = path.curdir!n! os.abort()", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('symbol with import statements with alias 2', () => { + const code = ` +// @filename: test.py +//// [|{|"r": "import sys!n!!n!"|}import os, os.path as p1, sys +//// +//// def [|/*marker*/foo|](): +//// p = p1.curdir +//// os.abort()|] + +// @filename: moved.py +//// [|{|"r": "import os!n!import os.path as p1!n!!n!!n!def foo():!n! p = p1.curdir!n! os.abort()", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('symbol with import statements with multiple unused imports', () => { + const code = ` +// @filename: test.py +//// [|{|"r": "import os.path, sys!n!!n!"|}import os, os.path, sys +//// +//// def [|/*marker*/foo|](): +//// os.abort()|] + +// @filename: moved.py +//// [|{|"r": "import os!n!!n!!n!def foo():!n! os.abort()", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('symbol with import statements with used imports', () => { + const code = ` +// @filename: test.py +//// [|{|"r": "import os.path as path, sys!n!!n!p = path.curdir!n!!n!"|}import os, os.path as path, sys +//// +//// p = path.curdir +//// +//// def [|/*marker*/foo|](): +//// p = path.curdir +//// os.abort()|] + +// @filename: moved.py +//// [|{|"r": "import os!n!import os.path as path!n!!n!!n!def foo():!n! p = path.curdir!n! os.abort()", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('symbol with invalid import', () => { + const code = ` +// @filename: test.py +//// import notExist +//// +//// p = notExist.fooStr +//// +//// [|{|"r": ""|}def [|/*marker*/foo|](): +//// p = notExist.fooStr|] + +// @filename: moved.py +//// [|{|"r": "def foo():!n! p = notExist.fooStr", "name": "dest"|}|] + `; + + testFromCode(code, true); +}); + +test('symbol with import with error', () => { + const code = ` +// @filename: test.py +//// #pyright: strict +//// import lib # should have no stub diagnostic +//// +//// lib.bar() +//// +//// [|{|"r": ""|}def [|/*marker*/foo|](): +//// p = lib.bar()|] + +// @filename: lib/__init__.py +// @library: true +//// def bar(): pass + +// @filename: moved.py +//// [|{|"r": "import lib!n!!n!!n!def foo():!n! p = lib.bar()", "name": "dest"|}|] + `; + + testFromCode(code, true); +}); + +function testFromCode(code: string, expectsMissingImport = false) { const state = parseAndGetTestState(code).state; testMoveSymbolAtPosition( state, state.getMarkerByName('marker').fileName, state.getMarkerByName('dest').fileName, - state.getPositionRange('marker').start + state.getPositionRange('marker').start, + undefined, + undefined, + expectsMissingImport ); } diff --git a/packages/pyright-internal/src/tests/moveSymbol.insertion.test.ts b/packages/pyright-internal/src/tests/moveSymbol.insertion.test.ts index 84e0b03fe..89f297bc8 100644 --- a/packages/pyright-internal/src/tests/moveSymbol.insertion.test.ts +++ b/packages/pyright-internal/src/tests/moveSymbol.insertion.test.ts @@ -449,6 +449,120 @@ test('move variable with doc string', () => { testFromCode(code); }); +test('move a variable with another variable next line', () => { + const code = ` +// @filename: test.py +//// [|{|"r":"!n!guess_word = 'c'"|}import random +//// +//// [|/*marker*/answer_word|] = random.choice(['a','b','c','d']) +//// guess_word = 'c'|] + +// @filename: moved.py +//// [|{|"r":"import random!n!!n!!n!answer_word = random.choice(['a','b','c','d'])", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('Handle comments at the begining better 1', () => { + const code = ` +// @filename: test.py +//// # this function doesn't do much +//// [|{|"r":""|}def [|/*marker*/myfunc|](a, b): +//// return a + b|] + +// @filename: moved.py +//// [|{|"r":"def myfunc(a, b):!n! return a + b", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('Handle comments at the begining better 2', () => { + const code = ` +// @filename: test.py +//// import os +//// +//// [|{|"r":""|}# this function doesn't do much +//// def [|/*marker*/myfunc|](a, b): +//// return a + b|] + +// @filename: moved.py +//// [|{|"r":"# this function doesn't do much!n!def myfunc(a, b):!n! return a + b", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('variable with multiline expression', () => { + const code = ` +// @filename: test.py +//// [|{|"r":"!n!"|}from functools import partial +//// +//// [|/*marker*/sum1_2|] = partial(sum, +//// [1, +//// 2] +//// )|] + +// @filename: moved.py +//// [|{|"r":"from functools import partial!n!!n!!n!sum1_2 = partial(sum,!n![1,!n!2]!n!)", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('multiple variables in a single line 1', () => { + const code = ` +// @filename: test.py +//// [|{|"r":""|}[|/*marker*/a|] = 1; |]b = 1 + +// @filename: moved.py +//// [|{|"r":"a = 1;", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('multiple variables in a single line 2', () => { + const code = ` +// @filename: test.py +//// a = 1;[|{|"r":""|}[|/*marker*/b|] = 2|] + +// @filename: moved.py +//// [|{|"r":"b = 2", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('multiple variables in multiple lines 1', () => { + const code = ` +// @filename: test.py +//// [|{|"r":""|}[|/*marker*/a|] = \\ +//// 1 + 2; |]b = 3 + \\ +//// 4 + +// @filename: moved.py +//// [|{|"r":"a = \\\\!n! 1 + 2;", "name": "dest"|}|] + `; + + testFromCode(code); +}); + +test('multiple variables in multiple lines 2', () => { + const code = ` +// @filename: test.py +//// a = \\ +//// 1 + 2; [|{|"r":""|}[|/*marker*/b|] = 3 + \\ +//// 4|] + +// @filename: moved.py +//// [|{|"r":"b = 3 + \\\\!n! 4", "name": "dest"|}|] + `; + + testFromCode(code); +}); + function testFromCode(code: string) { const state = parseAndGetTestState(code).state; diff --git a/packages/pyright-internal/src/tests/renameModuleTestUtils.ts b/packages/pyright-internal/src/tests/renameModuleTestUtils.ts index e69bf5eb7..2faf93ab8 100644 --- a/packages/pyright-internal/src/tests/renameModuleTestUtils.ts +++ b/packages/pyright-internal/src/tests/renameModuleTestUtils.ts @@ -30,7 +30,8 @@ export function testMoveSymbolAtPosition( newFilePath: string, position: Position, text?: string, - replacementText?: string + replacementText?: string, + expectsMissingImport = false ) { const actions = state.program.moveSymbolAtPosition( filePath, @@ -60,7 +61,7 @@ export function testMoveSymbolAtPosition( .join('|')}` ); - _verifyFileOperations(state, actions, ranges, replacementText); + _verifyFileOperations(state, actions, ranges, replacementText, expectsMissingImport); } export function testRenameModule( @@ -102,18 +103,23 @@ function _verifyFileOperations( state: TestState, fileEditActions: FileEditActions, ranges: Range[], - replacementText: string | undefined + replacementText: string | undefined, + expectsMissingImport = false ) { const editsPerFileMap = createMapFromItems(fileEditActions.edits, (e) => e.filePath); - _verifyMissingImports(); + if (!expectsMissingImport) { + _verifyMissingImports(); + } verifyEdits(state, fileEditActions, ranges, replacementText); applyFileOperations(state, fileEditActions); // Make sure we don't have missing imports after the change. - _verifyMissingImports(); + if (!expectsMissingImport) { + _verifyMissingImports(); + } function _verifyMissingImports() { for (const editFileName of editsPerFileMap.keys()) {