Skip to content

[missing_const_for_fn]: fix suggestions for fn with abi that requires const_extern_fn feature #13037

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 5, 2024
Merged

[missing_const_for_fn]: fix suggestions for fn with abi that requires const_extern_fn feature #13037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clippy_config/src/msrvs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ msrv_aliases! {
1,68,0 { PATH_MAIN_SEPARATOR_STR }
1,65,0 { LET_ELSE, POINTER_CAST_CONSTNESS }
1,63,0 { CLONE_INTO }
1,62,0 { BOOL_THEN_SOME, DEFAULT_ENUM_ATTRIBUTE }
1,62,0 { BOOL_THEN_SOME, DEFAULT_ENUM_ATTRIBUTE, CONST_EXTERN_FN }
1,59,0 { THREAD_LOCAL_INITIALIZER_CAN_BE_MADE_CONST }
1,58,0 { FORMAT_ARGS_CAPTURE, PATTERN_TRAIT_CHAR_ARRAY, CONST_RAW_PTR_DEREF }
1,56,0 { CONST_FN_UNION }
Expand Down
16 changes: 15 additions & 1 deletion clippy_lints/src/missing_const_for_fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use rustc_middle::lint::in_external_macro;
use rustc_session::impl_lint_pass;
use rustc_span::def_id::LocalDefId;
use rustc_span::Span;
use rustc_target::spec::abi::Abi;

declare_clippy_lint! {
/// ### What it does
Expand Down Expand Up @@ -115,7 +116,10 @@ impl<'tcx> LateLintPass<'tcx> for MissingConstForFn {
.iter()
.any(|param| matches!(param.kind, GenericParamKind::Const { .. }));

if already_const(header) || has_const_generic_params {
if already_const(header)
|| has_const_generic_params
|| !could_be_const_with_abi(cx, &self.msrv, header.abi)
{
return;
}
},
Expand Down Expand Up @@ -171,3 +175,13 @@ impl<'tcx> LateLintPass<'tcx> for MissingConstForFn {
fn already_const(header: hir::FnHeader) -> bool {
header.constness == Constness::Const
}

fn could_be_const_with_abi(cx: &LateContext<'_>, msrv: &Msrv, abi: Abi) -> bool {
match abi {
Abi::Rust => true,
// `const extern "C"` was stablized after 1.62.0
Abi::C { unwind: false } => msrv.meets(msrvs::CONST_EXTERN_FN),
// Rest ABIs are still unstable and need the `const_extern_fn` feature enabled.
_ => cx.tcx.features().const_extern_fn,
}
}
9 changes: 9 additions & 0 deletions tests/ui/missing_const_for_fn/cant_be_const.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,4 +180,13 @@ mod msrv {
unsafe { *self.1 as usize }
}
}

#[clippy::msrv = "1.61"]
extern "C" fn c() {}
}

mod with_extern {
extern "C-unwind" fn c_unwind() {}
extern "system" fn system() {}
extern "system-unwind" fn system_unwind() {}
Comment on lines +189 to +191
Copy link
Member Author

@J-ZhengLi J-ZhengLi Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm a little bit unsure should we report these with help messages like "if your are on nightly toolchain, enable const_extern_fn feature with #![feature(const_extern_fn)]" instead of just skip them, it surely will make some noise tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you really want to add that and can detect that a nightly toolchain is a build requirement (for example via a toolchain file), I'm not opposed to it.

Copy link
Member Author

@J-ZhengLi J-ZhengLi Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, that reminds me I probably (???) could use this cx.sess().is_nightly_build to check that but I assume it will be useless in our test cases, since it will be true in all of them...

Well, I'll keep this in mind for future reference, maybe we could have some attribute like #[clippy::toolchain = "nightly"] in the future if that function turns out to be useful, but for now this might be fine (I think)~

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, the problem with is_nightly_build is that it's true when compiled on nightly. This is the wrong criteria, the code might still accept compiling on stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh right... I see, that'll made the users having to force nightly toolchain if they only follow the suggestion once...

hmmm, I understand why you mentioned "via a toolchain file" now, as it seems like the only sensible way here. But doing so would require clippy to have some basic de-serializer written to match that file, and I saw there's a security flaw with that file on this rustup issue, so I'm expecting rustup to make some changes regarding that file, meaning that we'll have to change them too~

For these reasons I think it might not worth the efforts, and lets just call this perfect imperfection :D

}
15 changes: 15 additions & 0 deletions tests/ui/missing_const_for_fn/could_be_const.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,21 @@ mod msrv {
let bar = Bar { val: 1 };
let _ = unsafe { bar.val };
}

#[clippy::msrv = "1.62"]
mod with_extern {
const extern "C" fn c() {}
//~^ ERROR: this could be a `const fn`

#[rustfmt::skip]
const extern fn implicit_c() {}
//~^ ERROR: this could be a `const fn`

// any item functions in extern block won't trigger this lint
extern "C" {
fn c_in_block();
}
}
}

mod issue12677 {
Expand Down
15 changes: 15 additions & 0 deletions tests/ui/missing_const_for_fn/could_be_const.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,21 @@ mod msrv {
let bar = Bar { val: 1 };
let _ = unsafe { bar.val };
}

#[clippy::msrv = "1.62"]
mod with_extern {
extern "C" fn c() {}
//~^ ERROR: this could be a `const fn`

#[rustfmt::skip]
extern fn implicit_c() {}
//~^ ERROR: this could be a `const fn`

// any item functions in extern block won't trigger this lint
extern "C" {
fn c_in_block();
}
}
}

