From 6e0c62b3dd4fd9fdc088b9152246713b677842ba Mon Sep 17 00:00:00 2001
From: yuvalsw <105596564+yuvalsw@users.noreply.github.com>
Date: Mon, 12 Feb 2024 20:51:37 +0200
Subject: [PATCH] Documented diagnostics cycle breaking. (#5051)

---
 crates/cairo-lang-semantic/src/items/imp.rs | 7 ++++++-
 crates/cairo-lang-semantic/src/items/trt.rs | 7 ++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/crates/cairo-lang-semantic/src/items/imp.rs b/crates/cairo-lang-semantic/src/items/imp.rs
index d1b0177ff18..7d39d63112d 100644
--- a/crates/cairo-lang-semantic/src/items/imp.rs
+++ b/crates/cairo-lang-semantic/src/items/imp.rs
@@ -428,6 +428,10 @@ pub fn priv_impl_declaration_data_inner(
 #[derive(Clone, Debug, PartialEq, Eq, DebugWithDb)]
 #[debug_db(dyn SemanticGroup + 'static)]
 pub struct ImplDefinitionData {
+    /// The diagnostics here are "flat" - that is, only the diagnostics found on the impl level
+    /// itself, and don't include the diagnostics of its items. The reason it's this way is that
+    /// computing the items' diagnostics require a query about their impl, forming a cycle of
+    /// queries. Adding the items' diagnostics only after the whole computation breaks this cycle.
     diagnostics: Diagnostics<SemanticDiagnostic>,
     function_asts: OrderedHashMap<ImplFunctionId, ast::FunctionWithBody>,
     item_type_asts: Arc<OrderedHashMap<ImplTypeId, ast::ItemTypeAlias>>,
@@ -446,7 +450,8 @@ pub fn impl_semantic_definition_diagnostics(
         return Diagnostics::default();
     };
 
-    // TODO(yuval): move these into priv_impl_definition_data.
+    // The diagnostics from `priv_impl_definition_data` are only the diagnostics from the impl
+    // level. They should be enriched with the items' diagnostics.
     diagnostics.extend(data.diagnostics);
     for impl_function_id in data.function_asts.keys() {
         diagnostics.extend(db.impl_function_declaration_diagnostics(*impl_function_id));
diff --git a/crates/cairo-lang-semantic/src/items/trt.rs b/crates/cairo-lang-semantic/src/items/trt.rs
index e9a575670ec..3ab146e9169 100644
--- a/crates/cairo-lang-semantic/src/items/trt.rs
+++ b/crates/cairo-lang-semantic/src/items/trt.rs
@@ -268,6 +268,10 @@ pub fn priv_trait_declaration_data(
 #[derive(Clone, Debug, PartialEq, Eq, DebugWithDb)]
 #[debug_db(dyn SemanticGroup + 'static)]
 pub struct TraitDefinitionData {
+    /// The diagnostics here are "flat" - that is, only the diagnostics found on the trait level
+    /// itself, and don't include the diagnostics of its items. The reason it's this way is that
+    /// computing the items' diagnostics require a query about their trait, forming a cycle of
+    /// queries. Adding the items' diagnostics only after the whole computation breaks this cycle.
     diagnostics: Diagnostics<SemanticDiagnostic>,
     function_asts: OrderedHashMap<TraitFunctionId, ast::TraitItemFunction>,
     item_type_asts: OrderedHashMap<TraitTypeId, ast::TraitItemType>,
@@ -286,7 +290,8 @@ pub fn trait_semantic_definition_diagnostics(
         return Diagnostics::default();
     };
 
-    // TODO(yuval): move these into priv_trait_definition_data.
+    // The diagnostics from `priv_trait_definition_data` are only the diagnostics from the trait
+    // level. They should be enriched with the items' diagnostics.
     diagnostics.extend(data.diagnostics);
     for trait_function_id in data.function_asts.keys() {
         diagnostics.extend(db.trait_function_declaration_diagnostics(*trait_function_id));