Added new diagnostic rule "reportPropertyTypeMismatch" that verifies that the type of the input parameter to a property's setter is assignable to the return type of the getter.

This commit is contained in:
Eric Traut 2020-09-05 14:55:12 -07:00
parent 01637af00c
commit cea376e59b
11 changed files with 164 additions and 28 deletions

View File

@ -48,6 +48,8 @@ The following settings control pyrights diagnostic output (warnings or errors
**reportGeneralTypeIssues** [boolean or string, optional]: Generate or suppress diagnostics for general type inconsistencies, unsupported operations, argument/parameter mismatches, etc. This covers all of the basic type-checking rules not covered by other rules. It does not include syntax errors. The default value for this setting is 'error'.
**reportPropertyTypeMismatch** [boolean or string, optional]: Generate or suppress diagnostics for properties where the type of the value passed to the setter is not assignable to the value returned by the getter. Such mismatches violate the intended use of properties, which are meant to act like variables. The default value for this setting is 'error'.
**reportMissingImports** [boolean or string, optional]: Generate or suppress diagnostics for imports that have no corresponding imported python file or type stub file. The default value for this setting is 'none', although pyright can do a much better job of static type checking if type stub files are provided for all imports.
**reportMissingModuleSource** [boolean or string, optional]: Generate or suppress diagnostics for imports that have no corresponding source file. This happens when a type stub is found, but the module source file was not found, indicating that the code may fail at runtime when using this execution environment. Type checking will be done using the type stub. The default value for this setting is 'warning'.

View File

@ -9180,7 +9180,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
for (let i = node.decorators.length - 1; i >= 0; i--) {
const decorator = node.decorators[i];
const newDecoratedType = applyFunctionDecorator(decoratedType, functionType, decorator);
const newDecoratedType = applyFunctionDecorator(decoratedType, functionType, decorator, node);
if (isUnknown(newDecoratedType)) {
// Report this error only on the first unknown type.
if (!foundUnknown) {
@ -9342,7 +9342,8 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
function applyFunctionDecorator(
inputFunctionType: Type,
originalFunctionType: FunctionType,
decoratorNode: DecoratorNode
decoratorNode: DecoratorNode,
functionNode: FunctionNode
): Type {
const fileInfo = getFileInfo(decoratorNode);
@ -9377,7 +9378,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
if (isProperty(baseType)) {
const memberName = decoratorNode.leftExpression.memberName.value;
if (memberName === 'setter') {
return clonePropertyWithSetter(baseType, originalFunctionType);
return clonePropertyWithSetter(baseType, originalFunctionType, functionNode);
} else if (memberName === 'deleter') {
return clonePropertyWithDeleter(baseType, originalFunctionType);
}
@ -9479,7 +9480,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
return propertyObject;
}
function clonePropertyWithSetter(prop: Type, fset: FunctionType): Type {
function clonePropertyWithSetter(prop: Type, fset: FunctionType, errorNode: FunctionNode): Type {
if (!isProperty(prop)) {
return prop;
}
@ -9505,6 +9506,34 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
}
});
// Verify parameters for fset.
if (errorNode.parameters.length >= 2 && errorNode.parameters[1].typeAnnotation) {
// Verify consistency of the type.
const fgetType = getGetterTypeFromProperty(classType, /* inferTypeIfNeeded */ false);
if (fgetType && !isAnyOrUnknown(fgetType)) {
const fsetType = getTypeOfAnnotation(errorNode.parameters[1].typeAnnotation);
// The setter type should be assignable to the getter type.
const diag = new DiagnosticAddendum();
if (
!canAssignType(
fgetType,
fsetType,
diag,
/* typeVarMap */ undefined,
CanAssignFlags.DoNotSpecializeTypeVars
)
) {
addDiagnostic(
getFileInfo(errorNode).diagnosticRuleSet.reportPropertyTypeMismatch,
DiagnosticRule.reportPropertyTypeMismatch,
Localizer.Diagnostic.setterGetterTypeMismatch() + diag.getString(),
errorNode.parameters[1].typeAnnotation
);
}
}
}
// Fill in the fset method.
const fsetSymbol = Symbol.createWithType(SymbolFlags.ClassMember, fset);
fields.set('fset', fsetSymbol);
@ -13121,31 +13150,20 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
if (ClassType.isPropertyClass(destType) && ClassType.isPropertyClass(srcType)) {
let typesAreConsistent = true;
const fgetDest = destType.details.fields.get('fget');
const fgetSrc = srcType.details.fields.get('fget');
if (fgetDest && fgetSrc) {
const fgetDestType = getDeclaredTypeOfSymbol(fgetDest);
const fgetSrcType = getDeclaredTypeOfSymbol(fgetSrc);
const fgetDestReturnType = getGetterTypeFromProperty(destType, /* inferTypeIfNeeded */ true);
const fgetSrcReturnType = getGetterTypeFromProperty(srcType, /* inferTypeIfNeeded */ true);
if (fgetDestReturnType && fgetSrcReturnType) {
if (
fgetDestType &&
fgetSrcType &&
fgetDestType.category === TypeCategory.Function &&
fgetSrcType.category === TypeCategory.Function
!canAssignType(
fgetDestReturnType,
fgetSrcReturnType,
diag,
/* typeVarMap */ undefined,
CanAssignFlags.Default,
recursionCount + 1
)
) {
const fgetDestReturnType = getFunctionEffectiveReturnType(fgetDestType);
const fgetSrcReturnType = getFunctionEffectiveReturnType(fgetSrcType);
if (
!canAssignType(
fgetDestReturnType,
fgetSrcReturnType,
diag,
/* typeVarMap */ undefined,
CanAssignFlags.Default,
recursionCount + 1
)
) {
typesAreConsistent = false;
}
typesAreConsistent = false;
}
}
@ -13350,6 +13368,23 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
return true;
}
function getGetterTypeFromProperty(propertyClass: ClassType, inferTypeIfNeeded: boolean): Type | undefined {
if (!ClassType.isPropertyClass(propertyClass)) {
return undefined;
}
const fgetSymbol = propertyClass.details.fields.get('fget');
if (fgetSymbol) {
const fgetType = getDeclaredTypeOfSymbol(fgetSymbol);
if (fgetType && fgetType.category === TypeCategory.Function) {
return getFunctionEffectiveReturnType(fgetType, /* args */ undefined, inferTypeIfNeeded);
}
}
return undefined;
}
function verifyTypeArgumentsAssignable(
destType: ClassType,
srcType: ClassType,
@ -13748,6 +13783,19 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
);
}
if (flags & CanAssignFlags.DoNotSpecializeTypeVars) {
if (destType !== srcType) {
diag.addMessage(
Localizer.DiagnosticAddendum.typeAssignmentMismatch().format({
sourceType: printType(srcType),
destType: printType(destType),
})
);
return false;
}
return true;
}
const specializedSrcType = getConcreteTypeFromTypeVar(srcType);
return canAssignType(destType, specializedSrcType, diag, undefined, flags, recursionCount + 1);
}

View File

@ -102,6 +102,10 @@ export const enum CanAssignFlags {
// For function types, skip the return type check.
SkipFunctionReturnTypeCheck = 1 << 4,
// Normally type vars are specialized during type comparisons.
// With this flag, a type var must match a type var exactly.
DoNotSpecializeTypeVars = 1 << 5,
}
const singleTickRegEx = /'/g;
@ -508,7 +512,7 @@ export function isParamSpecType(type: Type): boolean {
return type.isParamSpec;
}
export function isProperty(type: Type): boolean {
export function isProperty(type: Type): type is ObjectType {
return isObject(type) && ClassType.isPropertyClass(type.classType);
}

View File

@ -80,6 +80,9 @@ export interface DiagnosticRuleSet {
// Report general type issues?
reportGeneralTypeIssues: DiagnosticLevel;
// Report mismatch in types between property getter and setter?
reportPropertyTypeMismatch: DiagnosticLevel;
// Report missing imports?
reportMissingImports: DiagnosticLevel;
@ -226,6 +229,7 @@ export function getBooleanDiagnosticRules() {
export function getDiagLevelDiagnosticRules() {
return [
DiagnosticRule.reportGeneralTypeIssues,
DiagnosticRule.reportPropertyTypeMismatch,
DiagnosticRule.reportMissingImports,
DiagnosticRule.reportMissingModuleSource,
DiagnosticRule.reportMissingTypeStubs,
@ -285,6 +289,7 @@ export function getOffDiagnosticRuleSet(): DiagnosticRuleSet {
strictParameterNoneValue: false,
enableTypeIgnoreComments: true,
reportGeneralTypeIssues: 'none',
reportPropertyTypeMismatch: 'none',
reportMissingImports: 'warning',
reportMissingModuleSource: 'warning',
reportMissingTypeStubs: 'none',
@ -340,6 +345,7 @@ export function getBasicDiagnosticRuleSet(): DiagnosticRuleSet {
strictParameterNoneValue: false,
enableTypeIgnoreComments: true,
reportGeneralTypeIssues: 'error',
reportPropertyTypeMismatch: 'error',
reportMissingImports: 'error',
reportMissingModuleSource: 'warning',
reportMissingTypeStubs: 'none',
@ -395,6 +401,7 @@ export function getStrictDiagnosticRuleSet(): DiagnosticRuleSet {
strictParameterNoneValue: true,
enableTypeIgnoreComments: true, // Not overridden by strict mode
reportGeneralTypeIssues: 'error',
reportPropertyTypeMismatch: 'error',
reportMissingImports: 'error',
reportMissingModuleSource: 'warning',
reportMissingTypeStubs: 'error',
@ -747,6 +754,13 @@ export class ConfigOptions {
defaultSettings.reportGeneralTypeIssues
),
// Read the "reportPropertyTypeMismatch" entry.
reportPropertyTypeMismatch: this._convertDiagnosticLevel(
configObj.reportPropertyTypeMismatch,
DiagnosticRule.reportPropertyTypeMismatch,
defaultSettings.reportPropertyTypeMismatch
),
// Read the "reportMissingImports" entry.
reportMissingImports: this._convertDiagnosticLevel(
configObj.reportMissingImports,

View File

@ -17,6 +17,7 @@ export enum DiagnosticRule {
enableTypeIgnoreComments = 'enableTypeIgnoreComments',
reportGeneralTypeIssues = 'reportGeneralTypeIssues',
reportPropertyTypeMismatch = 'reportPropertyTypeMismatch',
reportMissingImports = 'reportMissingImports',
reportMissingModuleSource = 'reportMissingModuleSource',
reportMissingTypeStubs = 'reportMissingTypeStubs',

View File

@ -491,6 +491,7 @@ export namespace Localizer {
export const returnTypeUnknown = () => getRawString('Diagnostic.returnTypeUnknown');
export const returnTypePartiallyUnknown = () =>
new ParameterizedString<{ returnType: string }>(getRawString('Diagnostic.returnTypePartiallyUnknown'));
export const setterGetterTypeMismatch = () => getRawString('Diagnostic.setterGetterTypeMismatch');
export const singleOverload = () =>
new ParameterizedString<{ name: string }>(getRawString('Diagnostic.singleOverload'));
export const staticClsSelfParam = () => getRawString('Diagnostic.staticClsSelfParam');

View File

@ -233,6 +233,7 @@
"returnTypeMismatch": "Expression of type \"{exprType}\" cannot be assigned to return type \"{returnType}\"",
"returnTypeUnknown": "Return type is unknown",
"returnTypePartiallyUnknown": "Return type, \"{returnType}\", is partially unknown",
"setterGetterTypeMismatch": "Property setter value type is not assignable to the getter return type",
"singleOverload": "\"{name}\" is marked as overload, but additional overloads are missing",
"staticClsSelfParam": "Static methods should not take a \"self\" or \"cls\" parameter",
"stringNonAsciiBytes": "Non-ASCII character not allowed in bytes string literal",

View File

@ -624,6 +624,18 @@ test('Properties5', () => {
validateResults(analysisResults, 0);
});
test('Properties6', () => {
// Analyze with reportPropertyTypeMismatch enabled.
const analysisResult1 = TestUtils.typeAnalyzeSampleFiles(['properties6.py']);
validateResults(analysisResult1, 2);
// Analyze with reportPropertyTypeMismatch disabled.
const configOptions = new ConfigOptions('.');
configOptions.diagnosticRuleSet.reportPropertyTypeMismatch = 'none';
const analysisResult2 = TestUtils.typeAnalyzeSampleFiles(['properties6.py'], configOptions);
validateResults(analysisResult2, 0);
});
test('Operators1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['operators1.py']);

View File

@ -0,0 +1,36 @@
# This sample tests the reportPropertyTypeMismatch diagnostic rule.
from typing import Generic, List, Optional, TypeVar
_T = TypeVar("_T")
class ClassA(Generic[_T]):
@property
def prop_1(self) -> Optional[float]:
return 2
@prop_1.setter
def prop_1(self, value: int) -> None:
pass
@property
def prop_2(self) -> Optional[int]:
return 2
# This should generate an error because a float
# is not assignable to an Optional[int].
@prop_2.setter
def prop_2(self, value: float) -> None:
pass
@property
def prop_3(self) -> List[_T]:
return []
# This should generate an error because _T is
# not assignable to List[_T].
@prop_3.setter
def prop_3(self, value: _T) -> None:
pass

View File

@ -116,6 +116,17 @@
"error"
]
},
"reportPropertyTypeMismatch": {
"type": "string",
"description": "Diagnostics for property whose setter and getter have mismatched types.",
"default": "error",
"enum": [
"none",
"information",
"warning",
"error"
]
},
"reportMissingImports": {
"type": "string",
"description": "Diagnostics for imports that have no corresponding imported python file or type stub file.",

View File

@ -129,6 +129,12 @@
"title": "Controls reporting of general type issues",
"default": "error"
},
"reportPropertyTypeMismatch": {
"$id": "#/properties/reportPropertyTypeMismatch",
"$ref": "#/definitions/diagnostic",
"title": "Controls reporting of property getter/setter type mismatches",
"default": "error"
},
"reportMissingImports": {
"$id": "#/properties/reportMissingImports",
"$ref": "#/definitions/diagnostic",