Skip to content

Commit

Permalink
Added check for importing a Final variable from another module and …
Browse files Browse the repository at this point in the history
…then attempting to overwrite it. This partially addresses microsoft/pylance-release#6455.

Added check for an attempt to assign to a module-local variable if it is shadowing a `Final` variable declared by the builtins module or some other chained file. This addresses microsoft/pylance-release#6455.
  • Loading branch information
erictraut committed Dec 2, 2024
1 parent 8a9f066 commit 0f6f9c2
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 4 deletions.
69 changes: 67 additions & 2 deletions packages/pyright-internal/src/analyzer/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ import * as AnalyzerNodeInfo from './analyzerNodeInfo';
import { ConstraintTracker } from './constraintTracker';
import { getBoundCallMethod, getBoundInitMethod, getBoundNewMethod } from './constructors';
import { addInheritedDataClassEntries } from './dataClasses';
import { Declaration, DeclarationType, isAliasDeclaration } from './declaration';
import { Declaration, DeclarationType, isAliasDeclaration, isVariableDeclaration } from './declaration';
import { getNameNodeForDeclaration } from './declarationUtils';
import { deprecatedAliases, deprecatedSpecialForms } from './deprecatedSymbols';
import { getEnumDeclaredValueType, isEnumClassWithMembers, transformTypeForEnumMember } from './enums';
Expand All @@ -109,7 +109,7 @@ import * as ParseTreeUtils from './parseTreeUtils';
import { ParseTreeWalker } from './parseTreeWalker';
import { validateClassPattern } from './patternMatching';
import { isMethodOnlyProtocol, isProtocolUnsafeOverlap } from './protocols';
import { ScopeType } from './scope';
import { Scope, ScopeType } from './scope';
import { getScopeForNode } from './scopeUtils';
import { IPythonMode } from './sourceFile';
import { SourceMapper, isStubFile } from './sourceMapper';
Expand Down Expand Up @@ -3024,6 +3024,8 @@ export class Checker extends ParseTreeWalker {

this._reportIncompatibleDeclarations(name, symbol);

this._reportOverwriteOfImportedFinal(name, symbol);
this._reportOverwriteOfBuiltinsFinal(name, symbol, scope);
this._reportMultipleFinalDeclarations(name, symbol, scope.type);

this._reportFinalInLoop(symbol);
Expand Down Expand Up @@ -3205,6 +3207,69 @@ export class Checker extends ParseTreeWalker {
}
}

// If a variable that is marked Final in one module is imported by another
// module, an attempt to overwrite the imported symbol should generate an
// error.
private _reportOverwriteOfImportedFinal(name: string, symbol: Symbol) {
if (this._evaluator.isFinalVariable(symbol)) {
return;
}

const decls = symbol.getDeclarations();

const finalImportDecl = decls.find((decl) => {
if (decl.type === DeclarationType.Alias) {
const resolvedDecl = this._evaluator.resolveAliasDeclaration(decl, /* resolveLocalNames */ true);
if (resolvedDecl && isVariableDeclaration(resolvedDecl) && resolvedDecl.isFinal) {
return true;
}
}

return false;
});

if (!finalImportDecl) {
return;
}

decls.forEach((decl) => {
if (decl !== finalImportDecl) {
this._evaluator.addDiagnostic(
DiagnosticRule.reportGeneralTypeIssues,
LocMessage.finalReassigned().format({ name }),
getNameNodeForDeclaration(decl) ?? decl.node
);
}
});
}

// If the builtins module (or any implicitly chained module) defines a
// Final variable, an attempt to overwrite it should generate an error.
private _reportOverwriteOfBuiltinsFinal(name: string, symbol: Symbol, scope: Scope) {
if (scope.type !== ScopeType.Module || !scope.parent) {
return;
}

const shadowedSymbolInfo = scope.parent.lookUpSymbolRecursive(name);
if (!shadowedSymbolInfo) {
return;
}

if (!this._evaluator.isFinalVariable(shadowedSymbolInfo.symbol)) {
return;
}

const decls = symbol.getDeclarations();
decls.forEach((decl) => {
this._evaluator.addDiagnostic(
DiagnosticRule.reportGeneralTypeIssues,
LocMessage.finalReassigned().format({ name }),
getNameNodeForDeclaration(decl) ?? decl.node
);
});
}

// If a variable is marked Final, it should receive only one assigned value.
private _reportMultipleFinalDeclarations(name: string, symbol: Symbol, scopeType: ScopeType) {
if (!this._evaluator.isFinalVariable(symbol)) {
return;
Expand Down
4 changes: 2 additions & 2 deletions packages/pyright-internal/src/analyzer/typeEvaluatorTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import { AnalyzerFileInfo } from './analyzerFileInfo';
import { CodeFlowReferenceExpressionNode, FlowNode } from './codeFlowTypes';
import { ConstraintTracker } from './constraintTracker';
import { Declaration } from './declaration';
import * as DeclarationUtils from './declarationUtils';
import { ResolvedAliasInfo } from './declarationUtils';
import { SymbolWithScope } from './scope';
import { Symbol, SynthesizedTypeInfo } from './symbol';
import { PrintTypeFlags } from './typePrinter';
Expand Down Expand Up @@ -684,7 +684,7 @@ export interface TypeEvaluator {
declaration: Declaration,
resolveLocalNames: boolean,
options?: ResolveAliasOptions
) => DeclarationUtils.ResolvedAliasInfo | undefined;
) => ResolvedAliasInfo | undefined;
getTypeOfIterable: (
typeResult: TypeResult,
isAsync: boolean,
Expand Down
8 changes: 8 additions & 0 deletions packages/pyright-internal/src/tests/samples/final7.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# This sample is used in conjunction with final8.py to test that imported
# Final symbols cannot be overwritten.

from typing import Final


var1: Final[int] = 1
var2: Final = 2
20 changes: 20 additions & 0 deletions packages/pyright-internal/src/tests/samples/final8.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# This sample tests that final variables imported from another module
# cannot be overwritten.

from .final7 import *

# This should generate an error.
var1 = 1

# This should generate an error.
var2 = 1


def func1():
from .final7 import var1, var2

# This should generate an error.
var1 = 1

# This should generate an error.
var2 = 1
5 changes: 5 additions & 0 deletions packages/pyright-internal/src/tests/typeEvaluator4.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ test('Final6', () => {
TestUtils.validateResults(analysisResults, 2);
});

test('Final8', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['final8.py']);
TestUtils.validateResults(analysisResults, 4);
});

test('InferredTypes1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['inferredTypes1.py']);
TestUtils.validateResults(analysisResults, 0);
Expand Down

0 comments on commit 0f6f9c2

Please sign in to comment.