Add diagnostics for missing bodies for free and associated items

[BenjaminBrienen]

These are currently only handled by the flycheck, so this improves the UX.

In rustc, the *WithoutBody diagnostics are only constructed in impl Visitor for AstValidator.

• ConstWithoutBody is emitted for ItemKind::Const if body.is_none().
  • Note that ItemKind::Const only describes a module item, not an associated type, which is instead an AssocItemKind::Const.
• StaticWithoutBody is emitted for ItemKind::Static if body.is_none().
• TyAliasWithoutBody is emitted for ItemKind::TyAlias if body.is_none().
  • Note that ItemKind::TyAlias only describes a module item, not an associated type, which is instead an AssocItemKind::Type.
• AssocConstWithoutBody is emitted for AssocItemKind::Const if the ctxt is an AssocCtxt::Impl and the body is_none().
• AssocTypeWithoutBody is emitted for AssocItemKind::Type if the ctxt is an AssocCtxt::Impl and the body is_none()

- no other conditions or early returns for any of these that I can see.

FnWithoutBody is not handled here - it is gated behind if body.is_none() && !is_intrinsic && !self.is_sdylib_interface.

Note that while traits can default defaults for consts, statcs, and type aliases (unstable), in order to use these defaults in an impl, they must not be written at all (rather than not writing the body. This is confirmed by considering the implementation in rustc.

Stacked on #23190

[ChayimFriedman2]

Type aliases should have as well. Also, the parser test is for... what exactly? we don't add tests just because. In fact this already has tests.

View changes since this review

[ChayimFriedman2]

And then please squash.

[BenjaminBrienen]

I thought that the error codes worked a different way so "error 21: missing type for static" didn't give any results. I see it now, though.

[BenjaminBrienen]

Need guidance for how except the valid cases where there are no bodies as well as why the diagnostics in the expression store are not being emmitted for the static and const cases.

[ChayimFriedman2]

Rebasing wasn't such a good idea because you were on more advanced master. You can reset and cherry-pick (or I'll just rebase then you).

[BenjaminBrienen]

It should be the latest master + your branch + my change now

[ChayimFriedman2]

Handling consts, statics and fns should be done in body lowering, not signature lowering. All diagnostics should have the same struct/variant (and file), with a differentiating enum field if needed. Share code where possible, and use exhaustive matches.

View changes since this review

[BenjaminBrienen]

Makes sense. Done! :)

[BenjaminBrienen]

diff --git a/crates/hir-def/src/expr_store.rs b/crates/hir-def/src/expr_store.rs
index d46a98b168..ab6fadd062 100644
--- a/crates/hir-def/src/expr_store.rs
+++ b/crates/hir-def/src/expr_store.rs
@@ -321,15 +321,6 @@ struct FormatTemplate {
     implicit_capture_to_source: FxHashMap<ExprId, InFile<(ExprPtr, TextRange)>>,
 }
 
-#[derive(Debug, PartialEq, Eq, Clone, Copy, Hash)]
-pub enum AstNodeRequiringBody {
-    AssocConst(InFile<AstPtr<ast::Const>>),
-    AssocType(InFile<AstPtr<ast::TypeAlias>>),
-    Const(InFile<AstPtr<ast::Const>>),
-    Static(InFile<AstPtr<ast::Static>>),
-    TypeAlias(InFile<AstPtr<ast::TypeAlias>>),
-}
-
 #[derive(Debug, Eq, PartialEq)]
 pub enum ExpressionStoreDiagnostics {
     InactiveCode { node: InFile<SyntaxNodePtr>, cfg: CfgExpr, opts: CfgOptions },
@@ -339,7 +330,7 @@ pub enum ExpressionStoreDiagnostics {
     UndeclaredLabel { node: InFile<AstPtr<ast::Lifetime>>, name: Name },
     PatternArgInExternFn { node: InFile<AstPtr<ast::Pat>> },
     FruInDestructuringAssignment { node: InFile<AstPtr<ast::Expr>> },
-    MissingBody { node: AstNodeRequiringBody },
+    MissingBody { node: InFile<SyntaxNodePtr> },
 }
 
 impl ExpressionStoreBuilder {
diff --git a/crates/hir-def/src/expr_store/lower.rs b/crates/hir-def/src/expr_store/lower.rs
index b645148aa2..cdc5b6a711 100644
--- a/crates/hir-def/src/expr_store/lower.rs
+++ b/crates/hir-def/src/expr_store/lower.rs
@@ -40,9 +40,9 @@ use crate::{
     UnresolvedMacro,
     attrs::AttrFlags,
     expr_store::{
-        AstNodeRequiringBody, Body, BodySourceMap, ExprPtr, ExprRoot, ExpressionStore,
-        ExpressionStoreBuilder, ExpressionStoreDiagnostics, ExpressionStoreSourceMap, HygieneId,
-        LabelPtr, LifetimePtr, PatPtr, StoreVisitor, TypePtr,
+        Body, BodySourceMap, ExprPtr, ExprRoot, ExpressionStore, ExpressionStoreBuilder,
+        ExpressionStoreDiagnostics, ExpressionStoreSourceMap, HygieneId, LabelPtr, LifetimePtr,
+        PatPtr, StoreVisitor, TypePtr,
         body::Param,
         expander::Expander,
         lower::generics::ImplTraitLowerFn,
@@ -216,35 +216,36 @@ fn validate_required_body(
     if body.is_some() {
         return;
     }
-    match owner {
+    let in_file_wrapper: Option<InFile<SyntaxNodePtr>> = match owner {
         // FIXME: add diagnostic for missing body
         // rustc says: if body.is_none() && !is_intrinsic && !self.is_sdylib_interface
-        DefWithBodyId::FunctionId(_function_id) => {}
-        DefWithBodyId::StaticId(static_id) => match static_id.loc(db).container {
+        DefWithBodyId::FunctionId(_function_id) => None,
+        DefWithBodyId::StaticId(id) => match id.loc(db).container {
             ItemContainerId::ModuleId(_) => {
-                collector.store.diagnostics.push(ExpressionStoreDiagnostics::MissingBody {
-                    node: AstNodeRequiringBody::Static(static_id.loc(db).ast_ptr(db)),
-                });
+                let source = id.loc(db).source(db);
+                Some(InFile::new(source.file_id, SyntaxNodePtr::new(source.value.syntax())))
             }
             ItemContainerId::ExternBlockId(_)
             | ItemContainerId::ImplId(_)
-            | ItemContainerId::TraitId(_) => {}
+            | ItemContainerId::TraitId(_) => None,
         },
-        DefWithBodyId::ConstId(const_id) => match const_id.loc(db).container {
+        DefWithBodyId::ConstId(id) => match id.loc(db).container {
             ItemContainerId::ModuleId(_) => {
-                collector.store.diagnostics.push(ExpressionStoreDiagnostics::MissingBody {
-                    node: AstNodeRequiringBody::Const(const_id.loc(db).ast_ptr(db)),
-                });
+                let source = id.loc(db).source(db);
+                Some(InFile::new(source.file_id, SyntaxNodePtr::new(source.value.syntax())))
             }
             ItemContainerId::ImplId(_) => {
-                collector.store.diagnostics.push(ExpressionStoreDiagnostics::MissingBody {
-                    node: AstNodeRequiringBody::AssocConst(const_id.loc(db).ast_ptr(db)),
-                });
+                let source = id.loc(db).source(db);
+                Some(InFile::new(source.file_id, SyntaxNodePtr::new(source.value.syntax())))
             }
-            ItemContainerId::ExternBlockId(_) | ItemContainerId::TraitId(_) => {}
+            ItemContainerId::ExternBlockId(_) | ItemContainerId::TraitId(_) => None,
         },
-        DefWithBodyId::VariantId(_) => {}
-    }
+        DefWithBodyId::VariantId(_) => None,
+    };
+    let Some(node) = in_file_wrapper else {
+        return;
+    };
+    collector.store.diagnostics.push(ExpressionStoreDiagnostics::MissingBody { node });
 }
 
 pub(crate) fn lower_type_ref(
@@ -365,12 +366,12 @@ pub(crate) fn lower_type_alias(
         match container {
             ItemContainerId::ModuleId(_) => {
                 expr_collector.store.diagnostics.push(ExpressionStoreDiagnostics::MissingBody {
-                    node: AstNodeRequiringBody::TypeAlias(alias.map(|alias| AstPtr::new(&alias))),
+                    node: InFile::new(alias.file_id, SyntaxNodePtr::new(alias.value.syntax())),
                 });
             }
             ItemContainerId::ImplId(_) => {
                 expr_collector.store.diagnostics.push(ExpressionStoreDiagnostics::MissingBody {
-                    node: AstNodeRequiringBody::AssocType(alias.map(|alias| AstPtr::new(&alias))),
+                    node: InFile::new(alias.file_id, SyntaxNodePtr::new(alias.value.syntax())),
                 });
             }
             _ => (),
diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs
index e66be0ee48..a41e7f49d8 100644
--- a/crates/hir/src/diagnostics.rs
+++ b/crates/hir/src/diagnostics.rs
@@ -62,7 +62,7 @@ use crate::{
     struct_tail_raw,
 };
 
-pub use hir_def::{VariantId, expr_store::AstNodeRequiringBody};
+pub use hir_def::VariantId;
 pub use hir_ty::{
     GenericArgsProhibitedReason, IncorrectGenericsLenKind, ReturnKind,
     diagnostics::{CaseType, IncorrectCase},
@@ -404,7 +404,7 @@ pub struct FruInDestructuringAssignment {
 
 #[derive(Debug)]
 pub struct MissingBody {
-    pub node: AstNodeRequiringBody,
+    pub node: InFile<SyntaxNodePtr>,
 }
 
 #[derive(Debug)]
diff --git a/crates/ide-diagnostics/src/handlers/missing_body.rs b/crates/ide-diagnostics/src/handlers/missing_body.rs
index 50b2b16f99..483e5d6d1a 100644
--- a/crates/ide-diagnostics/src/handlers/missing_body.rs
+++ b/crates/ide-diagnostics/src/handlers/missing_body.rs
@@ -1,27 +1,24 @@
+use syntax::SyntaxKind;
+
 use crate::{Diagnostic, DiagnosticCode, DiagnosticsContext};
 
 // Diagnostic: missing-body
 //
 // This diagnostic is triggered when a body is missing.
 pub(crate) fn missing_body(ctx: &DiagnosticsContext<'_, '_>, d: &hir::MissingBody) -> Diagnostic {
-    let (message, node) = match d.node {
-        hir::AstNodeRequiringBody::AssocConst(node) => {
-            ("associated constant in `impl` without body", node.map(Into::into))
-        }
-        hir::AstNodeRequiringBody::AssocType(node) => {
-            ("associated type in `impl` without body", node.map(Into::into))
-        }
-        hir::AstNodeRequiringBody::Const(node) => {
-            ("free constant item without body", node.map(Into::into))
+    let message = match d.node.value.kind() {
+        SyntaxKind::CONST => {
+            "constant requires body"
         }
-        hir::AstNodeRequiringBody::Static(node) => {
-            ("free static item without body", node.map(Into::into))
+        SyntaxKind::STATIC => {
+            "static requires body"
         }
-        hir::AstNodeRequiringBody::TypeAlias(node) => {
-            ("free type alias without body", node.map(Into::into))
+        SyntaxKind::TYPE_ALIAS => {
+            "type alias requires body"
         }
+        _ => unreachable!(),
     };
-    Diagnostic::new_with_syntax_node_ptr(ctx, DiagnosticCode::SyntaxError, message, node).stable()
+    Diagnostic::new_with_syntax_node_ptr(ctx, DiagnosticCode::SyntaxError, message, d.node).stable()
 }
 
 #[cfg(test)]
@@ -34,7 +31,7 @@ mod tests {
             r#"
 trait Foo { const BAR: u32; }
 impl Foo for () { const BAR: u32; }
-                //^^^^^^^^^^^^^^^ error: associated constant in `impl` without body
+                //^^^^^^^^^^^^^^^ error: constant requires body
         "#,
             &["unused_variables"],
         );
@@ -46,7 +43,7 @@ impl Foo for () { const BAR: u32; }
             r#"
 trait Foo { type Bar; }
 impl Foo for () { type Bar; }
-                //^^^^^^^^^ error: associated type in `impl` without body
+                //^^^^^^^^^ error: type alias requires body
         "#,
             &["unused_variables"],
         );
@@ -57,7 +54,7 @@ impl Foo for () { type Bar; }
         check_diagnostics_with_disabled(
             r#"
   const FOO: u32;
-//^^^^^^^^^^^^^^^ error: free constant item without body
+//^^^^^^^^^^^^^^^ error: constant requires body
         "#,
             &["unused_variables"],
         );
@@ -68,7 +65,7 @@ impl Foo for () { type Bar; }
         check_diagnostics_with_disabled(
             r#"
   static FOO: u32;
-//^^^^^^^^^^^^^^^^ error: free static item without body
+//^^^^^^^^^^^^^^^^ error: static requires body
         "#,
             &["unused_variables"],
         );
@@ -79,7 +76,7 @@ impl Foo for () { type Bar; }
         check_diagnostics_with_disabled(
             r#"
   type Foo;
-//^^^^^^^^^ error: free type alias without body
+//^^^^^^^^^ error: type alias requires body
         "#,
             &["unused_variables"],
         );

IDK if this is what you want but you can take over if you feel like you can get it exactly how you want it easily

[ChayimFriedman2]

    MissingBody { node: InFile<SyntaxNodePtr>, kind: MissingBodyItemKind },

#[derive(Debug, PartialEq, Eq, Clone, Copy, Hash)]
pub enum MissingBodyItemKind {
    AssocConst,
    AssocType,
    Const,
    Static,
    TypeAlias,
}

[BenjaminBrienen]

I could probably also get container from the caller the same way.

[BenjaminBrienen]

Hm, almost. EnumVariantLoc doesn't have a container.

[ChayimFriedman2]

Thanks! But I'll wait for my PR to be merged before merging this.

View changes since this review

[ChayimFriedman2]

You can now rebase on master and I'll merge this.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[ChayimFriedman2]

Thanks!

View changes since this review