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

Add a lint for unused const fn results #50805

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
10 participants
@est31
Copy link
Contributor

est31 commented May 16, 2018

Implements RFC 2450.
This PR adds a lint that checks for invocations of functions which are const fn where the result is discarded.

This is dead code as const fn is free of side effects.

Implements my suggestion from #48926 (comment)

Important gotcha: this is not a "unused constant expression" lint but a superset of such a lint: we also lint for const fn invocations with non-constant parameters. Invocations in such cases would still be side effect free... well maybe except for drop impls but this is as close as we can get.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented May 16, 2018

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented May 16, 2018

This is dead code as const fn is free of side effects.

That's not quite true in the sense relevant for this lint. const fns are very restricted, but e.g. handling &mut references and writing through them works in miri and should work. For example, mem::replace could be a const fn in the near future, and while mem::replace(some_mut_ref, some_value); is probably better written as *some_mut_ref = some_value;, it's not dead code.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented May 16, 2018

I believe all new lints require RFCs.

@est31

This comment has been minimized.

Copy link
Contributor Author

est31 commented May 16, 2018

@rkruppe good point. What about changing to let the lint fire in the following case then:

  • the function invoked is const fn (current requirement) and
  • the types of all input params lack exterior and interior mutability so there are no mut references and no std::UnsafeCell anywhere in the type.

This would make the lint watertight, modulo UB (where unsafe code has interior mutability and does NOT use std::UnsafeCell).

@steveklabnik I'd argue that this makes the dead code lint smarter so is more of an extension of currently present lints but shrug...

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented May 16, 2018

I agree this is a new lint. It's not one of the usual classical data flow analyses or CFG reachability checks or narrow annotation (edit: nvm we're talking dead code, not unused_must_use), but something that aggressively reasons about possible side-effects from many user-defined functions.

Furthermore, even if we'd agree that this is "just an extension of the dead code lint", it'll generate a lot of completely new warning and thus warrants extra scrutiny.

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented May 16, 2018

the types of all input params lack exterior and interior mutability so there are no mut references and no std::UnsafeCell anywhere in the type.

I don't think that's really sound either. We don't yet have anything like T: const Trait but clearly we'll need some way to use trait bounds in const fns, and then self-consuming methods can perform mutation when called on types that contain mutable references, without that mutable reference being known at the call site. Silly example:

trait Foo {
    fn foo(self);
}

struct Bar<'a>(&'a mut i32);

impl<'a> Foo for Bar<'a> {
    fn foo(mut self) {
        self.0 = 42;
    }
}

const fn foo<T: const Foo>(x: T) {
    x.foo(); // lint would presumably fire, but this does something
}
@est31

This comment has been minimized.

Copy link
Contributor Author

est31 commented May 16, 2018

@rkruppe that type encapsulates something mutable and thus has some kind of mutability itself (no idea how it's called whether interior or exterior or some other kind but it is mutability).

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented May 16, 2018

Sure but there's no way for the lint to know that when analyzing the body of foo, since it is generic.

@est31

This comment has been minimized.

Copy link
Contributor Author

est31 commented May 17, 2018

@rkruppe I see what you are meaning. Hmmm tough problem. Any ideas to fix the lint, anyone? Leaving this open for further ideas.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 19, 2018

☔️ The latest upstream changes (presumably #50763) made this pull request unmergeable. Please resolve the merge conflicts.

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented May 28, 2018

Ping from triage @pnkfelix! This PR needs your review.

@est31

This comment has been minimized.

Copy link
Contributor Author

est31 commented May 28, 2018

I'm going to close this PR instead. Sadly I have no idea how to salvage it (see above) even though I hope there is a way.

@est31

This comment has been minimized.

Copy link
Contributor Author

est31 commented May 29, 2018

@rkruppe what about letting the lint only run on invocations where all the generic types are known? This would emit lints for { String::from("hi"); } and friends which was the original motivation.

@est31 est31 reopened this May 29, 2018

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented May 29, 2018

That ought to work, maybe with some minor (surmountable) caveats that I'm missing at the moment.

cc @Centril @RalfJung who've also thought about what guarantees const fn exactly provides and doesn't provide.

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented May 29, 2018

Meant to ping @RalfJung, not @ralfj, sorry!

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented May 29, 2018

@rkruppe SGTM 👍 Traits which only have const associated items should also work whether or not their types are fully known?

I think such a lint along the lines outlined by @est31 in their last comment jives well with my modified notion of Ralf's notion. The modified notion being that execution of const fn at runtime must correspond to execution of const fn in CTFE for the same arguments (up to interior mutability and &mut). This includes that if CTFE returns ⊥ so must runtime execution.

@est31

This comment has been minimized.

Copy link
Contributor Author

est31 commented May 29, 2018

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented May 30, 2018

The modified notion being that execution of const fn at runtime must correspond to execution of const fn in CTFE for the same arguments (up to interior mutability and &mut). This includes that if CTFE returns ⊥ so must runtime execution.

I don't see the connection between this strong form of CTFE correctness and the lint, could you elaborate?

I don't think such a guarantee is realistic. CTFE will do checks that we can never perform at run-time, such that e.g. offset will trigger an error when it leaves the bounds of the allocation. Notice that (as far as I am concerned) CTFE correctness is a property of the CTFE interpreter (aka miri) that has to apply to all code, even unsafe, so the fact that offset is unsafe does not make a difference. Put differently, it's a property of the interpreter and has nothing to do with a type system that rules out certain code.


Off the top of my head, I cannot think of any way in which calling a const fn with just plain values and mutable references could be useful. But anyway this is just a lint so I guess if we're wrong, people will tell us?

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented May 30, 2018

I don't see the connection between this strong form of CTFE correctness and the lint, could you elaborate?

I'm saying that the lint works with that definition, i.e: there is no problem.


I'll reply here for now, but let's continue the conversation about guarantees elsewhere after that.
The extra guarantee I'm talking about, that interp_miri(expr) = ⊥ -> interp_runtime(expr) = ⊥, is not checked by the type system. It is akin to guarantees about unsafe { .. } in normal functions in that it can't be violated directly inside const fn without an unsafe { .. } block. But in such a block in const fn, the proof obligation, that const fn ensures this property, still exists and must be upheld by the author of the function, even tho it is not checked by the compiler. What I'm talking about is not a property of the interpreter. You could perhaps make it one by having an optional --rts-miri flag which embeds miri in the binary.

@TimNN

This comment has been minimized.

Copy link
Contributor

TimNN commented Jun 12, 2018

Marking as blocked on the RFC.

@TimNN TimNN added S-blocked and removed S-waiting-on-author labels Jun 12, 2018

@est31

This comment has been minimized.

Copy link
Contributor Author

est31 commented Jul 3, 2018

The period of my contributions to Rust upstream has reached an end. Thus I'm unable to continue my work on this. I still think something like this is a great addition. I urge anyone interested in this change to adopt and continue it from here on. Thanks.

@est31 est31 closed this Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.