[internal] Refactor engine_aware.rs to acquire the GIL fewer times

[Eric-Arellano]

The PyO3 maintainers recommended that our low-level helpers request Python (a lock for the GIL) as an argument, rather than acquiring it directly: #12451 (comment). This is because acquiring the GIL has a non-trivial overhead, and it allows callers to batch the operation. Indeed, this PR has two map operations where we now only acquire the GIL once, rather than n times.

This also simplifies our handling of TypeErrors for less verbose code. As explained in #13515 (comment), MyPy already will handle these issues and they're also not mission-critical. Not worth the boilerplate to duplicate MyPy.

[Eric-Arellano]

FYI my main motivation is refactoring our rust-cpython code so that it's easier to port to PyO3.

[Eric-Arellano]

PyO3 has this as a method on PyAny, so we'll be able to inline this soon!

[Eric-Arellano]

Apparently, we can't panic in engine_aware.rs like I was originally trying to do.

So then, I was trying to improve this code so that we safely handle errors but log them. It was really boiler-platey, and I only got halfway:

diff --git a/src/rust/engine/src/externs/engine_aware.rs b/src/rust/engine/src/externs/engine_aware.rs
index 22dc4c2e6..33a1ba0e0 100644
--- a/src/rust/engine/src/externs/engine_aware.rs
+++ b/src/rust/engine/src/externs/engine_aware.rs
@@ -4,7 +4,7 @@
 use crate::context::Context;
 use crate::externs;
 use crate::nodes::{lift_directory_digest, lift_file_digest};
-use crate::python::{TypeId, Value};
+use crate::python::{Failure, TypeId, Value};
 use crate::Types;
 
 use cpython::{ObjectProtocol, PyDict, PyString, Python};
@@ -43,19 +43,39 @@ impl EngineAwareReturnType {
   }
 
   fn level(py: Python, value: &Value) -> Option<Level> {
-    let level_val = externs::call_method0(py, value, "level").unwrap();
-    if level_val.is_none(py) {
-      return None;
-    }
-    Some(externs::val_to_log_level(&level_val).unwrap())
+    externs::call_method0(py, value, "level")
+      .and_then(|res| {
+        if res.is_none(py) {
+          Ok(None)
+        } else {
+          Ok(externs::val_to_log_level(&res).ok())
+        }
+      })
+      .unwrap_or_else(|err| {
+        log::error!(
+          "Error in EngineAwareReturnType.level(): {}",
+          Failure::from_py_err_with_gil(py, err)
+        );
+        None
+      })
   }
 
   fn message(py: Python, value: &Value) -> Option<String> {
-    let msg_val = externs::call_method0(py, value, "message").unwrap();
-    if msg_val.is_none(py) {
-      return None;
-    }
-    msg_val.extract(py).unwrap()
+    externs::call_method0(py, value, "message")
+      .and_then(|res| {
+        if res.is_none(py) {
+          Ok(None)
+        } else {
+          res.extract(py)
+        }
+      })
+      .unwrap_or_else(|err| {
+        log::error!(
+          "Error in EngineAwareReturnType.message(): {}",
+          Failure::from_py_err_with_gil(py, err)
+        );
+        None
+      })
   }
 
   fn artifacts(py: Python, types: &Types, value: &Value) -> Vec<(String, ArtifactOutput)> {
@@ -88,9 +108,14 @@ impl EngineAwareReturnType {
 
   pub(crate) fn is_cacheable(py: Python, value: &Value) -> bool {
     externs::call_method0(py, value, "cacheable")
-      .unwrap()
-      .extract(py)
-      .unwrap()
+      .and_then(|res| res.extract(py))
+      .unwrap_or_else(|err| {
+        log::error!(
+          "Error in EngineAwareReturnType.cacheable(): {}",
+          Failure::from_py_err_with_gil(py, err)
+        );
+        true
+      })
   }
 }
 
@@ -98,11 +123,21 @@ pub struct EngineAwareParameter;
 
 impl EngineAwareParameter {
   pub fn debug_hint(py: Python, value: &Value) -> Option<String> {
-    let hint = externs::call_method0(py, value, "debug_hint").ok()?;
-    if hint.is_none(py) {
-      return None;
-    }
-    hint.extract(py).ok()
+    externs::call_method0(py, value, "debug_hint")
+      .and_then(|res| {
+        if res.is_none(py) {
+          Ok(None)
+        } else {
+          res.extract(py)
+        }
+      })
+      .unwrap_or_else(|err| {
+        log::error!(
+          "Error in EngineAwareReturnType.debug_hint(): {}",
+          Failure::from_py_err_with_gil(py, err)
+        );
+        None
+      })
   }
 
   pub fn metadata(py: Python, context: &Context, value: &Value) -> Vec<(String, UserMetadataItem)> {

And then it dawned on me, MyPy will catch all these issues that we were trying to log. They're all TypeError! Not worth the complexity IMO for our Rust code to duplicate MyPy, especially given that these errors are not mission critical.