Merge #10807

10807: fix: Diagnose invalid derive attribute input r=Veykril a=Veykril

Doesn't yet diagnose incorrect syntax between the `(`, `)` braces as we discard those problems currently.
bors r+

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

crates/hir/src/diagnostics.rs

@@ -34,6 +34,7 @@ diagnostics![
     IncorrectCase,
     InvalidDeriveTarget,
     MacroError,
+    MalformedDerive,
     MismatchedArgCount,
     MissingFields,
     MissingMatchArms,
@@ -104,6 +105,11 @@ pub struct InvalidDeriveTarget {
     pub node: InFile<SyntaxNodePtr>,
 }
 
+#[derive(Debug)]
+pub struct MalformedDerive {
+    pub node: InFile<SyntaxNodePtr>,
+}
+
 #[derive(Debug)]
 pub struct NoSuchField {
     pub field: InFile<AstPtr<ast::RecordExprField>>,

crates/hir/src/lib.rs

@@ -83,10 +83,11 @@ pub use crate::{
     attrs::{HasAttrs, Namespace},
     diagnostics::{
         AddReferenceHere, AnyDiagnostic, BreakOutsideOfLoop, InactiveCode, IncorrectCase,
-        InvalidDeriveTarget, MacroError, MismatchedArgCount, MissingFields, MissingMatchArms,
+        InvalidDeriveTarget, MacroError, MalformedDerive, MismatchedArgCount, MissingFields,
-        MissingOkOrSomeInTailExpr, MissingUnsafe, NoSuchField, RemoveThisSemicolon,
+        MissingMatchArms, MissingOkOrSomeInTailExpr, MissingUnsafe, NoSuchField,
-        ReplaceFilterMapNextWithFindMap, UnimplementedBuiltinMacro, UnresolvedExternCrate,
+        RemoveThisSemicolon, ReplaceFilterMapNextWithFindMap, UnimplementedBuiltinMacro,
-        UnresolvedImport, UnresolvedMacroCall, UnresolvedModule, UnresolvedProcMacro,
+        UnresolvedExternCrate, UnresolvedImport, UnresolvedMacroCall, UnresolvedModule,
+        UnresolvedProcMacro,
     },
     has_source::HasSource,
     semantics::{PathResolution, Semantics, SemanticsScope, TypeInfo},
@@ -669,6 +670,21 @@ impl Module {
                         None => stdx::never!("derive diagnostic on item without derive attribute"),
                     }
                 }
+                DefDiagnosticKind::MalformedDerive { ast, id } => {
+                    let node = ast.to_node(db.upcast());
+                    let derive = node.attrs().nth(*id as usize);
+                    match derive {
+                        Some(derive) => {
+                            acc.push(
+                                MalformedDerive {
+                                    node: ast.with_value(SyntaxNodePtr::from(AstPtr::new(&derive))),
+                                }
+                                .into(),
+                            );
+                        }
+                        None => stdx::never!("derive diagnostic on item without derive attribute"),
+                    }
+                }
             }
         }
         for decl in self.declarations(db) {

crates/hir_def/src/nameres/collector.rs

@@ -8,7 +8,6 @@ use std::iter;
 use base_db::{CrateId, Edition, FileId, ProcMacroId};
 use cfg::{CfgExpr, CfgOptions};
 use hir_expand::{
-    ast_id_map::FileAstId,
     builtin_attr_macro::find_builtin_attr,
     builtin_derive_macro::find_builtin_derive,
     builtin_fn_macro::find_builtin_macro,
@@ -1081,8 +1080,10 @@ impl DefCollector<'_> {
                         return false;
                     }
                 }
-                MacroDirectiveKind::Attr { ast_id, mod_item, attr } => {
+                MacroDirectiveKind::Attr { ast_id: file_ast_id, mod_item, attr } => {
-                    let file_id = ast_id.ast_id.file_id;
+                    let &AstIdWithPath { ast_id, ref path } = file_ast_id;
+                    let file_id = ast_id.file_id;
+
                     let mut recollect_without = |collector: &mut Self, item_tree| {
                         // Remove the original directive since we resolved it.
                         let mod_dir = collector.mod_dirs[&directive.module_id].clone();
@@ -1100,8 +1101,8 @@ impl DefCollector<'_> {
                         false
                     };
 
-                    if let Some(ident) = ast_id.path.as_ident() {
+                    if let Some(ident) = path.as_ident() {
-                        if let Some(helpers) = self.derive_helpers_in_scope.get(&ast_id.ast_id) {
+                        if let Some(helpers) = self.derive_helpers_in_scope.get(&ast_id) {
                             if helpers.contains(ident) {
                                 cov_mark::hit!(resolved_derive_helper);
                                 // Resolved to derive helper. Collect the item's attributes again,
@@ -1112,7 +1113,7 @@ impl DefCollector<'_> {
                         }
                     }
 
-                    let def = resolver(ast_id.path.clone()).filter(MacroDefId::is_attribute);
+                    let def = resolver(path.clone()).filter(MacroDefId::is_attribute);
                     if matches!(
                         def,
                         Some(MacroDefId {  kind:MacroDefKind::BuiltInAttr(expander, _),.. })
@@ -1121,26 +1122,23 @@ impl DefCollector<'_> {
                         // Resolved to `#[derive]`
                         let item_tree = self.db.file_item_tree(file_id);
 
-                        let ast_id: FileAstId<ast::Item> = match *mod_item {
+                        match mod_item {
-                            ModItem::Struct(it) => item_tree[it].ast_id.upcast(),
+                            ModItem::Struct(_) | ModItem::Union(_) | ModItem::Enum(_) => (),
-                            ModItem::Union(it) => item_tree[it].ast_id.upcast(),
-                            ModItem::Enum(it) => item_tree[it].ast_id.upcast(),
                             _ => {
                                 let diag = DefDiagnostic::invalid_derive_target(
                                     directive.module_id,
-                                    ast_id.ast_id,
+                                    ast_id,
                                     attr.id,
                                 );
                                 self.def_map.diagnostics.push(diag);
-                                res = ReachedFixedPoint::No;
+                                return recollect_without(self, &item_tree);
-                                return false;
                             }
-                        };
+                        }
 
                         match attr.parse_derive() {
                             Some(derive_macros) => {
                                 for path in derive_macros {
-                                    let ast_id = AstIdWithPath::new(file_id, ast_id, path);
+                                    let ast_id = AstIdWithPath::new(file_id, ast_id.value, path);
                                     self.unresolved_macros.push(MacroDirective {
                                         module_id: directive.module_id,
                                         depth: directive.depth + 1,
@@ -1152,8 +1150,12 @@ impl DefCollector<'_> {
                                 }
                             }
                             None => {
-                                // FIXME: diagnose
+                                let diag = DefDiagnostic::malformed_derive(
-                                tracing::debug!("malformed derive: {:?}", attr);
+                                    directive.module_id,
+                                    ast_id,
+                                    attr.id,
+                                );
+                                self.def_map.diagnostics.push(diag);
                             }
                         }
 
@@ -1165,7 +1167,8 @@ impl DefCollector<'_> {
                     }
 
                     // Not resolved to a derive helper or the derive attribute, so try to resolve as a normal attribute.
-                    match attr_macro_as_call_id(ast_id, attr, self.db, self.def_map.krate, def) {
+                    match attr_macro_as_call_id(file_ast_id, attr, self.db, self.def_map.krate, def)
+                    {
                         Ok(call_id) => {
                             let loc: MacroCallLoc = self.db.lookup_intern_macro_call(call_id);
 
@@ -1198,7 +1201,7 @@ impl DefCollector<'_> {
 
                             self.def_map.modules[directive.module_id]
                                 .scope
-                                .add_attr_macro_invoc(ast_id.ast_id, call_id);
+                                .add_attr_macro_invoc(ast_id, call_id);
 
                             resolved.push((directive.module_id, call_id, directive.depth));
                             res = ReachedFixedPoint::No;

crates/hir_def/src/nameres/diagnostics.rs

@@ -32,6 +32,8 @@ pub enum DefDiagnosticKind {
     UnimplementedBuiltinMacro { ast: AstId<ast::Macro> },
 
     InvalidDeriveTarget { ast: AstId<ast::Item>, id: u32 },
+
+    MalformedDerive { ast: AstId<ast::Item>, id: u32 },
 }
 
 #[derive(Debug, PartialEq, Eq)]
@@ -116,4 +118,15 @@ impl DefDiagnostic {
             kind: DefDiagnosticKind::InvalidDeriveTarget { ast, id: id.ast_index },
         }
     }
+
+    pub(super) fn malformed_derive(
+        container: LocalModuleId,
+        ast: AstId<ast::Item>,
+        id: AttrId,
+    ) -> Self {
+        Self {
+            in_module: container,
+            kind: DefDiagnosticKind::MalformedDerive { ast, id: id.ast_index },
+        }
+    }
 }

crates/ide_diagnostics/src/handlers/invalid_derive_target.rs

@@ -8,7 +8,6 @@ pub(crate) fn invalid_derive_target(
     ctx: &DiagnosticsContext<'_>,
     d: &hir::InvalidDeriveTarget,
 ) -> Diagnostic {
-    // Use more accurate position if available.
     let display_range = ctx.sema.diagnostics_display_range(d.node.clone()).range;
 
     Diagnostic::new(

crates/ide_diagnostics/src/handlers/malformed_derive.rs

@@ -0,0 +1,37 @@
+use crate::{Diagnostic, DiagnosticsContext, Severity};
+
+// Diagnostic: malformed-derive
+//
+// This diagnostic is shown when the derive attribute has invalid input.
+pub(crate) fn malformed_derive(
+    ctx: &DiagnosticsContext<'_>,
+    d: &hir::MalformedDerive,
+) -> Diagnostic {
+    let display_range = ctx.sema.diagnostics_display_range(d.node.clone()).range;
+
+    Diagnostic::new(
+        "malformed-derive",
+        "malformed derive input, derive attributes are of the form `#[derive(Derive1, Derive2, ...)]`",
+        display_range,
+    )
+    .severity(Severity::Error)
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::tests::check_diagnostics;
+
+    #[test]
+    fn invalid_input() {
+        check_diagnostics(
+            r#"
+//- minicore:derive
+mod __ {
+    #[derive = "aaaa"]
+  //^^^^^^^^^^^^^^^^^^ error: malformed derive input, derive attributes are of the form `#[derive(Derive1, Derive2, ...)]`
+    struct Foo;
+}
+            "#,
+        );
+    }
+}

crates/ide_diagnostics/src/lib.rs

@@ -30,6 +30,7 @@ mod handlers {
     pub(crate) mod incorrect_case;
     pub(crate) mod invalid_derive_target;
     pub(crate) mod macro_error;
+    pub(crate) mod malformed_derive;
     pub(crate) mod mismatched_arg_count;
     pub(crate) mod missing_fields;
     pub(crate) mod missing_match_arms;
@@ -182,6 +183,7 @@ pub fn diagnostics(
             AnyDiagnostic::BreakOutsideOfLoop(d) => handlers::break_outside_of_loop::break_outside_of_loop(&ctx, &d),
             AnyDiagnostic::IncorrectCase(d) => handlers::incorrect_case::incorrect_case(&ctx, &d),
             AnyDiagnostic::MacroError(d) => handlers::macro_error::macro_error(&ctx, &d),
+            AnyDiagnostic::MalformedDerive(d) => handlers::malformed_derive::malformed_derive(&ctx, &d),
             AnyDiagnostic::MismatchedArgCount(d) => handlers::mismatched_arg_count::mismatched_arg_count(&ctx, &d),
             AnyDiagnostic::MissingFields(d) => handlers::missing_fields::missing_fields(&ctx, &d),
             AnyDiagnostic::MissingMatchArms(d) => handlers::missing_match_arms::missing_match_arms(&ctx, &d),