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 initial version of const_fn lint #3648

Merged
merged 7 commits into from Jan 29, 2019
Merged

Conversation

phansch
Copy link
Member

@phansch phansch commented Jan 9, 2019

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

TODO:

  • Finish up the docs
  • Fix the ICE

cc #2440

@phansch
Copy link
Member Author

phansch commented Jan 9, 2019

Soo... some questions:

  1. Is this a good name for the lint?
  2. There are two FIXMEs in the tests where is_min_const_fn doesn't detect that the function can be const. I would leave them in to be fixed later?
  3. Did I miss any obvious test cases? I suppose some more complex test cases would be nice.

@phansch phansch changed the title Add initial version of const_fn lint WIP: Add initial version of const_fn lint Jan 9, 2019
@phansch phansch changed the title WIP: Add initial version of const_fn lint Add initial version of const_fn lint Jan 9, 2019
@phansch phansch added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 9, 2019
@phansch
Copy link
Member Author

phansch commented Jan 9, 2019

Lots of CI failures, so I'm going to have to do some more work first before it's ready for review.

@phansch

This comment has been minimized.

@bors

This comment has been minimized.

bors added a commit that referenced this pull request 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

This comment has been minimized.

return;
}
},
FnKind::Method(ident, sig, _vis, attrs) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will also trigger on methods in traits with default bodies

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that matter though? Trait methods with default bodies can't be const anyway , according to the RFC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, what I meant was that the code looks to me like it will suggest to make trait methods with default bodies const fn

phansch added a commit to phansch/rust that referenced this pull request Jan 18, 2019
This is causing issues with a new Clippy lint that will be able to
detect possible const functions.

As per rust-lang/rust-clippy#3648 (comment)
Centril added a commit to Centril/rust that referenced this pull request Jan 19, 2019
Remove delay_span_bug from qualify_min_const_fn

This is causing issues with a new Clippy lint that will be able to
detect possible const functions.

As per rust-lang/rust-clippy#3648 (comment)

r? @oli-obk
@phansch phansch force-pushed the const_fn_lint branch 3 times, most recently from fdc3f46 to cadac5b Compare January 21, 2019 07:41
clippy_lints/src/utils/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/utils/mod.rs Outdated Show resolved Hide resolved
@phansch phansch added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 23, 2019
@phansch
Copy link
Member Author

phansch commented Jan 23, 2019

I think this is ready for another review now

///
/// **Why is this bad?**
///
/// Not using `const` is a missed optimization. Instead of having the function execute at runtime,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not quite true. This optimization will still happen for non-const fn. The major use case is crates forgetting to use const fn will unnecessarily limit their consumers in what they can do. This is similar to forgetting to implement PartialEq or similar things on types where it might be helpful to have that.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 23, 2019

r=me with documentation nit addressed

@bors
Copy link
Collaborator

bors commented Jan 23, 2019

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

@phansch phansch force-pushed the const_fn_lint branch 4 times, most recently from 97653e0 to 0bec822 Compare January 29, 2019 06:00
@phansch
Copy link
Member Author

phansch commented Jan 29, 2019

r=me with documentation nit addressed

oh I missed that comment 👀

@bors r=oli-obk

@bors
Copy link
Collaborator

bors commented Jan 29, 2019

📌 Commit d0d7c5e has been approved by oli-obk

@bors
Copy link
Collaborator

bors commented Jan 29, 2019

⌛ Testing commit d0d7c5e with merge 6b1a2a9...

bors added a commit that referenced this pull request 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
@bors
Copy link
Collaborator

bors commented Jan 29, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 6b1a2a9 to master...

@bors bors merged commit d0d7c5e into rust-lang:master Jan 29, 2019
@phansch phansch deleted the const_fn_lint branch January 29, 2019 20:42
VardhanThigle pushed a commit to jethrogb/rust that referenced this pull request Jan 31, 2019
This is causing issues with a new Clippy lint that will be able to
detect possible const functions.

As per rust-lang/rust-clippy#3648 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants