search

apollographql / apollo-rs

Spec compliant GraphQL Tools in Rust.
Apache License 2.0
566 stars 45 forks source link

Check validity of `ast::Name` (and assorted breaking changes) #713

Closed SimonSapin closed 10 months ago

SimonSapin commented 11 months ago

Fixes https://github.com/apollographql/apollo-rs/issues/710

Featuring unsafe tagged pointers and Miri on CI

goto-bus-stop commented 10 months ago

I like it a lot overall and I'm not opposed to a bit of easily auditable unsafe. How much use would we expect the no-alloc static representation to have? Well-known names like "Query" or directives implemented by a server would use it but I'm wondering if that's enough allocations to be worth the complexity 😅 There is an alternative option to validate the name!() input at compile time but still copy it into an allocation at runtime right? That might be a footgun if you use it in a hot loop I guess.

SimonSapin commented 10 months ago

I’d expect almost all static NodeStrs to come from the name! macro. Yes, having name! still check validity at compile-time but allocate on the heap is also a possibility. And yes, only a small minority of all Names involved e.g. when handling one request in the Router come from name!.

Even so, my opinion is that the complexity here is far from excessive: it’s entirely encapsulated in a ~300 lines modules (~100 of which are safe boilerplate impls). And verification by Miri gives some confidence in correctness. We routinely rely on Rowan and other libraries that also use unsafe internally (in specific modules, encapsulated with a safe abstraction.)