Split internal vs access not allowed errors

Summary:
I'm working on a refactor that will allow us to model attribute actions
in a descriptor-oriented way where get and set have separate type information.

One of the big issues getting in my way was that the `attr.rs` LookupResult type
was conflating two kinds of problems that don't belong together under the
`Error` variant:
- internal errors, which are actually just bugs in Pyre2 and occur when we
  completely fail to resolve an attribute
- errors caused by a lookup being on an illegal action (which are very similar
  to `NotFound`, except that they happen when we *did* find the attribute and
  we just don't want to allow the access, e.g. class object access on an
  instance-only member)

I really need these to be clearly distinguished because the first category of
error really does belong in `LookupResult` itself, but the second category
probably should live at a lower level attached to attribute *actions* (and
actually gets detected inside of `classes.rs`).

This diff makes the split.

Reviewed By: ndmitchell

Differential Revision: D68687295

fbshipit-source-id: 98ecbaf1936ec7b60afb020b3d3fbde41cbeee74

pyre2/pyre2/bin/alt/attr.rs

@@ -28,7 +28,9 @@ pub enum LookupResult {
     NotFound(NotFound),
     /// The lookup failed for some other reason. Fallback behavior does not
     /// make sense and callers should just bail.
-    Error(LookupError),
+    AccessNotAllowed(AccessNotAllowed),
+    /// There was a Pyre-internal error
+    InternalError(InternalError),
 }
 
 pub enum NotFound {
@@ -37,9 +39,12 @@ pub enum NotFound {
     ModuleExport(Module),
 }
 
-pub enum LookupError {
+pub enum InternalError {
     /// An internal error caused by `as_attribute_base` being partial.
     AttributeBaseUndefined(Type),
+}
+
+pub enum AccessNotAllowed {
     /// The attribute is only initialized on instances, but we saw an attempt
     /// to use it as a class attribute.
     ClassUseOfInstanceAttribute(Class),
@@ -59,7 +64,8 @@ impl LookupResult {
         match self {
             LookupResult::Found(ty) => Ok(ty),
             LookupResult::NotFound(err) => Err(err.to_error_msg(attr_name)),
-            LookupResult::Error(err) => Err(err.to_error_msg(attr_name, todo_ctx)),
+            LookupResult::AccessNotAllowed(err) => Err(err.to_error_msg(attr_name)),
+            LookupResult::InternalError(err) => Err(err.to_error_msg(attr_name, todo_ctx)),
         }
     }
 }
@@ -82,20 +88,16 @@ impl NotFound {
     }
 }
 
-impl LookupError {
-    pub fn to_error_msg(self, attr_name: &Name, todo_ctx: &str) -> String {
+impl AccessNotAllowed {
+    pub fn to_error_msg(self, attr_name: &Name) -> String {
         match self {
-            LookupError::AttributeBaseUndefined(ty) => format!(
-                "TODO: {todo_ctx} attribute base undefined for type: {}",
-                ty.deterministic_printing()
-            ),
-            LookupError::ClassUseOfInstanceAttribute(class) => {
+            AccessNotAllowed::ClassUseOfInstanceAttribute(class) => {
                 let class_name = class.name();
                 format!(
                     "Instance-only attribute `{attr_name}` of class `{class_name}` is not visible on the class"
                 )
             }
-            LookupError::ClassAttributeIsGeneric(class) => {
+            AccessNotAllowed::ClassAttributeIsGeneric(class) => {
                 let class_name = class.name();
                 format!(
                     "Generic attribute `{attr_name}` of class `{class_name}` is not visible on the class"
@@ -105,6 +107,18 @@ impl LookupError {
     }
 }
 
+impl InternalError {
+    pub fn to_error_msg(self, attr_name: &Name, todo_ctx: &str) -> String {
+        match self {
+            InternalError::AttributeBaseUndefined(ty) => format!(
+                "TODO: {todo_ctx} attribute base undefined for type: {} (trying to access {})",
+                ty.deterministic_printing(),
+                attr_name
+            ),
+        }
+    }
+}
+
 /// A normalized type representation which is convenient for attribute lookup,
 /// since many cases are collapsed. For example, Type::Literal is converted to
 /// it's corresponding class type.
@@ -146,12 +160,12 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                             None => LookupResult::NotFound(NotFound::ClassAttribute(class)),
                         }
                     }
-                    Err(NoClassAttribute::IsGenericMember) => {
-                        LookupResult::Error(LookupError::ClassAttributeIsGeneric(class))
-                    }
-                    Err(NoClassAttribute::InstanceOnlyAttribute) => {
-                        LookupResult::Error(LookupError::ClassUseOfInstanceAttribute(class))
-                    }
+                    Err(NoClassAttribute::IsGenericMember) => LookupResult::AccessNotAllowed(
+                        AccessNotAllowed::ClassAttributeIsGeneric(class),
+                    ),
+                    Err(NoClassAttribute::InstanceOnlyAttribute) => LookupResult::AccessNotAllowed(
+                        AccessNotAllowed::ClassUseOfInstanceAttribute(class),
+                    ),
                 }
             }
             Some(AttributeBase::Module(module)) => match self.get_module_attr(&module, attr_name) {
@@ -180,7 +194,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
             }
             Some(AttributeBase::Any(style)) => LookupResult::Found(style.propagate()),
             Some(AttributeBase::Never) => LookupResult::Found(Type::never()),
-            None => LookupResult::Error(LookupError::AttributeBaseUndefined(ty)),
+            None => LookupResult::InternalError(InternalError::AttributeBaseUndefined(ty)),
         }
     }
 

pyre2/pyre2/bin/alt/expr.rs

@@ -335,7 +335,10 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
             LookupResult::NotFound(_) => {
                 return None;
             }
-            LookupResult::Error(e) => {
+            LookupResult::AccessNotAllowed(e) => {
+                self.error_call_target(range, e.to_error_msg(method_name))
+            }
+            LookupResult::InternalError(e) => {
                 self.error_call_target(range, e.to_error_msg(method_name, "Expr::call_method"))
             }
             LookupResult::Found(attr) => {