Skip to content

Chalk HasInterner derive macro doesn't work because it expands to named constant #9562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
flodiebold opened this issue Jul 11, 2021 · 3 comments · Fixed by #9572
Closed

Chalk HasInterner derive macro doesn't work because it expands to named constant #9562

flodiebold opened this issue Jul 11, 2021 · 3 comments · Fixed by #9572
Labels
A-macro macro expansion S-actionable Someone could pick this issue up and work on it right now

Comments

@flodiebold
Copy link
Member

flodiebold commented Jul 11, 2021

Reproduction:

use std::marker::PhantomData;

use chalk_ir::interner::{HasInterner, Interner};
use chalk_derive::HasInterner;

#[derive(HasInterner)]
struct WithInterner<I: Interner> {
    _phantom: PhantomData<I>,
}

fn test<I: Interner>(i: I) {
    let x: <WithInterner<I> as HasInterner>::Interner = i;
}

With chalk_ir and chalk_derive dependencies. x should be type I, which works if I write the HasInterner impl by hand, but not in the above code.

It turns out that the synstructure derive macro expands to an impl in a named constant:

#[allow(non_upper_case_globals)]
#[doc(hidden)]
const _DERIVE_chalk_ir_interner_HasInterner_FOR_WithInterner: () = {
    impl<I: Interner> ::chalk_ir::interner::HasInterner for WithInterner<I> {
        type Interner = I;
    }
};

Which we don't support. Maybe we can change synstructure to use unnamed consts, or build a hack to treat _DERIVE consts like unnamed ones 😬

@flodiebold flodiebold added A-macro macro expansion S-actionable Someone could pick this issue up and work on it right now labels Jul 11, 2021
@flodiebold flodiebold changed the title Chalk HasInterner derive macro not expanded Chalk HasInterner derive macro doesn't work because it expands to named constant Jul 11, 2021
@flodiebold
Copy link
Member Author

This patch does indeed fix it:

diff --git a/crates/hir_def/src/item_tree/lower.rs b/crates/hir_def/src/item_tree/lower.rs
index aa2dbaba1..48e6bb4a6 100644
--- a/crates/hir_def/src/item_tree/lower.rs
+++ b/crates/hir_def/src/item_tree/lower.rs
@@ -448,7 +448,10 @@ impl<'a> Ctx<'a> {
     }
 
     fn lower_const(&mut self, konst: &ast::Const) -> FileItemTreeId<Const> {
-        let name = konst.name().map(|it| it.as_name());
+        let mut name = konst.name().map(|it| it.as_name());
+        if name.as_ref().map_or(false, |n| n.to_string().starts_with("_DERIVE_")) {
+            name = None;
+        }
         let type_ref = self.lower_type_ref_opt(konst.ty());
         let visibility = self.lower_visibility(konst);
         let ast_id = self.source_ast_id_map.ast_id(konst);

@lnicola
Copy link
Member

lnicola commented Jul 11, 2021

CC mystor/synstructure#43

@flodiebold
Copy link
Member Author

Ah, nice

flodiebold added a commit to flodiebold/chalk that referenced this issue Jul 11, 2021
This makes the derives work in rust-analyzer.

CC rust-lang/rust-analyzer#9562
flodiebold added a commit to flodiebold/rust-analyzer that referenced this issue Jul 11, 2021
This treats the consts generated by older synstructure versions like
unnamed consts. We should remove this at some point (at least after
Chalk has switched).
@bors bors bot closed this as completed in 3fc5f01 Jul 11, 2021
flodiebold added a commit to flodiebold/chalk that referenced this issue Jul 11, 2021
This makes the derives work in rust-analyzer.

CC rust-lang/rust-analyzer#9562
bors added a commit to rust-lang/chalk that referenced this issue Jul 12, 2021
Use unnamed consts in chalk-derive

This makes the derives work in rust-analyzer.

CC rust-lang/rust-analyzer#9562
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macro macro expansion S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants