Implement proper From for starlark::Error

Summary: This can be done now that the default `std::error::Error` conversion has been removed. It will allow us to remove a bunch of conversion code and make Starlark<->buck2_error conversion easier for future development Reviewed By: JakobDegen Differential Revision: D67768978 fbshipit-source-id: adf1a7abab47dd7ad9d1b0893f5abc6b037cf8a3
facebook · Jan 6, 2025 · 3180103 · 3180103
1 parent 4b3e296
commit 3180103
Show file tree

Hide file tree

Showing 48 changed files with 89 additions and 197 deletions.
diff --git a/app/buck2_action_impl/src/actions/impls/run.rs b/app/buck2_action_impl/src/actions/impls/run.rs
@@ -43,7 +43,6 @@ use buck2_core::fs::artifact_path_resolver::ArtifactFs;
 use buck2_core::fs::buck_out_path::BuildArtifactPath;
 use buck2_core::fs::paths::forward_rel_path::ForwardRelativePathBuf;
 use buck2_error::conversion::from_any;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use buck2_events::dispatch::span_async_simple;
 use buck2_execute::artifact::fs::ExecutorFs;
@@ -403,7 +402,6 @@ impl RunAction {
     ) -> buck2_error::Result<Self> {
         let starlark_values = starlark_values
             .downcast_starlark()
-            .map_err(from_starlark)
             .internal_error("Must be `run_action_values`")?;
 
         Self::unpack(&starlark_values)?;

diff --git a/app/buck2_action_impl/src/actions/impls/write.rs b/app/buck2_action_impl/src/actions/impls/write.rs
@@ -29,7 +29,6 @@ use buck2_build_api::interpreter::rule_defs::cmd_args::value_as::ValueAsCommandL
 use buck2_build_api::interpreter::rule_defs::cmd_args::AbsCommandLineContext;
 use buck2_build_api::interpreter::rule_defs::cmd_args::DefaultCommandLineContext;
 use buck2_core::category::CategoryRef;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use buck2_execute::artifact::fs::ExecutorFs;
 use buck2_execute::execute::command_executor::ActionExecutionTimingData;
@@ -101,10 +100,7 @@ impl WriteAction {
             return Err(WriteActionValidationError::TooManyInputs.into());
         }
 
-        if ValueAsCommandLineLike::unpack_value(contents.value())
-            .map_err(from_starlark)?
-            .is_none()
-        {
+        if ValueAsCommandLineLike::unpack_value(contents.value())?.is_none() {
             return Err(WriteActionValidationError::ContentsNotCommandLineValue(
                 contents.value().to_repr(),
             )

diff --git a/app/buck2_action_impl/src/actions/impls/write_macros.rs b/app/buck2_action_impl/src/actions/impls/write_macros.rs
@@ -35,7 +35,6 @@ use buck2_core::fs::paths::RelativePathBuf;
 use buck2_core::fs::project_rel_path::ProjectRelativePathBuf;
 use buck2_error::conversion::from_any;
 use buck2_error::internal_error;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use buck2_execute::artifact::fs::ExecutorFs;
 use buck2_execute::execute::command_executor::ActionExecutionTimingData;
@@ -105,10 +104,7 @@ impl WriteMacrosToFileAction {
     ) -> buck2_error::Result<Self> {
         if outputs.is_empty() {
             Err(WriteMacrosActionValidationError::NoOutputsSpecified.into())
-        } else if ValueAsCommandLineLike::unpack_value(contents.value())
-            .map_err(from_starlark)?
-            .is_none()
-        {
+        } else if ValueAsCommandLineLike::unpack_value(contents.value())?.is_none() {
             Err(
                 WriteMacrosActionValidationError::ContentsNotCommandLineValue(
                     contents.value().to_repr(),

diff --git a/app/buck2_action_impl/src/dynamic/deferred.rs b/app/buck2_action_impl/src/dynamic/deferred.rs
@@ -35,7 +35,6 @@ use buck2_core::deferred::key::DeferredHolderKey;
 use buck2_core::fs::artifact_path_resolver::ArtifactFs;
 use buck2_error::buck2_error;
 use buck2_error::internal_error;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use buck2_events::dispatch::get_dispatcher;
 use buck2_events::dispatch::span_async;
@@ -127,9 +126,7 @@ pub fn invoke_dynamic_output_lambda<'v>(
             (&[], &named)
         }
     };
-    let return_value = eval
-        .eval_function(lambda, pos, named)
-        .map_err(from_starlark)?;
+    let return_value = eval.eval_function(lambda, pos, named)?;
 
     let provider_collection = match args {
         DynamicLambdaArgs::OldPositional { .. } => {
@@ -538,7 +535,7 @@ fn new_attr_value<'v>(
             let mut r = SmallMap::with_capacity(xs.len());
             for (k, v) in xs {
                 let prev = r.insert_hashed(
-                    k.to_value().get_hashed().map_err(from_starlark)?,
+                    k.to_value().get_hashed()?,
                     new_attr_value(
                         v,
                         _input_artifacts_materialized,

diff --git a/app/buck2_action_impl/src/dynamic/dynamic_actions_globals.rs b/app/buck2_action_impl/src/dynamic/dynamic_actions_globals.rs
@@ -11,7 +11,6 @@ use std::cell::OnceCell;
 use std::iter;
 
 use buck2_error::buck2_error;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use starlark::environment::GlobalsBuilder;
 use starlark::starlark_module;
@@ -66,7 +65,6 @@ pub fn new_dynamic_actions_callable<'v>(
             None,
             &DynamicActionsCallbackReturnType::starlark_type_repr(),
         )
-        .map_err(from_starlark)
         .buck_error_context("`impl` function must be callable with given params")?;
 
     let callable_ty = Ty::function(
@@ -77,7 +75,6 @@ pub fn new_dynamic_actions_callable<'v>(
                 ty.callable_param_ty(),
             )
         }))
-        .map_err(from_starlark)
         .internal_error("Signature must be correct")?,
         StarlarkDynamicActions::starlark_type_repr(),
     );

diff --git a/app/buck2_analysis/src/analysis/env.rs b/app/buck2_analysis/src/analysis/env.rs
@@ -28,7 +28,6 @@ use buck2_core::provider::label::ConfiguredProvidersLabel;
 use buck2_core::target::configured_target_label::ConfiguredTargetLabel;
 use buck2_core::unsafe_send_future::UnsafeSendFuture;
 use buck2_error::conversion::from_any;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use buck2_events::dispatch::get_dispatcher;
 use buck2_execute::digest_config::HasDigestConfig;
@@ -455,8 +454,7 @@ pub fn get_user_defined_rule_spec(
             ctx: ValueTyped<'v, AnalysisContext<'v>>,
         ) -> buck2_error::Result<Value<'v>> {
             let rule_impl = get_rule_impl(eval, &self.module, &self.name)?;
-            eval.eval_function(rule_impl.to_value(), &[ctx.to_value()], &[])
-                .map_err(|e| from_starlark(e).into())
+            Ok(eval.eval_function(rule_impl.to_value(), &[ctx.to_value()], &[])?)
         }
 
         fn promise_artifact_mappings<'v>(

diff --git a/app/buck2_analysis/src/attrs/resolve/configured_attr.rs b/app/buck2_analysis/src/attrs/resolve/configured_attr.rs
@@ -16,7 +16,6 @@ use buck2_build_api::interpreter::rule_defs::provider::dependency::DependencyGen
 use buck2_core::package::package_relative_path::PackageRelativePath;
 use buck2_core::package::source_path::SourcePath;
 use buck2_core::package::PackageLabel;
-use buck2_error::starlark_error::from_starlark;
 use buck2_interpreter::types::configured_providers_label::StarlarkConfiguredProvidersLabel;
 use buck2_interpreter::types::opaque_metadata::OpaqueMetadata;
 use buck2_interpreter::types::target_label::StarlarkTargetLabel;
@@ -132,9 +131,7 @@ impl ConfiguredAttrExt for ConfiguredAttr {
                 let mut res = SmallMap::with_capacity(dict.len());
                 for (k, v) in dict.iter() {
                     res.insert_hashed(
-                        k.resolve_single(pkg, ctx)?
-                            .get_hashed()
-                            .map_err(from_starlark)?,
+                        k.resolve_single(pkg, ctx)?.get_hashed()?,
                         v.resolve_single(pkg, ctx)?,
                     );
                 }
@@ -246,9 +243,7 @@ fn configured_attr_to_value<'v>(
 
             for (k, v) in map.iter() {
                 res.insert_hashed(
-                    configured_attr_to_value(&k, pkg, heap)?
-                        .get_hashed()
-                        .map_err(from_starlark)?,
+                    configured_attr_to_value(&k, pkg, heap)?.get_hashed()?,
                     configured_attr_to_value(&v, pkg, heap)?,
                 );
             }
@@ -272,7 +267,7 @@ fn configured_attr_to_value<'v>(
 
             for (trans, p) in t.deps.iter() {
                 map.insert_hashed(
-                    heap.alloc(trans).get_hashed().map_err(from_starlark)?,
+                    heap.alloc(trans).get_hashed()?,
                     heap.alloc(StarlarkConfiguredProvidersLabel::new(p.dupe())),
                 );
             }

diff --git a/app/buck2_anon_target/src/anon_target_attr_coerce.rs b/app/buck2_anon_target/src/anon_target_attr_coerce.rs
@@ -18,7 +18,6 @@ use buck2_build_api::interpreter::rule_defs::provider::dependency::Dependency;
 use buck2_build_api::interpreter::rule_defs::resolved_macro::ResolvedStringWithMacros;
 use buck2_core::provider::label::ProvidersLabel;
 use buck2_core::soft_error;
-use buck2_error::starlark_error::from_starlark;
 use buck2_interpreter::types::configured_providers_label::StarlarkProvidersLabel;
 use buck2_interpreter::types::target_label::StarlarkTargetLabel;
 use buck2_node::attrs::attr_type::bool::BoolLiteral;
@@ -60,7 +59,7 @@ impl AnonTargetAttrTypeCoerce for AttrType {
                 Some(s) => Ok(AnonTargetAttr::Bool(BoolLiteral(s))),
                 None => Err(AnonTargetCoercionError::type_error("bool", value).into()),
             },
-            AttrTypeInner::Int(_) => match i64::unpack_value(value).map_err(from_starlark)? {
+            AttrTypeInner::Int(_) => match i64::unpack_value(value)? {
                 Some(x) => Ok(AnonTargetAttr::Int(x)),
                 None => Err(AnonTargetCoercionError::type_error("int", value).into()),
             },
@@ -132,9 +131,7 @@ impl AnonTargetAttrTypeCoerce for AttrType {
                         id: promise_artifact.artifact.id.as_ref().clone(),
                         short_path: promise_artifact.short_path.clone(),
                     }))
-                } else if let Some(artifact_like) =
-                    ValueAsArtifactLike::unpack_value(value).map_err(from_starlark)?
-                {
+                } else if let Some(artifact_like) = ValueAsArtifactLike::unpack_value(value)? {
                     let artifact = artifact_like.0.get_bound_artifact()?;
                     Ok(AnonTargetAttr::Artifact(artifact))
                 } else {
@@ -226,7 +223,7 @@ fn to_anon_target_any(value: Value, ctx: &AnonAttrCtx) -> buck2_error::Result<An
         Ok(AnonTargetAttr::None)
     } else if let Some(x) = value.unpack_bool() {
         Ok(AnonTargetAttr::Bool(BoolLiteral(x)))
-    } else if let Some(x) = i64::unpack_value(value).map_err(from_starlark)? {
+    } else if let Some(x) = i64::unpack_value(value)? {
         Ok(AnonTargetAttr::Int(x))
     } else if let Some(x) = DictRef::from_value(value) {
         Ok(AnonTargetAttr::Dict(

diff --git a/app/buck2_anon_target/src/anon_target_attr_resolve.rs b/app/buck2_anon_target/src/anon_target_attr_resolve.rs
@@ -27,7 +27,6 @@ use buck2_build_api::keep_going::KeepGoing;
 use buck2_core::package::PackageLabel;
 use buck2_core::provider::label::ConfiguredProvidersLabel;
 use buck2_core::target::configured_target_label::ConfiguredTargetLabel;
-use buck2_error::starlark_error::from_starlark;
 use buck2_interpreter::types::configured_providers_label::StarlarkProvidersLabel;
 use buck2_node::attrs::attr_type::dep::DepAttrType;
 use buck2_node::attrs::attr_type::query::ResolvedQueryLiterals;
@@ -113,9 +112,7 @@ impl AnonTargetAttrResolution for AnonTargetAttr {
                 let mut res = SmallMap::with_capacity(dict.len());
                 for (k, v) in dict.iter() {
                     res.insert_hashed(
-                        k.resolve_single(pkg, anon_resolution_ctx)?
-                            .get_hashed()
-                            .map_err(from_starlark)?,
+                        k.resolve_single(pkg, anon_resolution_ctx)?.get_hashed()?,
                         v.resolve_single(pkg, anon_resolution_ctx)?,
                     );
                 }

diff --git a/app/buck2_anon_target/src/anon_target_node.rs b/app/buck2_anon_target/src/anon_target_node.rs
@@ -37,7 +37,6 @@ use buck2_core::target::configured_target_label::ConfiguredTargetLabel;
 use buck2_core::target::label::label::TargetLabel;
 use buck2_data::action_key_owner::BaseDeferredKeyProto;
 use buck2_data::ToProtoMessage;
-use buck2_error::starlark_error::from_starlark;
 use buck2_node::rule_type::StarlarkRuleType;
 use cmp_any::PartialEqAny;
 use dupe::Dupe;
@@ -244,13 +243,11 @@ impl AnonTargetDyn for AnonTarget {
         let mut fulfilled_artifact_mappings = HashMap::new();
 
         for (id, func) in promise_artifact_mappings.values().enumerate() {
-            let artifact = eval
-                .eval_function(*func, &[anon_target_result], &[])
-                .map_err(from_starlark)?;
+            let artifact = eval.eval_function(*func, &[anon_target_result], &[])?;
 
             let promise_id = PromiseArtifactId::new(BaseDeferredKey::AnonTarget(self.dupe()), id);
 
-            match ValueAsArtifactLike::unpack_value(artifact).map_err(from_starlark)? {
+            match ValueAsArtifactLike::unpack_value(artifact)? {
                 Some(artifact) => {
                     fulfilled_artifact_mappings
                         .insert(promise_id.clone(), artifact.0.get_bound_artifact()?);

diff --git a/app/buck2_build_api/src/actions/calculation.rs b/app/buck2_build_api/src/actions/calculation.rs
@@ -24,7 +24,6 @@ use buck2_data::ActionErrorDiagnostics;
 use buck2_data::ActionSubErrors;
 use buck2_data::ToProtoMessage;
 use buck2_error::conversion::from_any;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use buck2_event_observer::action_util::get_action_digest;
 use buck2_events::dispatch::async_record_root_spans;
@@ -430,7 +429,7 @@ fn try_run_error_handler(
                             )),
                         },
                         Err(e) => {
-                            let e = from_starlark(e).context("Error handler failed");
+                            let e = buck2_error::Error::from(e).context("Error handler failed");
                             Data::HandlerInvocationError(format!("{:#}", e))
                         }
                     };

diff --git a/app/buck2_build_api/src/analysis/extra_v.rs b/app/buck2_build_api/src/analysis/extra_v.rs
@@ -11,7 +11,6 @@ use std::cell::OnceCell;
 
 use allocative::Allocative;
 use buck2_error::conversion::from_any;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use gazebo::prelude::OptionExt;
 use starlark::any::ProvidesStaticType;
@@ -97,10 +96,9 @@ impl FrozenAnalysisExtraValue {
         module: &FrozenModule,
     ) -> buck2_error::Result<OwnedFrozenValueTyped<StarlarkAnyComplex<FrozenAnalysisExtraValue>>>
     {
-        module
+        Ok(module
             .owned_extra_value()
             .internal_error("extra_value not set")?
-            .downcast_starlark()
-            .map_err(from_starlark)
+            .downcast_starlark()?)
     }
 }
diff --git a/app/buck2_build_api/src/artifact_groups/registry.rs b/app/buck2_build_api/src/artifact_groups/registry.rs
@@ -8,7 +8,6 @@
  */
 
 use allocative::Allocative;
-use buck2_error::starlark_error::from_starlark;
 use dupe::Dupe;
 use starlark::eval::Evaluator;
 use starlark::values::FrozenValueTyped;
@@ -37,8 +36,8 @@ impl ArtifactGroupRegistry {
         eval: &mut Evaluator<'v, '_, '_>,
     ) -> starlark::Result<ValueTyped<'v, TransitiveSet<'v>>> {
         Ok(analysis_value_storage.register_transitive_set(move |key| {
-            let set = TransitiveSet::new_from_values(key.dupe(), definition, value, children, eval)
-                .map_err(from_starlark)?;
+            let set =
+                TransitiveSet::new_from_values(key.dupe(), definition, value, children, eval)?;
             Ok(eval.heap().alloc_typed(set))
         })?)
     }

diff --git a/app/buck2_build_api/src/interpreter/rule_defs/cmd_args/options.rs b/app/buck2_build_api/src/interpreter/rule_defs/cmd_args/options.rs
@@ -17,7 +17,6 @@ use buck2_core::fs::paths::RelativePath;
 use buck2_core::fs::paths::RelativePathBuf;
 use buck2_core::fs::project_rel_path::ProjectRelativePath;
 use buck2_core::fs::project_rel_path::ProjectRelativePathBuf;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use buck2_execute::artifact::fs::ExecutorFs;
 use buck2_interpreter::types::cell_root::CellRoot;
@@ -685,7 +684,6 @@ impl<'v, 'x> CommandLineOptionsRef<'v, 'x> {
 
         let origin = value
             .unpack()
-            .map_err(from_starlark)
             .internal_error("Must be a valid RelativeOrigin as this was checked in the setter")?;
         let mut relative_path = origin.resolve(ctx)?;
         for _ in 0..parent {

diff --git a/app/buck2_build_api/src/interpreter/rule_defs/provider/builtin/default_info.rs b/app/buck2_build_api/src/interpreter/rule_defs/provider/builtin/default_info.rs
@@ -16,7 +16,6 @@ use buck2_artifact::artifact::artifact_type::Artifact;
 use buck2_artifact::artifact::artifact_type::OutputArtifact;
 use buck2_build_api_derive::internal_provider;
 use buck2_error::conversion::from_any;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use dupe::Dupe;
 use starlark::any::ProvidesStaticType;
@@ -244,8 +243,7 @@ impl FrozenDefaultInfo {
                     starlark_artifact.dupe()
                 } else {
                     // This code path is for StarlarkPromiseArtifact. We have to create a `StarlarkArtifact` object here.
-                    let artifact_like = ValueAsArtifactLike::unpack_value(frozen_value.to_value())
-                        .map_err(from_starlark)?
+                    let artifact_like = ValueAsArtifactLike::unpack_value(frozen_value.to_value())?
                         .buck_error_context("Should be list of artifacts")?;
                     artifact_like.0.get_bound_starlark_artifact()?
                 },

diff --git a/app/buck2_build_api/src/interpreter/rule_defs/provider/builtin/execution_platform_info.rs b/app/buck2_build_api/src/interpreter/rule_defs/provider/builtin/execution_platform_info.rs
@@ -14,7 +14,6 @@ use buck2_build_api_derive::internal_provider;
 use buck2_core::configuration::data::ConfigurationData;
 use buck2_core::execution_types::execution::ExecutionPlatform;
 use buck2_core::target::label::label::TargetLabel;
-use buck2_error::starlark_error::from_starlark;
 use buck2_interpreter::types::target_label::StarlarkTargetLabel;
 use dupe::Dupe;
 use starlark::any::ProvidesStaticType;
@@ -57,12 +56,7 @@ pub struct ExecutionPlatformInfoGen<V: ValueLifetimeless> {
 
 impl<'v, V: ValueLike<'v>> ExecutionPlatformInfoGen<V> {
     pub fn to_execution_platform(&self) -> buck2_error::Result<ExecutionPlatform> {
-        let target = self
-            .label
-            .cast::<&StarlarkTargetLabel>()
-            .unpack()
-            .map_err(from_starlark)?
-            .label();
+        let target = self.label.cast::<&StarlarkTargetLabel>().unpack()?.label();
         let cfg = ConfigurationInfo::from_value(self.configuration.get().to_value())
             .ok_or_else(|| {
                 ExecutionPlatformProviderErrors::ExpectedConfigurationInfo(

diff --git a/app/buck2_build_api/src/interpreter/rule_defs/provider/builtin/external_runner_test_info.rs b/app/buck2_build_api/src/interpreter/rule_defs/provider/builtin/external_runner_test_info.rs
@@ -15,7 +15,6 @@ use buck2_build_api_derive::internal_provider;
 use buck2_core::provider::label::ConfiguredProvidersLabel;
 use buck2_error::buck2_error;
 use buck2_error::conversion::from_any;
-use buck2_error::starlark_error::from_starlark;
 use buck2_error::BuckErrorContext;
 use buck2_interpreter::types::configured_providers_label::StarlarkConfiguredProvidersLabel;
 use either::Either;
@@ -492,11 +491,9 @@ where
         }
     }
 
-    NoneOr::<bool>::unpack_value(info.use_project_relative_paths.get().to_value())
-        .map_err(from_starlark)?
+    NoneOr::<bool>::unpack_value(info.use_project_relative_paths.get().to_value())?
         .buck_error_context("`use_project_relative_paths` must be a bool if provided")?;
-    NoneOr::<bool>::unpack_value(info.run_from_project_root.get().to_value())
-        .map_err(from_starlark)?
+    NoneOr::<bool>::unpack_value(info.run_from_project_root.get().to_value())?
         .buck_error_context("`run_from_project_root` must be a bool if provided")?;
     unpack_opt_executor(info.default_executor.get().to_value())
         .buck_error_context("Invalid `default_executor`")?;