add new lint: ip_constant
#14878
add new lint: ip_constant
#14878
Conversation
rustbot
added
the
S-waiting-on-review
relaxcn
changed the title
localhost_hardcodeipv4v6_constant_hardcode
Thanks for your comments. It's a mistake, more constants are supported now. For the second point you mentioned, I wrote some test cases for it, but because of the below code in rust-clippy/clippy_lints/src/methods/mod.rs Lines 4725 to 4729 in 1029572 The macro related codes can not make the suggestion be emitted. macro_rules! ipv4_new {
($a:expr, $b:expr, $c:expr, $d:expr) => {
std::net::Ipv4Addr::new($a, $b, $c, $d)
};
}
fn macro_test() {
let _ = ipv4_new!(127, 0, 0, 1);
// no lint
let _ = ipv4_new!(255, 255, 255, 255);
// no lint
let _ = ipv4_new!(0, 0, 0, 0);
// no lint
}I'm not sure it is acceptable or not. |
|
Additional, I changed the lint name from |
|
A name like |
|
Also re the name, we already have |
Yes, agree. |
I wrote a test cases about For example a file const EXTERNAL_CONST_U8_127: u8 = 127;
const EXTERNAL_CONST_U16_0: u16 = 0;When I run Do you know someway to skip testing this file? It had been solved by adding |
relaxcn
changed the title
ipv4v6_constant_hardcodeip_constant
|
No, I meant including a |
Sorry, do you mean this file std::net::Ipv4Addr::new(127, 0, 0, 1)Then using it in ui-test file: let _ = include!("localhost.txt");
// should not lint?I have tried it, there is an error in error: use `std::net::Ipv4Addr::LOCALHOST` instead
--> tests/ui/localhost.txt:1:1
|
LL | std::net::Ipv4Addr::new(127, 0, 0, 1)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::net::Ipv4Addr::LOCALHOST`It's quite surprising to me that the span of the expression can actually point to a specific external file. I used to write static analysis tools mainly for C++ code. In C++, macros are just simple text replacements, and in the final AST, it's basically impossible to trace the replaced code back to the file or location it came from. In fact, due to macro expansion, the positions in the code before and after preprocessing can even change. |
|
How about |
I got an error: error: include macro expected single expression in source
--> tests/ui/localhost.txt:1:4
|
1 | 127, 0, 0, 1
| ^
|
= note: `#[deny(incomplete_include)]` on by default
error[E0061]: this function takes 4 arguments but 1 argument was supplied
--> tests/ui/ip_constant.rs:126:13
|
126 | let _ = std::net::Ipv4Addr::new(include!("localhost.txt"));
| ^^^^^^^^^^^^^^^^^^^^^^^--------------------------- three arguments of type `u8`, `u8`, and `u8` are missing
|
note: associated function defined here
--> /home/ciel/.rustup/toolchains/nightly-2025-05-21-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/net/ip_addr.rs:496:18
|
496 | pub const fn new(a: u8, b: u8, c: u8, d: u8) -> Ipv4Addr {
| ^^^
help: provide the arguments
|
126 - let _ = std::net::Ipv4Addr::new(include!("localhost.txt"));
126 + let _ = std::net::Ipv4Addr::new(127, /* u8 */, /* u8 */, /* u8 */);
|The 127, 0, 0, 1The |
|
When I wrote test case like this: //@error-in-other-file: use `std::net::Ipv4Addr::LOCALHOST` instead
fn external_constant_test() {
let _ = include!("localhost.txt");
// lint in external file `localhost.txt`
}The std::net::Ipv4Addr::new(127, 0, 0, 1)I will get an error when running FAILED TEST: tests/ui/ip_constant.rs
command: rustfix tests/ui/ip_constant.rs
error: failed to apply suggestions for tests/ui/ip_constant.rs with rustfix
cannot apply suggestions for `tests/ui/localhost.txt` since main file is `tests/ui/ip_constant.rs`. Please use `//@no-rustfix` to disable rustfix
Add //@no-rustfix to the test file to ignore rustfix suggestionsSo I separated the test cases that use |
|
Ah, interesting, I didn't know that. Good thing you've got that test down. However, in that case, the lint should not apply |
Yes, you are right. That case should not apply auto-fix. But it's hard to handle I tried to fix this by checking if The |
|
I searched the entire project and there doesn't seem to be any other test cases using the |
|
You can have a look at this lint fn include_macro_test() {
let z = &[2, 3, 5];
let _ = include!("get.txt");
}The z.get(0)Maybe we need to discuss it with more people to get a final solution. |
|
I found the definition of #[stable(feature = "rust1", since = "1.0.0")]
#[rustc_builtin_macro]
#[macro_export]
#[rustc_diagnostic_item = "include_macro"] // useful for external lints
macro_rules! include {
($file:expr $(,)?) => {{ /* compiler built-in */ }};
}We can use the But I don't know how to get the data that before the |
|
Yeah, let's not worry about it right now. I'm starting the FCP. |
Thanks for your work. I haven't had a break yet, there are always some young night owls.🫡 |
There was a problem hiding this comment.
I find the code way much too complicated for what it is trying to achieve. Building a prefix tree by hand can be replaced by letting the compiler do the matching.
Moreover, if the first bytes/words of an IP address are given as constants, there is probably a good chance that the next bytes/words are as well, so we might evaluate them.
I would prefer something like that which is much shorter and gives the same functionality:
if let ExprKind::Path(QPath::TypeRelative(
Ty {
kind: TyKind::Path(QPath::Resolved(_, func_path)),
..
},
p,
)) = func.kind
&& p.ident.name == sym::new
&& let Some(func_def_id) = func_path.res.opt_def_id()
&& matches!(
cx.tcx.get_diagnostic_name(func_def_id),
Some(sym::Ipv4Addr | sym::Ipv6Addr)
)
&& let Some(args) = args
.iter()
.map(|arg| {
if let Some(Constant::Int(constant @ (0 | 1 | 127 | 255))) = ConstEvalCtxt::new(cx).eval(arg) {
u8::try_from(constant).ok()
} else {
None
}
})
.collect::<Option<SmallVec<[u8; 8]>>>()
{
let constant_name = match args.as_slice() {
[0, 0, 0, 0] | [0, 0, 0, 0, 0, 0, 0, 0] => "UNSPECIFIED",
[127, 0, 0, 1] | [0, 0, 0, 0, 0, 0, 0, 1] => "LOCALHOST",
[255, 255, 255, 255] => "BROADCAST",
_ => return,
};
span_lint_and_then(cx, IP_CONSTANT, expr.span, "hand-coded well-known IP address", |diag| {
diag.span_suggestion_verbose(
expr.span.with_lo(p.ident.span.lo()),
"use",
constant_name,
Applicability::MachineApplicable,
);
});
}(you would need to add extern crate smallvec; to clippy_lints/src/lib.rs, but this doesn't cost anything as it is used in clippy_utils already)
This also gives a better suggestion with a visual replacement pattern. Maybe the error message could be tweaked though. Example:
error: hand-coded well-known IP address
--> tests/ui/ip_constant.rs:38:13
|
LL | let _ = std::net::Ipv4Addr::new(127, 0, 0, 1);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use
|
LL - let _ = std::net::Ipv4Addr::new(127, 0, 0, 1);
LL + let _ = std::net::Ipv4Addr::LOCALHOST;
|
Thanks for your remarks. This is indeed a significant improvement. I’ve learned a lot from your suggestions. The code has been modified. |
Closes: #14819
changelog: new lint: [
ip_constant]