diff --git a/crates/pyrefly_config/src/error_kind.rs b/crates/pyrefly_config/src/error_kind.rs index 9f48799a62..bcfa01379b 100644 --- a/crates/pyrefly_config/src/error_kind.rs +++ b/crates/pyrefly_config/src/error_kind.rs @@ -384,6 +384,8 @@ pub enum ErrorKind { /// guaranteed. This is a separate error code from BadKeywordArgument to allow /// users to opt-in to this stricter check. PotentialBadKeywordArgument, + /// Accessing a protected class member from outside its defining class or a subclass. + ProtectedAccess, /// A protocol attribute was first defined inside a method instead of the class body. ProtocolImplicitlyDefinedAttribute, /// Calling `.cuda()` on a `torch.Tensor` hard-codes the target device. @@ -657,6 +659,7 @@ impl ErrorKind { ErrorKind::OrderedDataclassInheritance => Severity::Warn, ErrorKind::PossiblyUninitializedAttribute => Severity::Ignore, ErrorKind::PotentialBadKeywordArgument => Severity::Ignore, + ErrorKind::ProtectedAccess => Severity::Ignore, ErrorKind::PytorchEfficiencyLintCudaCall => Severity::Ignore, ErrorKind::PytorchEfficiencyLintItemCall => Severity::Ignore, ErrorKind::PytorchEfficiencyLintPrintTensor => Severity::Ignore, diff --git a/crates/pyrefly_config/src/migration/pyright.rs b/crates/pyrefly_config/src/migration/pyright.rs index eb882ede5a..10f9eb46c5 100644 --- a/crates/pyrefly_config/src/migration/pyright.rs +++ b/crates/pyrefly_config/src/migration/pyright.rs @@ -423,7 +423,7 @@ impl RuleOverrides { add(self.report_operator_issue, ErrorKind::NotCallable); add(self.report_return_type, ErrorKind::BadReturn); add(self.report_return_type, ErrorKind::InvalidYield); - add(self.report_private_usage, ErrorKind::NoAccess); + add(self.report_private_usage, ErrorKind::ProtectedAccess); add(self.report_deprecated, ErrorKind::Deprecated); add( self.report_incompatible_method_override, @@ -876,4 +876,14 @@ include = ["basedpyright.py"] assert!(!config.project_includes.is_empty()); Ok(()) } + + #[test] + fn test_report_private_usage_mapping() -> anyhow::Result<()> { + let pyr = serde_json::from_str::(r#"{"reportPrivateUsage": "warning"}"#)?; + let config = pyr.convert(); + let errors = config.root.errors.expect("expected a diagnostic override"); + assert_eq!(errors.severity(ErrorKind::ProtectedAccess), Severity::Warn); + assert_eq!(errors.severity(ErrorKind::NoAccess), Severity::Error); + Ok(()) + } } diff --git a/crates/pyrefly_python/src/ast.rs b/crates/pyrefly_python/src/ast.rs index 6257c3d9d6..eaf38ce336 100644 --- a/crates/pyrefly_python/src/ast.rs +++ b/crates/pyrefly_python/src/ast.rs @@ -595,6 +595,10 @@ impl Ast { name.starts_with("__") && !name.ends_with("__") } + pub fn is_protected_attr(name: &Name) -> bool { + name.len() > 1 && name.starts_with('_') && !name.starts_with("__") + } + // Parameters and variables that are prefixed (but not suffixed) with a single underscore // are potentially unused, so we should skip some diagnostics/errors. // Examples: `_`, `_x` diff --git a/pyrefly/lib/alt/solve.rs b/pyrefly/lib/alt/solve.rs index 32a82afc6f..bd8af907ec 100644 --- a/pyrefly/lib/alt/solve.rs +++ b/pyrefly/lib/alt/solve.rs @@ -3394,6 +3394,71 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } } + fn check_protected_attribute_access( + &self, + expect: &PrivateAttributeAccessCheck, + value_type: &Type, + errors: &ErrorCollector, + ) { + let Some(owners) = expect + .class_idxs + .iter() + .map(|class_idx| self.get_idx(*class_idx).0.clone()) + .collect::>>() + else { + return; + }; + let inaccessible = value_type.clone().into_unions().into_iter().any(|ty| { + let accessed_class = match ty { + Type::ClassDef(cls) => Some(cls), + Type::ClassType(cls) | Type::SelfType(cls) => Some(cls.into_class_object()), + Type::Type(inner) => match *inner { + Type::ClassType(cls) | Type::SelfType(cls) => Some(cls.into_class_object()), + _ => None, + }, + _ => None, + }; + let Some(accessed_class) = accessed_class else { + return false; + }; + if matches!( + expect.attr.id.as_str(), + "_asdict" | "_field_defaults" | "_fields" | "_make" | "_replace" + ) && self + .get_metadata_for_class(&accessed_class) + .named_tuple_metadata() + .is_some() + { + return false; + } + let Some(member) = + self.get_class_member_with_defining_class(&accessed_class, &expect.attr.id) + else { + return false; + }; + !member.defining_class.module_path().is_interface() + && !owners.iter().any(|owner| { + member.defining_class == *owner + || self + .get_mro_for_class(owner) + .ancestors_no_object() + .iter() + .any(|ancestor| ancestor.class_object() == &member.defining_class) + }) + }); + if inaccessible && self.has_static_attr(value_type, &expect.attr.id) { + self.error( + errors, + expect.attr.range(), + ErrorKind::ProtectedAccess, + format!( + "Protected attribute `{}` cannot be accessed outside of its defining class or a subclass", + expect.attr.id + ), + ); + } + } + fn check_private_attribute_access( &self, expect: &PrivateAttributeAccessCheck, @@ -3407,28 +3472,32 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { ) { return; } - if let Some(class_idx) = expect.class_idx { - let class_binding = self.get_idx(class_idx); - let Some(owner) = class_binding.0.as_ref() else { - return; - }; - if self + if Ast::is_protected_attr(&expect.attr.id) { + self.check_protected_attribute_access(expect, &value_type, errors); + return; + } + let owner = expect + .class_idxs + .first() + .and_then(|class_idx| self.get_idx(*class_idx).0.clone()); + if !expect.class_idxs.is_empty() && owner.is_none() { + return; + } + if let Some(owner) = owner.as_ref() + && self .get_class_fields(owner) .is_some_and(|f| f.contains(&expect.attr.id)) - && self.is_subset_eq( - &value_type, - &self.union( - self.heap.mk_class_def(owner.dupe()), - self.instantiate(owner), - ), - ) - { - return; // Valid private attribute access - } + && self.is_subset_eq( + &value_type, + &self.union( + self.heap.mk_class_def(owner.dupe()), + self.instantiate(owner), + ), + ) + { + return; } - // No defining class to restrict access to — the attribute is only - // reachable via dynamic fallback (__getattr__/__getattribute__), so - // name mangling is irrelevant. + // Dynamic fallback does not establish a class declaration to restrict access to. if !self.has_static_attr(&value_type, &expect.attr.id) { return; } diff --git a/pyrefly/lib/binding/binding.rs b/pyrefly/lib/binding/binding.rs index fe4e29bbce..1b7c4f44cf 100644 --- a/pyrefly/lib/binding/binding.rs +++ b/pyrefly/lib/binding/binding.rs @@ -128,7 +128,7 @@ assert_words!(KeyDecoratedFunction, 1); assert_words!(KeyUndecoratedFunction, 1); assert_words!(Binding, 4); -assert_words!(BindingExpect, 12); +assert_words!(BindingExpect, 8); assert_words!(BindingTypeAlias, 6); assert_words!(BindingAnnotation, 11); assert_words!(BindingClass, 11); @@ -1215,7 +1215,7 @@ pub enum ExprOrBinding { pub struct PrivateAttributeAccessCheck { pub value: Expr, pub attr: Identifier, - pub class_idx: Option>, + pub class_idxs: Vec>, } #[derive(Clone, Debug)] @@ -1345,7 +1345,7 @@ pub enum BindingExpect { range: TextRange, }, /// Track private attribute accesses that need semantic validation. - PrivateAttributeAccess(PrivateAttributeAccessCheck), + PrivateAttributeAccess(Box), /// Deferred check for uninitialized variables. This is a "dangling" binding /// that doesn't affect any other types - it only exists to emit an error at /// solve time if any of the termination keys don't have Never type. @@ -1456,11 +1456,10 @@ impl DisplayWith for BindingExpect { "PrivateAttributeAccess({}, {}, {})", m.display(&expectation.value), expectation.attr.id, - if let Some(class_idx) = expectation.class_idx { - format!("{}", ctx.display(class_idx)) - } else { - "None".to_owned() - } + intersperse_iter(", ", || expectation + .class_idxs + .iter() + .map(|idx| ctx.display(*idx))) ), Self::MatchExhaustiveness { subject_idx, diff --git a/pyrefly/lib/binding/expr.rs b/pyrefly/lib/binding/expr.rs index 65e04242ea..1519b48396 100644 --- a/pyrefly/lib/binding/expr.rs +++ b/pyrefly/lib/binding/expr.rs @@ -783,8 +783,8 @@ impl<'a> BindingsBuilder<'a> { match x { Expr::Attribute(attr) => { - self.check_private_attribute_usage(attr); self.ensure_expr(&mut attr.value, usage); + self.check_private_attribute_usage(attr); } Expr::Subscript(ExprSubscript { value, slice, .. }) => { // Some subscripts are (or contain) type expressions even when they appear in a @@ -1255,17 +1255,26 @@ impl<'a> BindingsBuilder<'a> { } fn check_private_attribute_usage(&mut self, attr: &ExprAttribute) { - if !Ast::is_mangled_attr(&attr.attr.id) { + if !Ast::is_mangled_attr(&attr.attr.id) && !Ast::is_protected_attr(&attr.attr.id) { return; } + let class_idxs = if Ast::is_protected_attr(&attr.attr.id) { + self.scopes.enclosing_class_keys() + } else { + self.scopes + .current_class_key() + .or_else(|| self.scopes.current_method_context()) + .into_iter() + .collect() + }; let expect = PrivateAttributeAccessCheck { value: (*attr.value).clone(), attr: attr.attr.clone(), - class_idx: self.scopes.current_method_context(), + class_idxs, }; self.insert_binding( KeyExpect::PrivateAttributeAccess(attr.attr.range()), - BindingExpect::PrivateAttributeAccess(expect), + BindingExpect::PrivateAttributeAccess(Box::new(expect)), ); } diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index bbbb437a3e..3ebcace54c 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -1931,6 +1931,11 @@ impl Scopes { self.current().class_key() } + /// Return enclosing classes from innermost to outermost. + pub fn enclosing_class_keys(&self) -> Vec> { + self.iter_rev().filter_map(Scope::class_key).collect() + } + /// Are we anywhere inside a class? If so, return the class object idx. /// This function looks at enclosing scopes. pub fn enclosing_class_object_idx(&self) -> Option> { diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index 609db4f5c9..8ab73d20a4 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -2444,6 +2444,54 @@ class C: __secret: int = 0 def reveal(self, a: A): return a.__secret # E: Private attribute `__secret` cannot be accessed outside of its defining class + +class Outer: + __secret: int = 0 + + class Inner: + def reveal(self, outer: "Outer") -> int: + return outer.__secret # E: Private attribute `__secret` cannot be accessed outside of its defining class +"#, +); + +testcase!( + test_protected_attribute_access, + TestEnv::new().enable_protected_access_error(), + r#" +class A: + _protected: int = 0 + + def reveal(self, other: "A") -> int: + return self._protected + other._protected + +outside_instance = A()._protected # E: Protected attribute `_protected` cannot be accessed outside of its defining class or a subclass +outside_class = A._protected # E: Protected attribute `_protected` cannot be accessed outside of its defining class or a subclass + +class B(A): + inherited = A._protected + + def reveal(self, other: A) -> int: + return self._protected + other._protected + +class Unrelated: + def leak(self, a: A) -> int: + return a._protected # E: Protected attribute `_protected` cannot be accessed outside of its defining class or a subclass +"#, +); + +testcase!( + test_same_named_protected_attribute_from_unrelated_class, + TestEnv::new().enable_protected_access_error(), + r#" +class A: + _protected: int = 0 + +class B(A): + def leak(self, other: "Unrelated") -> int: + return other._protected # E: Protected attribute `_protected` cannot be accessed outside of its defining class or a subclass + +class Unrelated: + _protected: int = 0 "#, ); @@ -4357,6 +4405,210 @@ def use(obj, a: Any, c: C) -> None: "#, ); +testcase!( + test_protected_attribute_ignored_by_default, + r#" +class A: + _protected: int = 0 + +value = A()._protected +"#, +); + +testcase!( + test_protected_attribute_dynamic_values, + TestEnv::new().enable_protected_access_error(), + r#" +from typing import Any + +class Dynamic: + def __getattr__(self, name: str) -> Any: ... + +def outside(value: Any, cls: type[Any], dynamic: Dynamic): + value._protected + cls._protected + dynamic._protected + +class A: + _protected: int = 0 + + def inside(self, value: Any, cls: type[Any], dynamic: Dynamic): + value._protected + cls._protected + dynamic._protected +"#, +); + +testcase!( + test_protected_attribute_from_stub, + { + let mut env = TestEnv::new().enable_protected_access_error(); + env.add("source", "class SourceBase:\n _protected: int = 0\n"); + env.add_with_path( + "lib", + "lib.pyi", + "from source import SourceBase\nclass A:\n _protected: int\n __private: int\nclass StubDerived(SourceBase): ...\n", + ); + env + }, + r#" +from lib import A, StubDerived + +value = A()._protected +A()._protected = 1 +A().__private # E: Private attribute `__private` cannot be accessed outside of its defining class +StubDerived()._protected # E: Protected attribute `_protected` cannot be accessed outside of its defining class or a subclass + +class B: + def method(self, value: A): + return value._protected + +class Derived(A): + pass + +inherited = Derived()._protected + +class Override(A): + _protected: int = 1 + +Override()._protected # E: Protected attribute `_protected` cannot be accessed outside of its defining class or a subclass + +class Source: + _protected: int = 1 + +def read(value: A | Source) -> int: + return value._protected # E: Protected attribute `_protected` cannot be accessed outside of its defining class or a subclass +"#, +); + +testcase!( + test_protected_attribute_on_bounded_type_variable, + TestEnv::new().enable_protected_access_error(), + r#" +class A: + _protected: int = 0 + + def read[T: A](self, other: T) -> int: + return other._protected +"#, +); + +testcase!( + test_protected_attribute_quoted_cast_default, + r#" +from typing import Any, assert_type, cast + +class Owner: + _value: int = 1 + +def read(value: Any) -> None: + assert_type(cast("Owner", value)._value, int) + assert_type(cast(Owner, value)._value, int) +"#, +); + +testcase!( + test_protected_attribute_quoted_cast_enabled, + TestEnv::new().enable_protected_access_error(), + r#" +from typing import Any, cast + +class Owner: + _value: int = 1 + +def read(value: Any) -> None: + cast("Owner", value)._value # E: Protected attribute `_value` cannot be accessed outside of its defining class or a subclass + cast(Owner, value)._value # E: Protected attribute `_value` cannot be accessed outside of its defining class or a subclass +"#, +); + +testcase!( + test_protected_attribute_named_tuple_public_api, + TestEnv::new().enable_protected_access_error(), + r#" +from collections import namedtuple +from typing import NamedTuple +from urllib.parse import urlsplit + +class Record(NamedTuple): + value: int + + def _internal(self) -> int: + return self.value + +record = Record(1) +record._replace(value=2) +record._asdict() +Record._make([1]) +record._fields +Record._field_defaults +record._internal() # E: Protected attribute `_internal` cannot be accessed outside of its defining class or a subclass + +Row = namedtuple("Row", ["value"]) +Row(1)._replace(value=2) +Row(1)._asdict() +urlsplit("https://example.com/a")._replace(scheme="file") + +class Other: + def _replace(self) -> None: ... + +Other()._replace() # E: Protected attribute `_replace` cannot be accessed outside of its defining class or a subclass +"#, +); + +testcase!( + test_protected_attribute_enclosing_class_scopes, + TestEnv::new().enable_protected_access_error(), + r#" +from typing import assert_type + +class _CodeWriter: + _indent: int = 1 + + def indent(self): + class Indenter: + def __exit__(_, *args): + assert_type(self._indent, int) + return Indenter() + +class Base: + _value: int = 1 + +class HeaderBase: + def __init_subclass__(cls, flag: int) -> None: ... + +class InHeader(Base, HeaderBase, flag=Base._value): # E: Protected attribute `_value` cannot be accessed outside of its defining class or a subclass + pass + +class Derived(Base): + class Inner: + value = Base._value + + def read(self, other: Base) -> int: + def nested() -> int: + return other._value + return nested() + +class Container: + class First: + _value: int = 1 + + class Second: + def read(self, other: "Container.First") -> int: + return other._value # E: Protected attribute `_value` cannot be accessed outside of its defining class or a subclass + +class Unrelated: + class Inner: + def read(self, other: Base) -> int: + return other._value # E: Protected attribute `_value` cannot be accessed outside of its defining class or a subclass + +def outside(other: Base) -> int: + def nested() -> int: + return other._value # E: Protected attribute `_value` cannot be accessed outside of its defining class or a subclass + return nested() +"#, +); + // The exemption for conditional methods requires every binding of the name in the class body // to be a `def`, even when a later `def` replaces the other binding. testcase!( diff --git a/pyrefly/lib/test/util.rs b/pyrefly/lib/test/util.rs index 4a3663a35b..607cfeabd8 100644 --- a/pyrefly/lib/test/util.rs +++ b/pyrefly/lib/test/util.rs @@ -307,6 +307,7 @@ pub struct TestEnv { unknown_attribute_access_error: bool, implicit_reexport_error: bool, potential_bad_keyword_argument_error: bool, + protected_access_error: bool, default_require_level: Require, extra_file_extensions: Vec, replace_imports_with_any: Vec, @@ -382,6 +383,7 @@ impl TestEnv { unknown_attribute_access_error: false, implicit_reexport_error: false, potential_bad_keyword_argument_error: false, + protected_access_error: false, default_require_level: Require::Exports, extra_file_extensions: Vec::new(), replace_imports_with_any: Vec::new(), @@ -739,6 +741,11 @@ impl TestEnv { self } + pub fn enable_protected_access_error(mut self) -> Self { + self.protected_access_error = true; + self + } + pub fn enable_potential_bad_keyword_argument_error(mut self) -> Self { self.potential_bad_keyword_argument_error = true; self @@ -874,6 +881,9 @@ impl TestEnv { config.root.errors = Some(ErrorDisplayConfig::new(HashMap::new())); }; let errors = config.root.errors.as_mut().unwrap(); + if self.protected_access_error { + errors.set_error_severity(ErrorKind::ProtectedAccess, Severity::Error); + } if self.implicitly_defined_attribute_error { errors.set_error_severity(ErrorKind::ImplicitlyDefinedAttribute, Severity::Error); } diff --git a/scripts/error_presets.json b/scripts/error_presets.json index 96e04f2ea0..a5f564debc 100644 --- a/scripts/error_presets.json +++ b/scripts/error_presets.json @@ -113,6 +113,7 @@ "partially-unknown-type": ["all"], "possibly-uninitialized-attribute": ["strict", "all"], "potential-bad-keyword-argument": ["strict", "all"], + "protected-access": ["all"], "protocol-implicitly-defined-attribute": ["legacy", "default", "strict", "all"], "pytorch-efficiency-lint-cuda-call": ["all"], "pytorch-efficiency-lint-item-call": ["all"], diff --git a/website/docs/error-kinds.mdx b/website/docs/error-kinds.mdx index e0d3ce5806..fff3639fec 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -1918,6 +1918,25 @@ opts: Options = {} f(name="test", **opts) # E: Multiple values for argument `name` ``` +## protected-access + +Default severity: `ignore` + +The `protected-access` error is reported when a protected class member, whose name starts with a +single underscore, is accessed from outside its defining class or a subclass. + +```python +class Service: + def _reset(self) -> None: ... + +Service()._reset() # protected-access +``` + +Protected member access is allowed within the defining class and its subclasses, including nested +class and function scopes. Members defined in stub files and the public underscore-prefixed +NamedTuple APIs are exempt. Double-underscore private attributes are checked separately under +[`no-access`](#no-access). + ## protocol-implicitly-defined-attribute Protocols must declare the attributes they require directly in the class body. Assigning to a new `self` attribute inside a protocol method introduces a member that implementations of the protocol would never be required to provide. diff --git a/website/docs/migrate/pyright/diagnostics-reference.mdx b/website/docs/migrate/pyright/diagnostics-reference.mdx index d52caf55d9..8751072df1 100644 --- a/website/docs/migrate/pyright/diagnostics-reference.mdx +++ b/website/docs/migrate/pyright/diagnostics-reference.mdx @@ -92,7 +92,7 @@ several kinds. | `reportUntypedClassDecorator` | `untyped-class-decorator` | Automatic | Direct | | | `reportUntypedBaseClass` | `unsupported-dynamic-base`, `unknown-base-class` | Manual | Related | `unknown-base-class` skips bases from unresolved imports; `unsupported-dynamic-base` covers `type()` calls. | | `reportUntypedNamedTuple` | `untyped-namedtuple` | Automatic | Direct | | -| `reportPrivateUsage` | `no-access` | Automatic | Related | The tools differ in what they consider protected or private usage. | +| `reportPrivateUsage` | `protected-access` | Automatic | Related | The tools differ in what they consider protected or private usage. | | `reportTypeCommentUsage` | No direct equivalent | Not mapped | None | Use a linter if the goal is to prohibit legacy type comments. | | `reportPrivateImportUsage` | `implicit-reexport` and/or `no-access` | Manual | Related | No one-to-one automatic mapping. | | `reportConstantRedefinition` | `treat-all-caps-as-final = true` plus `bad-assignment` | Manual | Related | This changes inference policy rather than enabling a single diagnostic kind. |