mod issue12677 {
Expand Down
30 changes: 26 additions & 4 deletions tests/ui/missing_const_for_fn/could_be_const.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,29 @@ LL | const fn union_access_can_be_const() {
| +++++

error: this could be a `const fn`
--> tests/ui/missing_const_for_fn/could_be_const.rs:152:9
--> tests/ui/missing_const_for_fn/could_be_const.rs:146:9
|
LL | extern "C" fn c() {}
| ^^^^^^^^^^^^^^^^^^^^
|
help: make the function `const`
|
LL | const extern "C" fn c() {}
| +++++

error: this could be a `const fn`
--> tests/ui/missing_const_for_fn/could_be_const.rs:150:9
|
LL | extern fn implicit_c() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: make the function `const`
|
LL | const extern fn implicit_c() {}
| +++++

error: this could be a `const fn`
--> tests/ui/missing_const_for_fn/could_be_const.rs:167:9
|
LL | / pub fn new(strings: Vec<String>) -> Self {
LL | | Self { strings }
Expand All @@ -213,7 +235,7 @@ LL | pub const fn new(strings: Vec<String>) -> Self {
| +++++

error: this could be a `const fn`
--> tests/ui/missing_const_for_fn/could_be_const.rs:157:9
--> tests/ui/missing_const_for_fn/could_be_const.rs:172:9
|
LL | / pub fn empty() -> Self {
LL | | Self { strings: Vec::new() }
Expand All @@ -226,7 +248,7 @@ LL | pub const fn empty() -> Self {
| +++++

error: this could be a `const fn`
--> tests/ui/missing_const_for_fn/could_be_const.rs:168:9
--> tests/ui/missing_const_for_fn/could_be_const.rs:183:9
|
LL | / pub fn new(text: String) -> Self {
LL | | let vec = Vec::new();
Expand All @@ -239,5 +261,5 @@ help: make the function `const`
LL | pub const fn new(text: String) -> Self {
| +++++

error: aborting due to 17 previous errors
error: aborting due to 19 previous errors

14 changes: 14 additions & 0 deletions tests/ui/missing_const_for_fn/could_be_const_with_const_extern_fn.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#![warn(clippy::missing_const_for_fn)]
#![allow(unsupported_calling_conventions)]
#![feature(const_extern_fn)]

const extern "C-unwind" fn c_unwind() {}
//~^ ERROR: this could be a `const fn`
const extern "system" fn system() {}
//~^ ERROR: this could be a `const fn`
const extern "system-unwind" fn system_unwind() {}
//~^ ERROR: this could be a `const fn`
pub const extern "stdcall" fn std_call() {}
//~^ ERROR: this could be a `const fn`
pub const extern "stdcall-unwind" fn std_call_unwind() {}
//~^ ERROR: this could be a `const fn`
14 changes: 14 additions & 0 deletions tests/ui/missing_const_for_fn/could_be_const_with_const_extern_fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#![warn(clippy::missing_const_for_fn)]
#![allow(unsupported_calling_conventions)]
#![feature(const_extern_fn)]

extern "C-unwind" fn c_unwind() {}
//~^ ERROR: this could be a `const fn`
extern "system" fn system() {}
//~^ ERROR: this could be a `const fn`
extern "system-unwind" fn system_unwind() {}
//~^ ERROR: this could be a `const fn`
pub extern "stdcall" fn std_call() {}
//~^ ERROR: this could be a `const fn`
pub extern "stdcall-unwind" fn std_call_unwind() {}
//~^ ERROR: this could be a `const fn`
59 changes: 59 additions & 0 deletions tests/ui/missing_const_for_fn/could_be_const_with_const_extern_fn.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
error: this could be a `const fn`
--> tests/ui/missing_const_for_fn/could_be_const_with_const_extern_fn.rs:5:1
|
LL | extern "C-unwind" fn c_unwind() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::missing-const-for-fn` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::missing_const_for_fn)]`
help: make the function `const`
|
LL | const extern "C-unwind" fn c_unwind() {}
| +++++

error: this could be a `const fn`
--> tests/ui/missing_const_for_fn/could_be_const_with_const_extern_fn.rs:7:1
|
LL | extern "system" fn system() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: make the function `const`
|
LL | const extern "system" fn system() {}
| +++++

error: this could be a `const fn`
--> tests/ui/missing_const_for_fn/could_be_const_with_const_extern_fn.rs:9:1
|
LL | extern "system-unwind" fn system_unwind() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: make the function `const`
|
LL | const extern "system-unwind" fn system_unwind() {}
| +++++

error: this could be a `const fn`
--> tests/ui/missing_const_for_fn/could_be_const_with_const_extern_fn.rs:11:1
|
LL | pub extern "stdcall" fn std_call() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: make the function `const`
|
LL | pub const extern "stdcall" fn std_call() {}
| +++++

error: this could be a `const fn`
--> tests/ui/missing_const_for_fn/could_be_const_with_const_extern_fn.rs:13:1
|
LL | pub extern "stdcall-unwind" fn std_call_unwind() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: make the function `const`
|
LL | pub const extern "stdcall-unwind" fn std_call_unwind() {}
| +++++

error: aborting due to 5 previous errors