Updated implementation of reportPrivateUsage check to differentiate b…

…etween protected class members (single underscore) and private class members (double underscore).

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": {

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'.
 

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('__');

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,27 +1728,34 @@ 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)) {
+        // Ignore privates in type stubs.
+        if (this._fileInfo.isStubFile) {
             return;
         }
 
-        // Ignore privates in type stubs.
-        if (this._fileInfo.isStubFile) {
+        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;
         }
 
@@ -1752,21 +1770,53 @@ 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) {
 
             classOrModuleNode = ParseTreeUtils.getEnclosingClassOrModule(classOrModuleNode);
         }
 
+        // 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;
+
+                    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)) {
-            const scopeName = classOrModuleNode instanceof ClassNode ?
-                'class' : 'module';
+            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 its owning ${ scopeName }`,
-                node);
+                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();
             }
         }

server/src/tests/samples/private1.py

@@ -20,7 +20,7 @@ def foo(self):
 b = TestClass()
 
 # This should generate an error
-c = b._priv1
+c = b.__priv1
 
 
 d = Foo()
@@ -31,4 +31,10 @@ def foo(self):
 f = _Test
 
 
+class TestSubclass(TestClass):
+    def blah(self):
+        return self._prot1
 
+    def blah2(self):
+        # This should generate an error
+        return self.__priv1

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
 
+

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', () => {