Skip to content
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

Compile-time checked version of unimplemented!() #1911

Open
jonhoo opened this issue Feb 19, 2017 · 32 comments
Open

Compile-time checked version of unimplemented!() #1911

jonhoo opened this issue Feb 19, 2017 · 32 comments
Labels
T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Feb 19, 2017

We all know and love the unimplemented!() macro for keeping track of things we haven't yet gotten around to. However, it is only checked at runtime if a certain code path is encountered. This is fine in many cases, but when writing new code from scratch (especially when porting), refactoring large amounts of code, or building comprehensive new features, you often have segments of code that you haven't implemented yet, but you know you'll have to.

At least for me, it is common to write a chunk of code, but leaving out some annoying edge cases, or maybe logic (like generating a unique identifier) that I want to deal with alter. I know that I will have to complete that block of code before the program does something useful, but I want to do it later. This can be either because I want to complete the main logical flow of the code first, because I haven't figured out how to do that part yet, or simply because I want to see if the rest of my code compiles correctly.

It would be extremely useful to have a variant of unimplemented!() that would generate a compile-time error if present in my code. Something like incomplete!(). However, it should have some properties that I believe mean it can't be implemented without some help from the compiler:

  1. It should be usable as an expression of any type (similar to ! for unimplemented!())
  2. It should only error if the program compiles correctly. Or rather, it should not prevent any other errors from being printed. In essence, all passes of the compiler (including the borrow checker) should run despite the presence of incomplete!().
  3. It should not emit warnings about dead code or unused variables following it (which using unimplemented!() does).

The closest I've been able to come up with on my own is the following

macro_rules! incomplete {
    () => {{
        #[deny(non_snake_case)]
        let _mustBeImplemented = ();
        loop {}
    }}
}

This will only error out after all compiler passes (2), and since it returns !, it is sort of usable in place of any expression (1). However, it does not fit (3), because the infinite loop makes the compiler believe that all code following it is dead. Furthermore, the error it produces is obviously not appropriate for what the macro's intent is.

NOTE: originally posted as rust-lang/rust#39930, but moved here following @sanmai-NL's suggestion.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 22, 2017

@cuviper
Copy link
Member

cuviper commented Feb 22, 2017

I feel like there might be a misunderstanding of having type !. This marks something that diverges, so execution is statically known not to proceed beyond this point. A function -> ! doesn't return this type, rather it doesn't return at all. This is how the compiler knows that all of the following code is dead.

Your existing incomplete!() might work better for you if you truly make it return anything, like:

macro_rules! incomplete {
    () => {{
        #[deny(non_snake_case)]
        let _mustBeImplemented = ();
        unsafe { ::std::mem::uninitialized() }
    }}
}

Another idea instead of the non_snake_case lint is to push it even later into a link error. e.g. You could declare and call an FFI function you_forgot_to_implement_something(). This will let your code fully pass things like cargo check, but the drawback is that the link error won't be localized to its use.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 22, 2017

@cuviper Ah, no, I'm aware of the semantic meaning of -> !, I just didn't think of the

unsafe { ::std::mem::uninitialized() }

trick, and used ! as a substitute (even though I know it technically means something completely different). That's neat. Moving it all the way to linking would be nice, but I think keeping localization information is actually sufficiently important that it wouldn't be a viable option.

@cuviper
Copy link
Member

cuviper commented Feb 22, 2017

I realized that has a type-inference problem if used in statement context, incomplete!(); without any assignment. Here's what I came up with, a little less pretty:

macro_rules! incomplete {
    ($ty:ty) => {{
        #[forbid(non_snake_case)]
        let _mustBeImplemented = ();
        unsafe { ::std::mem::uninitialized::<$ty>() }
    }};
    () => { incomplete!(_) }
}

fn main(){
    let _x: () = incomplete!(); // inferred type
    incomplete!(()); // explicit type `()`
}

Also, I think forbid is needed instead of deny, so the error doesn't get squashed if compiled as a dependency of some other crate.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 22, 2017

Oh, yeah, good catch. forbid vs deny I'm slightly less worried about, because it's unlikely you would push a crate in a state where it contains an incomplete!(), but you're right that this could happen if, say, you had a local dependent crate.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 22, 2017

I've also been thinking about whether there's a better way to have the displayed error be more relevant (obviously, having this be a "true" compiler error would be better). I was hoping to use forbid(unknown_lints), but unfortunately that doesn't seem to work correctly. unreachable-code also seems somewhat appropriate, and produces the cleanest message with this:

macro_rules! incomplete {
    ($ty:ty) => {{
        if false {
            loop{};
            #[deny(unreachable_code)]
            () // must be implemented
        }
        unsafe { ::std::mem::uninitialized::<$ty>() }
    }};
    () => { incomplete!(_) }
}

fn main() {
    let _x: () = incomplete!(); // inferred type
    incomplete!(()); // explicit type `()`
}

namely

warning: unreachable expression, #[warn(unreachable_code)] on by default
  --> x.rs:6:13
   |
6  |             () // must be implemented
   |             ^^
