Deny the unimplemented! macro, it is too similar to unsupported!

[quodlibetor]

unimplemented! has almost made it through code review in a place that
unsupported was intended at least once, despite 5 people performing code
review (#7507). The unimplemented macro is just shorthand around
panic!("unimplemented"); so require that if folks really want to panic.

[sploiselle]

TIL! +1

[benesch]

All those existing uses of unimplemented! were totally valid, though! Seems wrong to punish all those existing valid call sites because the sql crate decided to use the name unsupported! to mean something different. In general would prefer to adapt our code around the design of the stdlib, rather than banning parts of the stdlib to make space for our stuff.

So concretely: what if we instead renamed the unsupported! macro to something less similar? Maybe not_yet_supported! or something.

[ruchirK]

yeah I'll just add that I use unimplemented!() a lot in prototyping and it would be a bummer to not have access to that

[quodlibetor]

All those existing uses of unimplemented! were totally valid, though! Seems wrong to punish all those existing valid call sites

Is this a punishment, or just being more explicit? The problem isn't just that unsupported is a similar length to unimplemented, it's also conceptually the same, but they have very different behaviors, and unimplemented will autocomplete if folks are just vaguely thinking about "this feature isn't supported". Calling the SQL macro not_yet_supported would probably help, but it still remains an education problem instead of something that tooling can just prevent.

yeah I'll just add that I use unimplemented!() a lot in prototyping and it would be a bummer to not have access to that

You're still allowed to use it locally (same as todo!) it'll only cause errors if you run bin/check -- cargo check/cargo build will still be fine.

[benesch]

I think there are two separate discussions to be had here:

1. Does the unsupported! macro in the sql crate have a name that is too similar to unimplemented!?
2. Is it confusing that unimplemented! panics? Are we routinely accidentally introducing calls to unimplemented! when we meant to return an error?

I think (1) is not contentious. I'd maybe even advocate for making that a function that returns an error instead, and then changing the call sites to manually return the error: unsupported_err("message")?.

Re (2), I am not convinced that this is a persistent source of confusion that needs to be addressed by banning use of unimplemented! everywhere in our codebase. I'm more sympathetic to the argument in a vacuum—it is perhaps confusing to newcomers to the language that the stdlib has so many ways to cause panics—but at the same time, there is value in not fighting the language. When writing Rust, you just have to remember that the following things panic:

• panic!
• todo!
• unimplemented!
• Option.unwrap
• Result.unwrap
• Out-of-bounds slice indexing

It just doesn't seem to me like unimplemented! is particularly harder to spot than any of the other things that can panic?

[danhhz]

Additional data point, it seems the idiom that's developed in the rust community (citation needed) is that todo! is for prototyping and something you want to make sure you don't accidentally check in and unimplemented! is for declaring that you've intentionally not implemented something.

I'm personally also against this change. As Nikhil says, the current uses of unimplemented! are correct, and the issue of confusion with the sql macro is both separable and avoidable a different way.

[umanwizard]

No strong opinions on this. But if we end up not doing this, +1 on renaming unsupported to something that makes the behavior clearer.

[umanwizard]

@benesch and I discussed this and came to what we think is a reasonable consensus:

• unimplemented should be banned, but only in the main binary. The reasoning is that our desired robustness guarantee is that we don't panic unless a state we believe to be logically impossible has occurred, or something is so broken that it's impossible to continue, and those cases are already covered by unreachable! and panic!

• Cases where unimplemented! genuinely makes sense in the main binary (e.g., undocumented features that we wanted to land partially in the main branch) should be rare enough that workarounds like using panic! are fine.

• We should continue to allow unimplemented! in tests and non-critical scripts, e.g. kgen .

• This will require an extra step in our linter script, since we will want to do this particular check only on --bin materialized .

• Separately, we can rename the SQL crate's unsupported! crate to bail_unsupported! to better express the intent.

[benesch]

Here's the commit to do (1) per @umanwizard's suggestion: #8001

[benesch]

@benesch and I discussed this and came to what we think is a reasonable consensus:

• unimplemented should be banned, but only in the main binary. The reasoning is that our desired robustness guarantee is that we don't panic unless a state we believe to be logically impossible has occurred, or something is so broken that it's impossible to continue, and those cases are already covered by unreachable! and panic!
• Cases where unimplemented! genuinely makes sense in the main binary (e.g., undocumented features that we wanted to land partially in the main branch) should be rare enough that workarounds like using panic! are fine.
• We should continue to allow unimplemented! in tests and non-critical scripts, e.g. kgen .
• This will require an extra step in our linter script, since we will want to do this particular check only on --bin materialized .

I'm in support of this philosophy, but it's unfortunately going to be harder to enforce than described here. The sql crate exports a DummyCatalog that is used by tests in other crates. So it's ok that it uses unimplemented!, but it's kind of hard to perform that analysis statically.

[umanwizard]

Can we just override it with an #[allow on that particular impl?

[umanwizard]

The principled solution seems to be: put that behind a feature flag, and have the crates that rely on it turn on that feature flag only for their test dependencies. But that's a lot of hassle for marginal benefit, IMO.

Really, this (and some other things about our build process, like dead code detection, mandatory docs, etc.) are made more complicated by the fact that the crate is not intended to be a meaningful abstraction boundary in Materialize, and we have split ourselves into crates somewhat arbitrarily as a hack to improve compile times.

[quodlibetor]

bail_unsupported does seem like it catches the main conceptual difference between the two, which
is good. I also agree with the philosophy that it's fine to have unimplemented! in
non-production crates, and agree with the difficulties that we'd actually have trying to implement
that.

When writing Rust, you just have to remember that the following things panic:

• panic!
• todo!
• unimplemented!
• Option.unwrap
• Result.unwrap
• Out-of-bounds slice indexing

Right, my general feeling is:

• panic is obvious that it panics
• unwrap/except are both pretty commonly encountered, so it's easy for code review to have it
  be top of mind that you should validate the assumptions that make this unwrap safe (and we often
  encourage comments explaining why the unwrap is safe in this case.)
• slice indexing is ... complicated. It doesn't come up very often, and if there was an ergonomic way to ban it I think I would
  be in favor of doing so.

That leaves just todo! and unimplemented!. The todo macro was initially proposed because unimplemented is annoying to type, and ever since todo was added to std unimplemented's use has become ever more uncommon, to my mind making it a footgun because it's basically unused nowadays. I don't see the benefit of having something kind of innocuous-looking that people don't use very often that is just a tiny bit of sugar over a direct panic invocation, but obviously I'm in the minority here.

[quodlibetor]

FWIW I didn't expect this to have this much conversation, I very much believe that the bail_unsupported renaming will help, I just didn't expect anyone to care enough to keep unimplemented around -- we use it 16 times in our 150kloc codebase.

[umanwizard]

@quodlibetor I'm still in favor of adding the lint, if there's a reasonable way to ignore it for the DummyCatalog case!

[quodlibetor]

Yeah I mean we could #![allow(clippy::unimplemented)] in that module if people want it.