Use PromiseArtifactAttr in AnonTargetAttr::PromiseArtifact

Summary:
This is the final step in fixing the race:

1. Replace `AnonTargetAttr::PromiseArtifact`'s value to be `PromiseArtifactAttr`
2. Traverse the anon target's attrs to grab all of the promise artifact attrs
3. Run analysis to construct the `AnonTargetAttrResolutionContext`
4. Construct new fulfilled `StarlarkPromiseArtifact`s to return to the consumer analysis

Reviewed By: cjhopman

Differential Revision: D52741554

fbshipit-source-id: ce2a5031a74cd1139fe8e4be0fec63f098183bd5

app/buck2_anon_target/src/anon_target_attr.rs

@@ -12,7 +12,6 @@ use std::fmt::Display;
 
 use allocative::Allocative;
 use buck2_artifact::artifact::artifact_type::Artifact;
-use buck2_build_api::interpreter::rule_defs::artifact::StarlarkPromiseArtifact;
 use buck2_core::package::PackageLabel;
 use buck2_core::provider::label::ConfiguredProvidersLabel;
 use buck2_core::provider::label::ProvidersLabel;
@@ -38,6 +37,7 @@ use serde::Serializer;
 use serde_json::to_value;
 
 use crate::anon_target_attr_resolve::AnonTargetAttrTraversal;
+use crate::promise_artifacts::PromiseArtifactAttr;
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash, Allocative)]
 pub enum AnonTargetAttr {
@@ -66,7 +66,7 @@ pub enum AnonTargetAttr {
     // Accepts any bound artifacts. Maps to `attr.source()`.
     Artifact(Artifact),
     // Accepts unresolved promise artifacts. Maps to `attr.source()`.
-    PromiseArtifact(StarlarkPromiseArtifact),
+    PromiseArtifact(PromiseArtifactAttr),
     Arg(ConfiguredStringWithMacros),
     Label(ProvidersLabel),
 }
@@ -183,7 +183,7 @@ impl AnonTargetAttr {
     #[allow(unused)]
     pub fn traverse_anon_attr<'a>(
         &'a self,
-        _traversal: &mut dyn AnonTargetAttrTraversal,
+        traversal: &mut dyn AnonTargetAttrTraversal,
     ) -> anyhow::Result<()> {
         match self {
             AnonTargetAttr::Bool(_) => Ok(()),
@@ -192,31 +192,30 @@ impl AnonTargetAttr {
             AnonTargetAttr::EnumVariant(_) => Ok(()),
             AnonTargetAttr::List(list) => {
                 for v in list.iter() {
-                    v.traverse_anon_attr(_traversal)?;
+                    v.traverse_anon_attr(traversal)?;
                 }
                 Ok(())
             }
             AnonTargetAttr::Tuple(list) => {
                 for v in list.iter() {
-                    v.traverse_anon_attr(_traversal)?;
+                    v.traverse_anon_attr(traversal)?;
                 }
                 Ok(())
             }
             AnonTargetAttr::Dict(dict) => {
                 for (k, v) in dict.iter() {
-                    k.traverse_anon_attr(_traversal)?;
-                    v.traverse_anon_attr(_traversal)?;
+                    k.traverse_anon_attr(traversal)?;
+                    v.traverse_anon_attr(traversal)?;
                 }
                 Ok(())
             }
             AnonTargetAttr::None => Ok(()),
-            AnonTargetAttr::OneOf(l, _) => l.traverse_anon_attr(_traversal),
+            AnonTargetAttr::OneOf(l, _) => l.traverse_anon_attr(traversal),
             AnonTargetAttr::Dep(_) => Ok(()),
             AnonTargetAttr::Artifact(_) => Ok(()),
             AnonTargetAttr::Arg(_) => Ok(()),
-            AnonTargetAttr::PromiseArtifact(_) => {
-                // TODO(@wendyy) - use traversal here after updating the attr type
-                Ok(())
+            AnonTargetAttr::PromiseArtifact(promise_artifact) => {
+                traversal.promise_artifact(promise_artifact)
             }
             AnonTargetAttr::Label(_) => Ok(()),
         }

app/buck2_anon_target/src/anon_target_attr_coerce.rs

@@ -46,6 +46,7 @@ use starlark::values::Value;
 
 use crate::anon_target_attr::AnonTargetAttr;
 use crate::anon_targets::AnonAttrCtx;
+use crate::promise_artifacts::PromiseArtifactAttr;
 
 pub trait AnonTargetAttrTypeCoerce {
     fn coerce_item(&self, ctx: &AnonAttrCtx, value: Value) -> anyhow::Result<AnonTargetAttr>;
@@ -134,7 +135,10 @@ impl AnonTargetAttrTypeCoerce for AttrType {
                 // Check if this is a StarlarkPromiseArtifact first before checking other artifact types to
                 // allow anon targets to accept unresolved promise artifacts.
                 if let Some(promise_artifact) = StarlarkPromiseArtifact::from_value(value) {
-                    Ok(AnonTargetAttr::PromiseArtifact(promise_artifact.clone()))
+                    Ok(AnonTargetAttr::PromiseArtifact(PromiseArtifactAttr {
+                        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) {
                     let artifact = artifact_like.0.get_bound_artifact()?;
                     Ok(AnonTargetAttr::Artifact(artifact))

app/buck2_anon_target/src/anon_target_attr_resolve.rs

@@ -8,14 +8,19 @@
  */
 
 use std::collections::HashMap;
+use std::sync::Arc;
+use std::sync::OnceLock;
 
 use buck2_analysis::analysis::env::RuleAnalysisAttrResolutionContext;
 use buck2_analysis::attrs::resolve::attr_type::arg::ConfiguredStringWithMacrosExt;
 use buck2_analysis::attrs::resolve::attr_type::dep::DepAttrTypeExt;
 use buck2_analysis::attrs::resolve::ctx::AttrResolutionContext;
 use buck2_artifact::artifact::artifact_type::Artifact;
 use buck2_build_api::analysis::calculation::RuleAnalysisCalculation;
+use buck2_build_api::artifact_groups::promise::PromiseArtifact;
+use buck2_build_api::artifact_groups::promise::PromiseArtifactResolveError;
 use buck2_build_api::interpreter::rule_defs::artifact::StarlarkArtifact;
+use buck2_build_api::interpreter::rule_defs::artifact::StarlarkPromiseArtifact;
 use buck2_build_api::interpreter::rule_defs::provider::collection::FrozenProviderCollectionValue;
 use buck2_build_api::keep_going;
 use buck2_core::package::PackageLabel;
@@ -43,7 +48,6 @@ use crate::promise_artifacts::PromiseArtifactAttr;
 // No macros in anon targets, so query results are empty. Execution platform resolution should
 // always be inherited from the anon target.
 pub(crate) struct AnonTargetAttrResolutionContext<'v> {
-    #[allow(unused)] // TODO(@wendyy)
     pub(crate) promised_artifacts_map: HashMap<&'v PromiseArtifactAttr, Artifact>,
     pub(crate) rule_analysis_attr_resolution_ctx: RuleAnalysisAttrResolutionContext<'v>,
 }
@@ -122,9 +126,45 @@ impl AnonTargetAttrResolution for AnonTargetAttr {
             AnonTargetAttr::Dep(d) => DepAttrType::resolve_single(ctx, d),
             AnonTargetAttr::Artifact(d) => Ok(ctx.heap().alloc(StarlarkArtifact::new(d.clone()))),
             AnonTargetAttr::Arg(a) => a.resolve(ctx, &pkg),
-            AnonTargetAttr::PromiseArtifact(artifact) => {
-                // TODO(@wendyy) - use promised artifact map here to construct fulfilled `StarlarkPromiseArtifact`
-                Ok(ctx.heap().alloc(artifact.clone()))
+            AnonTargetAttr::PromiseArtifact(promise_artifact_attr) => {
+                let promise_id = promise_artifact_attr.id.clone();
+                // We validated that the analysis contains the promise artifact id earlier
+                let artifact = anon_resolution_ctx
+                    .promised_artifacts_map
+                    .get(&promise_artifact_attr)
+                    .unwrap();
+
+                // Assert the short path, since we have the real artifact now
+                if let Some(expected_short_path) = &promise_artifact_attr.short_path {
+                    artifact.get_path().with_short_path(|artifact_short_path| {
+                        if artifact_short_path != expected_short_path {
+                            Err(anyhow::Error::from(
+                                PromiseArtifactResolveError::ShortPathMismatch(
+                                    expected_short_path.clone(),
+                                    artifact_short_path.to_string(),
+                                ),
+                            ))
+                        } else {
+                            Ok(())
+                        }
+                    })?;
+                }
+
+                let fulfilled = OnceLock::new();
+                fulfilled.set(artifact.clone()).unwrap();
+
+                let fulfilled_promise_inner =
+                    PromiseArtifact::new(Arc::new(fulfilled), Arc::new(promise_id));
+
+                let fulfilled_promise_artifact = StarlarkPromiseArtifact::new(
+                    None,
+                    fulfilled_promise_inner,
+                    promise_artifact_attr.short_path.clone(),
+                );
+
+                // To resolve the promise artifact attr, we end up creating a new `StarlarkPromiseArtifact` with the `OnceLock` set
+                // with the artifact that was found from the upstream analysis.
+                Ok(ctx.heap().alloc(fulfilled_promise_artifact))
             }
             AnonTargetAttr::Label(label) => {
                 Ok(ctx.heap().alloc(StarlarkProvidersLabel::new(label.clone())))
@@ -155,6 +195,7 @@ impl AnonTargetDependents {
         anon_target: &AnonTargetKey,
     ) -> anyhow::Result<AnonTargetDependents> {
         struct DepTraversal(Vec<ConfiguredTargetLabel>);
+        struct PromiseArtifactTraversal(Vec<PromiseArtifactAttr>);
 
         impl ConfiguredAttrTraversal for DepTraversal {
             fn dep(&mut self, dep: &ConfiguredProvidersLabel) -> anyhow::Result<()> {
@@ -171,15 +212,25 @@ impl AnonTargetDependents {
             }
         }
 
+        impl AnonTargetAttrTraversal for PromiseArtifactTraversal {
+            fn promise_artifact(
+                &mut self,
+                promise_artifact: &PromiseArtifactAttr,
+            ) -> anyhow::Result<()> {
+                self.0.push(promise_artifact.clone());
+                Ok(())
+            }
+        }
+
         let mut dep_traversal = DepTraversal(Vec::new());
-        // @TODO(@wendyy) - populate after switching over to PromiseArtifactAttrs
-        let promise_artifacts = Vec::new();
+        let mut promise_artifact_traversal = PromiseArtifactTraversal(Vec::new());
         for x in anon_target.0.attrs().values() {
             x.traverse(anon_target.0.name().pkg(), &mut dep_traversal)?;
+            x.traverse_anon_attr(&mut promise_artifact_traversal)?;
         }
         Ok(AnonTargetDependents {
             deps: dep_traversal.0,
-            promise_artifacts,
+            promise_artifacts: promise_artifact_traversal.0,
         })
     }
 

app/buck2_anon_target/src/promise_artifacts.rs

@@ -75,7 +75,7 @@ impl PromiseArtifactRegistry {
 #[derive(Debug, Clone, PartialEq, Eq, Hash, Allocative)]
 pub(crate) struct PromiseArtifactAttr {
     pub(crate) id: PromiseArtifactId,
-    short_path: Option<ForwardRelativePathBuf>,
+    pub(crate) short_path: Option<ForwardRelativePathBuf>,
 }
 
 impl fmt::Display for PromiseArtifactAttr {

app/buck2_build_api/src/interpreter/rule_defs/artifact/starlark_promise_artifact.rs

@@ -91,7 +91,7 @@ enum PromiseArtifactError {
 pub struct StarlarkPromiseArtifact {
     pub declaration_location: Option<FileSpan>,
     pub artifact: PromiseArtifact,
-    short_path: Option<ForwardRelativePathBuf>,
+    pub short_path: Option<ForwardRelativePathBuf>,
 }
 
 starlark_simple_value!(StarlarkPromiseArtifact);