...
14 |     let _x: () = incomplete!(); // inferred type
   |                  ------------- in this macro invocation

warning: unreachable expression, #[warn(unreachable_code)] on by default
  --> x.rs:6:13
   |
6  |             () // must be implemented
   |             ^^
...
15 |     incomplete!(()); // explicit type `()`
   |     ---------------- in this macro invocation

though for some reason the warning doesn't seem to be turned into an error (neither does using dead_code instead)...

@cuviper
Copy link
Member

cuviper commented Feb 22, 2017

Hmm, the lint controls don't seem to work on statements -- only on items? Here's a way:

        #[forbid(unused_must_use)]
        fn _incomplete() { Err::<(), ()>(()); }

But it gets an extra note about where the lint level was set:

error: unused result which must be used
  --> <anon>:4:28
   |
4  |         fn _incomplete() { Err::<(), ()>(()); }
   |                            ^^^^^^^^^^^^^^^^^^
...
11 |     let _x: () = incomplete!(); // inferred type
   |                  ------------- in this macro invocation
   |
note: lint level defined here
  --> <anon>:3:18
   |
3  |         #[forbid(unused_must_use)]
   |                  ^^^^^^^^^^^^^^^
...
11 |     let _x: () = incomplete!(); // inferred type
   |                  ------------- in this macro invocation

Maybe there's a lint to trigger that's default deny/forbid.

@cuviper
Copy link
Member

cuviper commented Feb 23, 2017

