Implement Chalk's debug methods using TLS #3748
Conversation
|
What could have been... rust-lang/rust#25232 😛 |
| OP: FnOnce() -> R, | ||
| { | ||
| let ctx = DebugContext(p); | ||
| let static_p: &DebugContext<'static> = unsafe { std::mem::transmute(&ctx) }; |
There was a problem hiding this comment.
transmuteless alternative:, let p: &'static dyn HirDatabase = unsafe { &*(p as const _) };
There was a problem hiding this comment.
Doesn't work, I get lifetime 'static required 🤔
There was a problem hiding this comment.
The problem is that the dyn HirDatabase type might have borrowed data in it. I.e. the lifetime error goes away when I make the parameter &dyn HirDatabase + 'static, but then of course I get lifetime errors at the call site.
|
tbh, it's TLS and not unsafe that worries me most here... Like, this is just debug, so using tls is probably fine, but still, it feels like a giant hack. I'd much prefer to, eg, parametrize the solver with an option debug trait object. But I do believe that debuggability is super important, so TLS is better than no debug at all /me tries not to remember ripping out debug infra just couple of months ago. |
Chalk now panics if we don't implement these methods and run with CHALK_DEBUG, so I thought I'd try to implement them 'properly'. Sadly, it seems impossible to do without transmuting lifetimes somewhere. The problem is that we need a `&dyn HirDatabase` to get names etc., which we can't just put into TLS. I thought I could just use `scoped-tls`, but that doesn't support references to unsized types. So I put the `&dyn` into another struct and put the reference to *that* into the TLS, but I have to transmute the lifetime to 'static for that to work.
|
@matklad Let's merge this, unless you have objections? We explicitly only use the TLS if |
|
bors r+
…On Fri, 10 Apr 2020 at 15:08, Florian Diebold ***@***.***> wrote:
@matklad <https://github.com/matklad> Let's merge this, unless you have
objections? We explicitly only use the TLS if CHALK_DEBUG is set, so I'm
pretty sure this should have no effect at all on normal usage, but it's
helpful for debugging.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3748 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANB3M4MJHW4KQCLYB4YTE3RL4K3ZANCNFSM4LVGGPCA>
.
|
Build succeeded |
Chalk now panics if we don't implement these methods and run with CHALK_DEBUG, so I thought I'd try to implement them 'properly'. Sadly, it seems impossible to do without transmuting lifetimes somewhere. The problem is that we need a
&dyn HirDatabaseto get names etc., which we can't just put into TLS. I thought I could just usescoped-tls, but that doesn't support references to unsized types. So I put the&dyninto another struct and put the reference to that into the TLS, but I have to transmute the lifetime to 'static for that to work. I think this is sound, but I still don't really want to do it this way...Having names in the Chalk debug output is very nice, but maybe IDs will have to suffice 😞