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

RFC for unused const fn results #2450

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@est31
Copy link
Contributor

est31 commented May 29, 2018

Proposes a lint on side effect free const fn invocations that are not used by the program.

Rendered

An example would be:

const fn add_one(v: u32) -> u32 {
    v + 1
}
fn foo() {
    add_one(2); // WARNING unused result of const fn
}

Thanks go to @Centril and @rkruppe who reviewed the drafts.

The third criterion exists as when params are being moved, the function might invoke the param's `Drop` impl.
If this impl contains side effects, the code might not actually be not dead.
Thus, the lint should require that all moved input params either don't override the default `Drop` impl or use `const Drop` or something like it.
An initial version of the lint could simply require that overriding the `Drop`

This comment has been minimized.

@oli-obk

oli-obk May 29, 2018

Contributor

This needs to be enforced recursively, as otherwise a simple wrapper type around a Drop type would suffice to trick the analysis. Maybe we should just start out with copy types?

This comment has been minimized.

@est31

est31 May 30, 2018

Author Contributor

We might already need a recursive check for UnsafeCell. Or does UnsafeCell inhibit Copy?

This comment has been minimized.

@oli-obk

oli-obk May 30, 2018

Contributor

No, Cell is Copy and contains an UnsafeCell.

This comment has been minimized.

@scottmcm

scottmcm Jun 3, 2018

Member

Is this what TyS::is_freeze is for?

const fn add_one<T: const AddOne>(v: T) {
v.add_one();
}
```

This comment has been minimized.

@fstirlitz

fstirlitz May 30, 2018

Which of the two criteria does this example fall under? Remember () is inhabited.

This comment has been minimized.

@est31

est31 May 30, 2018

Author Contributor

"have all its generic types known" and "have the types of all input parameters contain no mutability". I re-started the numbering for the second group of criteria, but this seems to be confusing, so I might change that.

[summary]: #summary

Add a lint for unused results of `const fn` functions,
if we know for sure that the invocation is dead code.

This comment has been minimized.

@fstirlitz

fstirlitz May 30, 2018

IIRC 'dead code' is synonymous with 'unreachable code'. The invocations in your examples aren't unreachable; it's just that their results are discarded (and since they are free of side effects, they can be optimised away).

@WiSaGaN

This comment has been minimized.

Copy link
Contributor

WiSaGaN commented May 31, 2018

I feel positive in general for this. Several points below:

  • Should we add a way to suppress this warning for a specific function invocation? I can't think of a situation that I really need this, but there may be.

  • Besides, when defining a const fn can we attach attribute of allow_unused to it to suppress this warning in general for that function?

  • Should any thing be done when one add a must_use attribute to a const fn?

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Jun 3, 2018

This sounds great, @est31. I'd much rather piggy-back the lint on the already-happening constification (like slice len just happened, for example) than have an additional must-use-ification.

None known to the RFC author.

But languages that like Rust have a prominent concept of purity/side effect
freedom where you can easily discard results may have such lints.

This comment has been minimized.

@joshtriplett

joshtriplett Jun 4, 2018

Member

This sentence seems awkwardly phrased, with a long middle clause and no commas. Consider rephrasing it for clarity.

@Centril Centril added the T-lang label Jun 11, 2018

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Jun 18, 2018

[I've put off commenting here long enough, it's becoming clear I'll never get the time and energy to write something that's as careful and nuanced as this topic deserves. So I'll just give it my best. Apologies if that winds up being curt or heavy-handed.]

Like @scottmcm I am quite excited about the prospect of side-stepping the must_use-ification and getting better lints for free. However, after some reflection I am not sure whether that actually works, since const fns can diverge and that divergence can be desirable and a sufficient reason for calling. Divergence is discussed in the RFC, but hand-waved away by focusing on the not-obviously-useful case of infinite or long-running loops. Much more important, and harder to explain away, are panics, especially conditional ones (which can't be visible in the function signature).

For example, consider an assert!-like function1 that returns () but panics if some condition isn't true. Complaining about the return value being unused would be pointless, the interesting case is when it doesn't return. To pre-empt proposals to treat () specially, also consider Option::expect or Result::unwrap, which generally return non-unit values and can be useful as an assertion that something is Ok/Some even when that return value is unused. Two examples of that are the docs of Thread::join and I/O functions such as Seek::seek (which can be part of const fn code because Cursor<&[u8]> and other in-memory types implement it).

One can argue whether all of those are good coding style, but even if not, it makes clear that the proposed lint has serious false positives. Furthermore, these false positives are inherent in the approach and can't easily be fixed. Panics are an important tool in Rust, and as const fns encompass more and more of Rust, treating const fns without considering panics is as hopeless as approaching general Rust code without considering panics.

1 assert!() being a macro complicates the matter somewhat, but there's no shortage of functions which do basically the same job.


Now, to preempt some objections:

  • yes, unconditional panics are often visible from the function signature (returning -> !), but that's not very interesting (though it does probably prevent false positives on panic!() and the like)
  • yes, we could and probably should special case () return values (though it would be special casing and hence icky -- I don't think all ZSTs should get this treatment) that would help with some but not all cases
  • yes, this is a bit of an edge case, but we have a policy of very few false positives for rustc lints.
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jun 18, 2018

The RFC OP states that the goal is to "lint on side effect free const fn invocations that are not used by the program". The motivation then clarifies this as

The Rust language already has formalized side effect freedom for functions through the const fn language subset.

Which is true right now, but as @rkruppe correctly asserted, won't be so in the future. Panicking is a side-effect, but still fine for const fn, because panicking inside a const fn aborts the compilation in case it happens at compile-time.

While const fn and side-effect-free functions have a large overlap, they are not the same set.

In rust-lang/rust#51570 I am writing an analysis that figures out whether a call to a const fn should be promotable. Promotability is a subset of side-effect-freedom, because only trivial functions are allowed (that do not do any actual computations, but only destruct/construct objects)

Thus, I suggest we use the same analysis to decide which function calls to lint.

@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

@Aaron1011

This comment has been minimized.

Copy link

Aaron1011 commented Feb 14, 2019

I'm interested in adopting this RFC. What is the first step that I should take to do so?

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 14, 2019

@Aaron1011 Make a new branch of this one in your own fork; you should also try to address @rkruppe's and @oli-obk's comments / concerns in a new RFC.

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.