diff --git a/crates/pyrefly_config/src/error_kind.rs b/crates/pyrefly_config/src/error_kind.rs index 2da8b201b8..36f21ad5f1 100644 --- a/crates/pyrefly_config/src/error_kind.rs +++ b/crates/pyrefly_config/src/error_kind.rs @@ -347,6 +347,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. + PrivateUsage, /// 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. diff --git a/crates/pyrefly_config/src/migration/pyright.rs b/crates/pyrefly_config/src/migration/pyright.rs index 9eeb992bcc..f16eb84024 100644 --- a/crates/pyrefly_config/src/migration/pyright.rs +++ b/crates/pyrefly_config/src/migration/pyright.rs @@ -399,7 +399,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::PrivateUsage); add(self.report_deprecated, ErrorKind::Deprecated); add( self.report_incompatible_method_override, @@ -739,4 +739,14 @@ executionEnvironments = [ 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::PrivateUsage), 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 29ba1eb7d1..f0519c0c94 100644 --- a/crates/pyrefly_python/src/ast.rs +++ b/crates/pyrefly_python/src/ast.rs @@ -538,6 +538,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 69946cb869..0070ebf3bb 100644 --- a/pyrefly/lib/alt/solve.rs +++ b/pyrefly/lib/alt/solve.rs @@ -2856,6 +2856,67 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { ) { return; } + if Ast::is_protected_attr(&expect.attr.id) { + if let Some(class_idx) = expect.class_idx { + let class_binding = self.get_idx(class_idx); + if let Some(owner) = class_binding.0.as_ref() { + let mut found_declaration = false; + let mut access_allowed = true; + for ty in value_type.clone().into_unions() { + 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 { + continue; + }; + let Some(member) = self + .get_class_member_with_defining_class(&accessed_class, &expect.attr.id) + else { + continue; + }; + found_declaration = true; + if member.defining_class.module_path().is_interface() { + continue; + } + if member.defining_class != *owner + && !self + .get_mro_for_class(owner) + .ancestors_no_object() + .iter() + .any(|ancestor| ancestor.class_object() == &member.defining_class) + { + access_allowed = false; + } + } + if found_declaration && access_allowed { + return; + } + } + } + if !self.has_static_attr(&value_type, &expect.attr.id) { + return; + } + self.error( + errors, + expect.attr.range(), + ErrorKind::PrivateUsage, + format!( + "Protected attribute `{}` cannot be accessed outside of its defining class or a subclass", + expect.attr.id + ), + ); + 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 { diff --git a/pyrefly/lib/binding/expr.rs b/pyrefly/lib/binding/expr.rs index 62ac38c619..ce7c7e0ab7 100644 --- a/pyrefly/lib/binding/expr.rs +++ b/pyrefly/lib/binding/expr.rs @@ -1183,13 +1183,16 @@ 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 expect = PrivateAttributeAccessCheck { value: (*attr.value).clone(), attr: attr.attr.clone(), - class_idx: self.scopes.current_method_context(), + class_idx: self + .scopes + .current_class_key() + .or_else(|| self.scopes.current_method_context()), }; self.insert_binding( KeyExpect::PrivateAttributeAccess(attr.attr.range()), diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index 6c19c98ca9..b33c7d826b 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -1470,6 +1470,45 @@ class C: "#, ); +testcase!( + test_protected_attribute_access, + 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, + 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 +"#, +); + testcase!( test_private_attribute_inside_class, r#" diff --git a/website/docs/error-kinds.mdx b/website/docs/error-kinds.mdx index 9b09e988e2..9ab1193ea4 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -1690,6 +1690,21 @@ opts: Options = {} f(name="test", **opts) # E: Multiple values for argument `name` ``` +## private-usage + +The `private-usage` 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() # private-usage +``` + +Protected member access is allowed within the defining class and its subclasses. 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/migrating-from-pyright.mdx b/website/docs/migrating-from-pyright.mdx index 8c00281b53..f892b8ef13 100644 --- a/website/docs/migrating-from-pyright.mdx +++ b/website/docs/migrating-from-pyright.mdx @@ -138,7 +138,7 @@ and pyrefly's [error kinds](error-kinds.mdx). | reportNoOverloadImplementation | invalid-overload | | reportOperatorIssue | unsupported-operation | | reportPossiblyUnboundVariable | unbound-name | -| reportPrivateUsage | no-access | +| reportPrivateUsage | private-usage | | reportReturnType | bad-return | | reportUnboundVariable | unbound-name | | reportUndefinedVariable | unknown-name |