From 9e12d3e944e809818ef12707e2d12bacdc3eb483 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 24 Jun 2019 11:50:35 -0600 Subject: [PATCH] Updated implementation of reportPrivateUsage check to differentiate between protected class members (single underscore) and private class members (double underscore). --- client/schemas/pyrightconfig.schema.json | 2 +- docs/configuration.md | 2 +- server/src/analyzer/symbolUtils.ts | 9 ++- server/src/analyzer/typeAnalyzer.ts | 94 ++++++++++++++++++------ server/src/tests/samples/private1.py | 8 +- server/src/tests/samples/private2.py | 4 +- server/src/tests/typeAnalyzer.test.ts | 2 +- 7 files changed, 93 insertions(+), 28 deletions(-) diff --git a/client/schemas/pyrightconfig.schema.json b/client/schemas/pyrightconfig.schema.json index 173bc86b9..faac23561 100644 --- a/client/schemas/pyrightconfig.schema.json +++ b/client/schemas/pyrightconfig.schema.json @@ -195,7 +195,7 @@ "reportPrivateUsage": { "$id": "#/properties/reportPrivateUsage", "$ref": "#/definitions/diagnostic", - "title": "Controls reporting of private variables and functions used outside of the owning class or module", + "title": "Controls reporting of private variables and functions used outside of the owning class or module and usage of protected members outside of subclasses", "default": "none" }, "reportConstantRedefinition": { diff --git a/docs/configuration.md b/docs/configuration.md index 70c6c6575..e9a7be1a5 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -70,7 +70,7 @@ The following settings control pyright's diagnostic output (warnings or errors). **reportUntypedNamedTuple** [boolean or string, optional]: Generate or suppress diagnostics when “namedtuple” is used rather than “NamedTuple”. The former contains no type information, whereas the latter does. The default value for this setting is 'none'. -**reportPrivateUsage** [boolean or string, optional]: Generate or suppress diagnostics for uses of private variables or functions outside of the class or module that declares them. Private variables and functions, by convention, are named starting with a single underscoe (“_”) character. The default value for this setting is 'none'. +**reportPrivateUsage** [boolean or string, optional]: Generate or suppress diagnostics for incorrect usage of private or protected variables or functions. Protected class members begin with a single underscore (“_”) and can be accessed only by subclasses. Private class members begin with a double underscore but do not end in a double underscore and can be accessed only within the declaring class. Variables and functions declared outside of a class are considered private if their names start with either a single or double underscore, and they cannot be accessed outside of the declaring module. The default value for this setting is 'none'. **reportConstantRedefinition** [boolean or string, optional]: Generate or suppress diagnostics for attempts to redefine variables whose names are all-caps with underscores and numerals. The default value for this setting is 'none'. diff --git a/server/src/analyzer/symbolUtils.ts b/server/src/analyzer/symbolUtils.ts index 53fa8c0df..651d82f43 100644 --- a/server/src/analyzer/symbolUtils.ts +++ b/server/src/analyzer/symbolUtils.ts @@ -11,8 +11,15 @@ const _constantRegEx = /^[A-Z0-9_]+$/; const _underscoreOnlyRegEx = /^[_]+$/; export class SymbolUtils { - // Private symbol names start with a single underscore. + // Private symbol names start with a double underscore. static isPrivateName(name: string) { + return name.length > 2 && + name.startsWith('__') && + !name.endsWith('__'); + } + + // Protected symbol names start with a single underscore. + static isProtectedName(name: string) { return name.length > 1 && name.startsWith('_') && !name.startsWith('__'); diff --git a/server/src/analyzer/typeAnalyzer.ts b/server/src/analyzer/typeAnalyzer.ts index 9b43d5515..08952cf95 100644 --- a/server/src/analyzer/typeAnalyzer.ts +++ b/server/src/analyzer/typeAnalyzer.ts @@ -36,7 +36,7 @@ import { ImportResult, ImportType } from './importResult'; import { DefaultTypeSourceId, TypeSourceId } from './inferredType'; import { ParseTreeUtils } from './parseTreeUtils'; import { ParseTreeWalker } from './parseTreeWalker'; -import { Scope, ScopeType } from './scope'; +import { Scope, ScopeType, SymbolWithScope } from './scope'; import { Declaration, Symbol, SymbolCategory, SymbolTable } from './symbol'; import { SymbolUtils } from './symbolUtils'; import { TypeConstraintBuilder } from './typeConstraint'; @@ -1692,6 +1692,21 @@ export class TypeAnalyzer extends ParseTreeWalker { symbol.addDeclaration(declaration); } + private _isSymbolPrivate(nameValue: string, scopeType: ScopeType) { + // See if the symbol is private. + if (SymbolUtils.isPrivateName(nameValue)) { + return true; + } + + if (SymbolUtils.isProtectedName(nameValue)) { + // Protected names outside of a class scope are considered private. + const isClassScope = scopeType === ScopeType.Class; + return !isClassScope; + } + + return false; + } + private _conditionallyReportUnusedName(node: NameNode, reportPrivateOnly: boolean, diagLevel: DiagnosticLevel, message: string) { @@ -1703,10 +1718,6 @@ export class TypeAnalyzer extends ParseTreeWalker { return; } - if (reportPrivateOnly && !SymbolUtils.isPrivateName(nameValue)) { - return; - } - if (SymbolUtils.isDunderName(nameValue)) { return; } @@ -1717,22 +1728,19 @@ export class TypeAnalyzer extends ParseTreeWalker { const symbolInScope = this._currentScope.lookUpSymbolRecursive(nameValue); if (symbolInScope && !symbolInScope.symbol.isAccessed()) { - this._addUnusedName(node); + if (reportPrivateOnly) { + if (!this._isSymbolPrivate(nameValue, symbolInScope.scope.getType())) { + return; + } + } + this._addUnusedName(node); this._addDiagnostic(diagLevel, message, node); } } private _conditionallyReportPrivateUsage(node: NameNode) { if (this._fileInfo.diagnosticSettings.reportPrivateUsage === 'none') { - - return; - } - - const nameValue = node.nameToken.value; - - // Is it a private name? - if (!SymbolUtils.isPrivateName(nameValue)) { return; } @@ -1741,6 +1749,16 @@ export class TypeAnalyzer extends ParseTreeWalker { return; } + const nameValue = node.nameToken.value; + const isPrivateName = SymbolUtils.isPrivateName(nameValue); + const isProtectedName = SymbolUtils.isProtectedName(nameValue); + + // If it's not a protected or private name, don't bother with + // any further checks. + if (!isPrivateName && !isProtectedName) { + return; + } + const declarations = AnalyzerNodeInfo.getDeclarations(node); const primaryDeclaration = declarations && declarations.length > 0 ? declarations[0] : undefined; @@ -1752,7 +1770,7 @@ export class TypeAnalyzer extends ParseTreeWalker { primaryDeclaration.node); // If this is the name of a class, find the module or class that contains it rather - // than using constraining the use of the class name within the class itself. + // than constraining the use of the class name within the class itself. if (primaryDeclaration.node.parent && primaryDeclaration.node.parent === classOrModuleNode && classOrModuleNode instanceof ClassNode) { @@ -1760,13 +1778,45 @@ export class TypeAnalyzer extends ParseTreeWalker { classOrModuleNode = ParseTreeUtils.getEnclosingClassOrModule(classOrModuleNode); } - if (classOrModuleNode && !ParseTreeUtils.isNodeContainedWithin(node, classOrModuleNode)) { - const scopeName = classOrModuleNode instanceof ClassNode ? - 'class' : 'module'; + // If it's a class member, check whether it's a legal protected access. + let isProtectedAccess = false; + if (classOrModuleNode instanceof ClassNode) { + if (isProtectedName) { + const declarationClassType = AnalyzerNodeInfo.getExpressionType(classOrModuleNode); + if (declarationClassType && declarationClassType instanceof ClassType) { + // Note that the access is to a protected class member. + isProtectedAccess = true; - this._addDiagnostic(this._fileInfo.diagnosticSettings.reportPrivateUsage, - `'${ nameValue }' is private and used outside of its owning ${ scopeName }`, - node); + const enclosingClassNode = ParseTreeUtils.getEnclosingClass(node); + if (enclosingClassNode) { + isProtectedAccess = true; + const enclosingClassType = AnalyzerNodeInfo.getExpressionType(enclosingClassNode); + + // If the referencing class is a subclass of the declaring class, it's + // allowed to access a protected name. + if (enclosingClassType && enclosingClassType instanceof ClassType) { + if (TypeUtils.derivesFromClassRecursive(enclosingClassType, declarationClassType)) { + return; + } + } + } + } + } + } + + if (classOrModuleNode && !ParseTreeUtils.isNodeContainedWithin(node, classOrModuleNode)) { + if (isProtectedAccess) { + this._addDiagnostic(this._fileInfo.diagnosticSettings.reportPrivateUsage, + `'${ nameValue }' is protected and used outside of a derived class`, + node); + } else { + const scopeName = classOrModuleNode instanceof ClassNode ? + 'class' : 'module'; + + this._addDiagnostic(this._fileInfo.diagnosticSettings.reportPrivateUsage, + `'${ nameValue }' is private and used outside of the ${ scopeName } in which it is declared`, + node); + } } } @@ -2914,7 +2964,7 @@ export class TypeAnalyzer extends ParseTreeWalker { // "not access" unless the name indicates that it's private. const scopeType = permanentScope.getType(); if (scopeType === ScopeType.Class || scopeType === ScopeType.Module) { - if (!SymbolUtils.isPrivateName(name)) { + if (!this._isSymbolPrivate(name, scopeType)) { symbol.setIsAcccessed(); } } diff --git a/server/src/tests/samples/private1.py b/server/src/tests/samples/private1.py index 8cdf86446..a7d1ded44 100644 --- a/server/src/tests/samples/private1.py +++ b/server/src/tests/samples/private1.py @@ -20,7 +20,7 @@ a = _TestClass() b = TestClass() # This should generate an error -c = b._priv1 +c = b.__priv1 d = Foo() @@ -31,4 +31,10 @@ e = d._my_var1 f = _Test +class TestSubclass(TestClass): + def blah(self): + return self._prot1 + def blah2(self): + # This should generate an error + return self.__priv1 diff --git a/server/src/tests/samples/private2.py b/server/src/tests/samples/private2.py index 50b6a1dd3..7cf3257a6 100644 --- a/server/src/tests/samples/private2.py +++ b/server/src/tests/samples/private2.py @@ -5,5 +5,7 @@ class _TestClass(object): class TestClass(object): def __init__(self): - self._priv1 = 1 + self.__priv1 = 1 + self._prot1 = 1 + \ No newline at end of file diff --git a/server/src/tests/typeAnalyzer.test.ts b/server/src/tests/typeAnalyzer.test.ts index 19ae0583c..9766d1fdd 100644 --- a/server/src/tests/typeAnalyzer.test.ts +++ b/server/src/tests/typeAnalyzer.test.ts @@ -273,7 +273,7 @@ test('Private1', () => { // Turn on errors. configOptions.diagnosticSettings.reportPrivateUsage = 'error'; analysisResults = TestUtils.typeAnalyzeSampleFiles(['private1.py'], configOptions); - validateResults(analysisResults, 3); + validateResults(analysisResults, 4); }); test('Constant1', () => {