Good enough?

        fn _incomplete() { '_: loop {} }
error: invalid label name `'_`, #[deny(lifetime_underscore)] on by default
  --> <anon>:3:28
   |
3  |         fn _incomplete() { '_: loop {} }
   |                            ^^
...
10 |     let _x: () = incomplete!(); // inferred type
   |                  ------------- in this macro invocation
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #36892 <https://github.com/rust-lang/rust/issues/36892>

Still hacky to abuse lints, of course -- not as good as it could be as a real compiler macro.

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 23, 2017

That last one is pretty good. I liked the unreachable code one primarily because the message is very clear, highlighting the () // must be implemented, but being able to get rid of the note about the linting is nice. Compiler support would be best though, you're right. Hence this issue ;)

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 23, 2017

FWIW, I made a crate: https://github.com/jonhoo/incomplete.
It's still not as good as a compiler-supported variant, but it's a start.

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 25, 2017

Actually, come to think of it, maybe all I really want is a lint like missing_docs, so that I can choose

#![warn(unimplemented)]

or

#![deny(unimplemented)]

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 30, 2017

I noticed that the compile_error! macro in #1695 just landed. Unfortunately, that terminates compilation too early, and so doesn't actually fit well for this issue either..

@sanmai-NL
Copy link

fn my_fn() -> usize {
    unimplemented()!;
    0
}

produces:

warning: unreachable expression

Does this suffice to point out left-over unimplemented! invocations at compile time?

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 19, 2017

@sanmai-NL You mean unimplemented!(), right? I don't think that solution is adequate for what I had in mind, for a couple of reasons:

  • it's not an error if there are no other errors
  • it'll give me lots of additional errors for any statements below the unimplemented!()

@Centril Centril added T-libs-api Relevant to the library API team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC. labels Feb 23, 2018
@Centril
Copy link
Contributor

Centril commented Oct 8, 2018

I believe this is either unimplemented!() itself when executed in CTFE, which is tracked in rust-lang/rust#51999 or it is compile_error!. So I'll close in favor of those.

@Centril Centril closed this as completed Oct 8, 2018
@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 8, 2018

@Centril I don't think that's quite accurate. compile_error is close, but it fails too early (#1911 (comment)). Specifically, the request here is to have a device that will only prevent compilation if there are no other compilation errors. The idea is that I want something that reminds me to fill in a section later, but first fix all the other compilation errors. This is very handy when refactoring a somewhat large code-base. For example, consider the case where I change a function to return a Result where it wasn't previously. In of the callers, handling the Err case will be more complicated, so I mark it as incomplete! (or whatever we want to call it). I can then continue going through all the other parts of the code that need to be changed, and only when everything else has been fixed does the compiler remind me to go back and remind me that I still need to fill in that case.

@Centril
Copy link
Contributor

Centril commented Oct 8, 2018

@jonhoo so... the only difference between incomplete!() and const { panic!(...) } is that the latter will unconditionally cause a compilation error while the former will only do it if there are zero errors otherwise? That distinction feels somewhat niche...? (reopening anyways... but I'd recommend fleshing it out on IRLO to get some more eyes on it...)

@Centril Centril reopened this Oct 8, 2018
@burdges
Copy link

burdges commented Oct 8, 2018

Isn't the goal here #![deny(unimplemented)] more than incomplete!() now? You could kinda do that with grep -r unimplemented . right?

@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 8, 2018

@burdges No, there are reasonable cases where I want to run my code when it still has unimplemented! in it. incomplete! on the other hand are things I know I have to do before running, but want to defer writing while I handle other (more fundamental) errors. In part because those errors may cause me to further change the design such that the incomplete! block would also change.

@Centril yes, it's perhaps a little niche, but it would be super handy when doing bigger refactoring. If I'm not mistaken, const { panic!() } will fail to compile before borrow checking?

@Centril
Copy link
Contributor

Centril commented Oct 8, 2018

const { panic!() } will fail to compile before borrow checking?

cc @oli-obk re. this question

@oli-obk
Copy link
Contributor

oli-obk commented Oct 8, 2018

No, after checking. all const eval happens after borrow checking and Mir computation for that item. other items may be in different states though.

On topic of this feature request. this is doable in clippy and feels niche enough to me to not be in the compiler

@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 8, 2018

@oli-obk If this were in clippy it wouldn't actually be useful, because I don't think most developers regularly run clippy. You do however regularly run the compiler. It's an interesting point that const { panic!() } runs at the very end. it might be that that would actually be how to implement this macro!

@Centril
Copy link
Contributor

Centril commented Oct 8, 2018

(note: the const { .. } bit is fictive and not actually proposed by any RFC, but you can use a const item for that instead I think...)

@oli-obk
Copy link
Contributor

oli-obk commented Oct 9, 2018

It only runs in the end for the containing item. you might error out immediately on that and never see your other errors.

I don't see how being in clippy would make this less useful. Rls will show it, and if your workflow uses this kind of check, then your workflow will just contain clippy from now on 😃

That said. This kind of scheme does not fit into queries very well. Any improvements for queries will likely end up reporting these errors earlier again. I do think the grep method is the most reliable one, or doing it in clippy, which doesn't need to uphold the query system

As a hack you could create this macro on nightly yourself by using broken inline assembly.

@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 9, 2018

Rls will only show it if clippy is enabled :) I'm working on a 50k LOC codebase that wasn't originally written with clippy enabled, so it's not actually feasible to have clippy running all the time (at least not yet) because it generates too much noise. You might be right that for most projects a clippy lint is the way to go, but what would the macro then ever consist of? It would have to also be possible to compile outside of clippy?

@burdges
Copy link

burdges commented Oct 9, 2018

I quite like the idea of grep -r unimplemented . coupled with reminder text, so

unimplemented!() // TODO: ...

which permits seeing reminders for only a fragment of the tree too.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 9, 2018

I'm working on a 50k LOC codebase that wasn't originally written with clippy enabled, so it's not actually feasible to have clippy running all the time (at least not yet) because it generates too much noise.

You can run it all the time and just enable the lints you feel confident about. E.g. leave all lints disabled and just enable the one about incomplete!()

You might be right that for most projects a clippy lint is the way to go, but what would the macro then ever consist of?

It would just forward to unimplemented and the lint would check for the name.

It would have to also be possible to compile outside of clippy?

yes indeed. Without clippy it would just be the same as writing unimplemented!

Even easier would be to simply find all unimplemented!("incomplete"), which would work on regular code without an additional macro.

@czipperz
Copy link

Could this be implemented by linking to an external function that doesn't exist so the failure is pushed back until an exe is produced? See the panic-never crate

@afetisov
Copy link

Personally I would want a stronger thing which likely cannot be implemented as a library macro. Instead of just delaying the check to the end of the compiler pipeline, I would like incomplete! to act as a compile_error! which fires as long as the code survives dead code elimination. I.e. there is a separate compiler pass check_incomplete which runs at some unspecified time after the last dead code elimination pass (possibly at the codegen stage). If check_incomplete finds any instance of incomplete! in the code, it produces a compiler error at its position.

Possibly incomplete! could expand into a call to an opaque generic function of the form fn magic<T>() -> T, with a later pass checking for calls to this function.

This is similar to unimplemented! in that you get an error only if the code is (potentially) reachable at runtime, but at the same time it produces a static error instead of a delayed dynamic one.

@bjorn3
Copy link
Member

bjorn3 commented Jul 29, 2021

Dead code elimination for whole functions is done as part of the linker. (--gc-sections) Dead code elimination for parts of a function is done as optimization that may not run at all in debug mode and may have different effectiveness between compiler versions and backends. (Currently only LLVM is officially supported and shipped with rustc, but (incomplete) Cranelift (by me) and GCC backends exists too.) This means that such an incomplete!() macro may trigger or not depending on the compiler version, which would be a breaking change not allowed by our stability policy. You could already emulate this at the cost of your code only working in release mode and possibly breaking in the future by calling a non-existent extern function from inside the incomplete!() macro. You will get a linker error if the call was not eliminated.

@afetisov
Copy link

Yes, I don't expect this will ever be in the language. Still, it would be a cool thing to have. Maybe it could be done as a Clippy lint? It could be a check which traces all function calls starting from the public API and errors out on the calls to the macro.

@bjorn3
Copy link
Member

bjorn3 commented Jul 29, 2021

That could work, though it would need to look at all roots, including trait object creation and function pointer creation. It would also still fire for trivially unreachable things like if 1 + 1 != 2 { incomplete!() }, but that may be acceptable depending on your use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

9 participants