Skip to content

new lint: find functions that can be "const fn" #2440

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

Closed
matthiaskrgr opened this issue Feb 6, 2018 · 6 comments
Closed

new lint: find functions that can be "const fn" #2440

matthiaskrgr opened this issue Feb 6, 2018 · 6 comments
Assignees
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-suggestion Lint: Improving, adding or fixing lint suggestions T-MIR Type: This lint will require working with the MIR

Comments

@matthiaskrgr
Copy link
Member

Maybe we can automatically find simple functions that can be const fn

This was suggested during FOSDEM talk on rusts const evaluation: https://youtu.be/Zz863ksXRhA?t=1102
I didn't see a ticket about this yet so here we go.

@oli-obk oli-obk added E-hard Call for participation: This a hard problem and requires more experience or effort to work on A-lint Area: New lints T-MIR Type: This lint will require working with the MIR L-suggestion Lint: Improving, adding or fixing lint suggestions labels Feb 6, 2018
@sunjay
Copy link
Member

sunjay commented Dec 10, 2018

Yes! This would be great! I would be happy with it even if it just started as a simple lint that encouraged people to make code like this const:

impl Game {
    // Could be const
    pub fn new() -> Self {
        Self {
            tiles: Default::default(),
            current_piece: Piece::X,
            winner: None,
        }
    }
}

Of course, it could always grow from there to become more complete. This would just give people a start into using const. Not having const on simple associated functions like this makes it really hard to use these in const declarations in your code. If a library you use doesn't have this, you get completely stuck.

That's why I think it's really important to start getting people in the habit of putting const on functions when they can. I think a clippy lint will go a long way.

@oli-obk oli-obk added E-medium Call for participation: Medium difficulty level problem and requires some initial experience. and removed E-hard Call for participation: This a hard problem and requires more experience or effort to work on labels Dec 11, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Dec 11, 2018

So... implementing this lint has become significantly easier lately: Just call https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_min_const_fn.rs#L11 and check the result. There are some false positives, but we can't check them right now outside the compiler (because the check emits errors immediately instead of reporting a Result, so if we run it on normal functions we get loads of errors emitted).

phansch added a commit to phansch/rust that referenced this issue Jan 4, 2019
Trying to write a `const_fn` lint for Clippy. @oli-obk suggested
[here][link] to use the `is_min_const_fn` function from the
`qualify_min_const_fn` module. However, the module is currently private
and this commit makes it public.

I lack any historical knowledge of the development of the `const_fn`
feature, so I'm not sure if it was private on purpose or not. fwiw, all
modules are already public except `qualify_min_const_fn`.

[link]: rust-lang/rust-clippy#2440 (comment)
@phansch
Copy link
Member

phansch commented Jan 5, 2019

Going to give this a try over the next couple of days. First step is getting rust-lang/rust#57342 merged.

kennytm added a commit to kennytm/rust that referenced this issue Jan 5, 2019
librustc_mir: Make qualify_min_const_fn module public

Trying to write a `const_fn` lint for Clippy. @oli-obk suggested
[here][link] to use the `is_min_const_fn` function from the
`qualify_min_const_fn` module. However, the module is currently private
and this commit makes it public.

I lack any historical knowledge of the development of the `const_fn`
feature, so I'm not sure if it was private on purpose or not. fwiw, all
modules are already public except `qualify_min_const_fn`.

r? @oli-obk

[link]: rust-lang/rust-clippy#2440 (comment)
@phansch phansch self-assigned this Jan 8, 2019
bors added a commit that referenced this issue Jan 11, 2019
Add initial version of const_fn lint

This adds an initial version of a lint that can tell if a function could be `const`.

TODO:

- [x] Finish up the docs
- [ ] Fix the ICE

cc #2440
bors added a commit that referenced this issue Jan 29, 2019
Add initial version of const_fn lint

This adds an initial version of a lint that can tell if a function could be `const`.

TODO:

- [x] Finish up the docs
- [x] Fix the ICE

cc #2440
@sunjay
Copy link
Member

sunjay commented Mar 7, 2019

With @phansch's PR merged is this issue completely implemented?

@phansch
Copy link
Member

phansch commented Mar 7, 2019

@sunjay Yeah, I guess we can handle bugs and further improvements via new issues, so I'm going to close this one, thanks!

@phansch phansch closed this as completed Mar 7, 2019
@sunjay
Copy link
Member

sunjay commented Mar 7, 2019

Awesome! Thank you for your work 😁🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-suggestion Lint: Improving, adding or fixing lint suggestions T-MIR Type: This lint will require working with the MIR
Projects
None yet
Development

No branches or pull requests

4 participants