Skip to content

Commit

Permalink
Changed the handling of Final variables (that are not explicitly ma…
Browse files Browse the repository at this point in the history 
…rked `ClassVar`) within dataclass class bodies. This is consistent with the runtime and [this proposed change to the typing spec](python/typing#1669).
  • Loading branch information
@erictraut
erictraut committed Mar 31, 2024
1 parent 7997351 commit b085a26
Show file tree
Hide file tree
Showing 8 changed files with 59 additions and 24 deletions.
18 changes: 11 additions & 7 deletions packages/pyright-internal/src/analyzer/binder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3702,7 +3702,7 @@ export class Binder extends ParseTreeWalker {
}

// Is this annotation indicating that the variable is a "ClassVar"?
let classVarInfo = this._isAnnotationClassVar(typeAnnotation);
const classVarInfo = this._isAnnotationClassVar(typeAnnotation);

if (classVarInfo.isClassVar) {
if (!classVarInfo.classVarTypeNode) {
Expand All @@ -3711,7 +3711,10 @@ export class Binder extends ParseTreeWalker {
}

// PEP 591 indicates that a Final variable initialized within a class
// body should also be considered a ClassVar.
// body should also be considered a ClassVar unless it's in a dataclass.
// We can't tell at this stage whether it's a dataclass, so we'll simply
// record whether it's a Final assigned in a class body.
let isFinalAssignedInClassBody = false;
if (finalInfo.isFinal) {
const containingClass = ParseTreeUtils.getEnclosingClassOrFunction(target);
if (containingClass && containingClass.nodeType === ParseNodeType.Class) {
Expand All @@ -3720,10 +3723,7 @@ export class Binder extends ParseTreeWalker {
target.parent?.nodeType === ParseNodeType.Assignment ||
target.parent?.parent?.nodeType === ParseNodeType.Assignment
) {
classVarInfo = {
isClassVar: true,
classVarTypeNode: undefined,
};
isFinalAssignedInClassBody = true;
}
}
}
Expand All @@ -3743,9 +3743,13 @@ export class Binder extends ParseTreeWalker {
};
symbolWithScope.symbol.addDeclaration(declaration);

if (isFinalAssignedInClassBody) {
symbolWithScope.symbol.setIsFinalVarInClassBody();
}

if (classVarInfo.isClassVar) {
symbolWithScope.symbol.setIsClassVar();
} else {
} else if (!isFinalAssignedInClassBody) {
symbolWithScope.symbol.setIsInstanceMember();
}

Expand Down
5 changes: 1 addition & 4 deletions packages/pyright-internal/src/analyzer/dataClasses.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,11 +323,8 @@ export function synthesizeDataClassMethods(
// Don't include class vars. PEP 557 indicates that they shouldn't
// be considered data class entries.
const variableSymbol = classType.details.fields.get(variableName);
const isFinal = variableSymbol
?.getDeclarations()
.some((decl) => decl.type === DeclarationType.Variable && decl.isFinal);

if (variableSymbol?.isClassVar() && !isFinal) {
if (variableSymbol?.isClassVar() && !variableSymbol?.isFinalVarInClassBody()) {
// If an ancestor class declared an instance variable but this dataclass
// declares a ClassVar, delete the older one from the full data class entries.
// We exclude final variables here because a Final type annotation is implicitly
Expand Down
6 changes: 3 additions & 3 deletions packages/pyright-internal/src/analyzer/protocols.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { assignTypeToTypeVar } from './constraintSolver';
import { DeclarationType } from './declaration';
import { assignProperty } from './properties';
import { Symbol } from './symbol';
import { getLastTypedDeclaredForSymbol } from './symbolUtils';
import { getLastTypedDeclaredForSymbol, isEffectivelyClassVar } from './symbolUtils';
import { TypeEvaluator } from './typeEvaluatorTypes';
import {
ClassType,
Expand Down Expand Up @@ -635,8 +635,8 @@ function assignClassToProtocolInternal(
typesAreConsistent = false;
}

const isDestClassVar = destSymbol.isClassVar();
const isSrcClassVar = srcSymbol.isClassVar();
const isDestClassVar = isEffectivelyClassVar(destSymbol, /* isDataclass */ false);
const isSrcClassVar = isEffectivelyClassVar(srcSymbol, /* isDataclass */ false);
const isSrcVariable = srcSymbol.getDeclarations().some((decl) => decl.type === DeclarationType.Variable);

if (sourceIsClassObject) {
Expand Down
13 changes: 13 additions & 0 deletions packages/pyright-internal/src/analyzer/symbol.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ export const enum SymbolFlags {

// Indicates that the symbol should be exempt from override type checks.
IgnoredForOverrideChecks = 1 << 12,

// Indicates that the symbol is marked Final and is assigned a value
// in the class body. The typing spec indicates that these should be
// considered ClassVars unless they are found in a dataclass.
FinalVarInClassBody = 1 << 13,
}

let nextSymbolId = 1;
Expand Down Expand Up @@ -143,6 +148,14 @@ export class Symbol {
return !!(this._flags & SymbolFlags.ClassVar);
}

setIsFinalVarInClassBody() {
this._flags |= SymbolFlags.FinalVarInClassBody;
}

isFinalVarInClassBody() {
return !!(this._flags & SymbolFlags.FinalVarInClassBody);
}

setIsInitVar() {
this._flags |= SymbolFlags.InitVar;
}
Expand Down
12 changes: 12 additions & 0 deletions packages/pyright-internal/src/analyzer/symbolUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,15 @@ export function isTypedDictMemberAccessedThroughIndex(symbol: Symbol): boolean {
export function isVisibleExternally(symbol: Symbol) {
return !symbol.isExternallyHidden() && !symbol.isPrivatePyTypedImport();
}

export function isEffectivelyClassVar(symbol: Symbol, isInDataclass: boolean) {
if (symbol.isClassVar()) {
return true;
}

if (symbol.isFinalVarInClassBody()) {
return !isInDataclass;
}

return false;
}
17 changes: 13 additions & 4 deletions packages/pyright-internal/src/analyzer/typeEvaluator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ import * as ScopeUtils from './scopeUtils';
import { evaluateStaticBoolExpression } from './staticExpressions';
import { Symbol, SymbolFlags, indeterminateSymbolId } from './symbol';
import { isConstantName, isPrivateName, isPrivateOrProtectedName } from './symbolNameUtils';
import { getLastTypedDeclaredForSymbol } from './symbolUtils';
import { getLastTypedDeclaredForSymbol, isEffectivelyClassVar } from './symbolUtils';
import { SpeculativeModeOptions, SpeculativeTypeTracker } from './typeCacheUtils';
import {
AbstractSymbol,
Expand Down Expand Up @@ -5627,7 +5627,11 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
// this is normally considered a type violation. But it is allowed
// if the class variable is a descriptor object. In this case, we will
// clear the flag that causes an error to be generated.
if (usage.method === 'set' && memberInfo.symbol.isClassVar() && isAccessedThroughObject) {
if (
usage.method === 'set' &&
isEffectivelyClassVar(memberInfo.symbol, ClassType.isDataClass(containingClassType)) &&
isAccessedThroughObject
) {
const selfClass = selfType ?? memberName === '__new__' ? undefined : classType;
const typeResult = getTypeOfMemberInternal(errorNode, memberInfo, selfClass, flags);

Expand Down Expand Up @@ -5769,7 +5773,8 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
// Check for an attempt to overwrite or delete a ClassVar member from an instance.
if (
!isDescriptorApplied &&
memberInfo?.symbol.isClassVar() &&
memberInfo &&
isEffectivelyClassVar(memberInfo.symbol, ClassType.isDataClass(classType)) &&
(flags & MemberAccessFlags.DisallowClassVarWrites) !== 0
) {
diag?.addMessage(LocAddendum.memberSetClassVar().format({ name: memberName }));
Expand Down Expand Up @@ -21390,7 +21395,11 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
// If the symbol is explicitly marked as a ClassVar, consider only the
// declarations that assign to it from within the class body, not through
// a member access expression.
if (symbol.isClassVar() && decl.type === DeclarationType.Variable && decl.isDefinedByMemberAccess) {
if (
isEffectivelyClassVar(symbol, /* isDataclass */ false) &&
decl.type === DeclarationType.Variable &&
decl.isDefinedByMemberAccess
) {
return;
}

Expand Down
10 changes: 5 additions & 5 deletions packages/pyright-internal/src/analyzer/typeUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { assert } from '../common/debug';
import { ParameterCategory } from '../parser/parseNodes';
import { DeclarationType } from './declaration';
import { Symbol, SymbolFlags, SymbolTable } from './symbol';
import { isTypedDictMemberAccessedThroughIndex } from './symbolUtils';
import { isEffectivelyClassVar, isTypedDictMemberAccessedThroughIndex } from './symbolUtils';
import {
AnyType,
ClassType,
Expand Down Expand Up @@ -1645,7 +1645,7 @@ export function getProtocolSymbolsRecursive(
classType,
isInstanceMember: symbol.isInstanceMember(),
isClassMember: symbol.isClassMember(),
isClassVar: symbol.isClassVar(),
isClassVar: isEffectivelyClassVar(symbol, /* isDataclass */ false),
isTypeDeclared: symbol.hasTypedDeclarations(),
skippedUndeclaredType: false,
});
Expand Down Expand Up @@ -1807,7 +1807,7 @@ export function* getClassMemberIterator(
symbol,
isInstanceMember: true,
isClassMember: symbol.isClassMember(),
isClassVar: symbol.isClassVar(),
isClassVar: isEffectivelyClassVar(symbol, ClassType.isDataClass(specializedMroClass)),
classType: specializedMroClass,
isTypeDeclared: hasDeclaredType,
skippedUndeclaredType,
Expand Down Expand Up @@ -1847,7 +1847,7 @@ export function* getClassMemberIterator(
symbol,
isInstanceMember,
isClassMember,
isClassVar: symbol.isClassVar(),
isClassVar: isEffectivelyClassVar(symbol, isDataclass),
classType: specializedMroClass,
isTypeDeclared: hasDeclaredType,
skippedUndeclaredType,
Expand Down Expand Up @@ -1942,7 +1942,7 @@ export function getClassFieldsRecursive(classType: ClassType): Map<string, Class
symbol,
isInstanceMember: symbol.isInstanceMember(),
isClassMember: symbol.isClassMember(),
isClassVar: symbol.isClassVar(),
isClassVar: isEffectivelyClassVar(symbol, ClassType.isDataClass(specializedMroClass)),
isTypeDeclared: true,
skippedUndeclaredType: false,
});
Expand Down
2 changes: 1 addition & 1 deletion packages/pyright-internal/src/tests/typeEvaluator3.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -820,7 +820,7 @@ test('Classes5', () => {

configOptions.diagnosticRuleSet.reportIncompatibleVariableOverride = 'error';
analysisResults = TestUtils.typeAnalyzeSampleFiles(['classes5.py'], configOptions);
TestUtils.validateResults(analysisResults, 36);
TestUtils.validateResults(analysisResults, 35);
});

test('Classes6', () => {
Expand Down

0 comments on commit b085a26

Please sign in to comment.