Uplift temporary-cstring-as-ptr lint from clippy into rustc
#75671
Conversation
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit f7da096488c6fd9d4be7c480affa26d9f7b80a02 with merge ca7ac508d257828878fb0e7cade1ad46381056f8... |
|
Sgtm, modulo CI passing, perf being clean and we'll need an MCP so the entire compiler team is informed about this. Can you open an MCP? It just needs a sentence or two and maybe an example on what is now being linted. |
nathanwhit
force-pushed
the
cstring-temp-lint
branch
from
f7da096 to
bb1759b
Compare
src/librustc_lint/methods.rs
Outdated
|
|
||
| declare_lint! { | ||
| pub TEMPORARY_CSTRING_AS_PTR, | ||
| Deny, |
There was a problem hiding this comment.
We'll need a crater run to check the fallout from this (but it should still be deny by default unless we break some foundational crate, IMO).
There was a problem hiding this comment.
dependencies get their lints capped anyway, so it will only break building that crate directly.
There was a problem hiding this comment.
I feel that it shouldn't be deny by default as long as it considers func(CString::new("...").unwrap().as_ptr()); to be erroneous (since in typical situations this would be a false positive).
There was a problem hiding this comment.
+1 for making this Warn, and not Deny (though I don't know if we have other similar deny-by-default lints).
I think we also should add a regression test for func(CString::new("...").unwrap().as_ptr());. I wouldn't personally call it as a false-positive though: it's not UB, but still sufficiently obscure style worth linting against.
There was a problem hiding this comment.
I agree on the Warn by default, since the lint covers cases that aren't incorrect. That's reflected in the PR now.
Sure thing! Filed rust-lang/compiler-team#346. |
nathanwhit
force-pushed
the
cstring-temp-lint
branch
from
2337d68 to
6d43d7f
Compare
|
☔ The latest upstream changes (presumably #73084) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I think rust-lang/rust-clippy#5838 should be fixed before making this deny-by-default. Warn-by-default is fine if fixing the false positive is too difficult and we want to get this in soon. |
jyn514
added
A-lint
|
I feel moderately strongly that rust-lang/rust-clippy#5838 is not a false positive. it is false-positive for "this code is UB", as it is not UB. But it absolutely is a true positive for "this code is confusing, because you need to be a language lawyer to understand why it is not UB". |
Then it should say that. Instead it currently says
Which is not true. It will be invalid outside the statement, which encompasses both the |
|
Also I think it would be nice for it to link to https://doc.rust-lang.org/reference/destructors.html, but I won't block on that. |
nathanwhit
force-pushed
the
cstring-temp-lint
branch
from
6d43d7f to
ab8b582
Compare
There was a problem hiding this comment.
I think this should add a test case for rust-lang/rust-clippy#5838. If the behavior there is intentional, then it should be tested; it shouldn't slip in by accident. If it's not intentional, the lint should be fixed.
jyn514
added
S-waiting-on-author
nathanwhit
force-pushed
the
cstring-temp-lint
branch
from
ab8b582 to
42d91b3
Compare
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Uh, can we have a policy that the Clippy team is pinged for major changes to Clippy in rustc, such as uplifting lints. Even though the lint was removed, it wasn't deprecated properly (which is a lengthy process by design). Luckily @ebroto caught that in the rustup. But with a ping to the Clippy team we can avoid pull requests, like this "emergency sync": #78505 |
…eywiser Uplift the invalid_atomic_ordering lint from clippy to rustc This is mostly just a rebase of rust-lang#79654; I've copy/pasted the text from that PR below. r? `@lcnr` since you reviewed the last one, but feel free to reassign. --- This is an implementation of rust-lang/compiler-team#390. As mentioned, in general this turns an unconditional runtime panic into a (compile time) lint failure. It has no false positives, and the only false negatives I'm aware of are if `Ordering` isn't specified directly and is comes from an argument/constant/whatever. As a result of it having no false positives, and the alternative always being strictly wrong, it's on as deny by default. This seems right. In the [zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Uplift.20the.20.60invalid_atomic_ordering.60.20lint.20from.20clippy/near/218483957) `@joshtriplett` suggested that lang team should FCP this before landing it. Perhaps libs team cares too? --- Some notes on the code for reviewers / others below ## Changes from clippy The code is changed from [the implementation in clippy](https://github.com/rust-lang/rust-clippy/blob/68cf94f6a66e47234e3adefc6dfbe806cd6ad164/clippy_lints/src/atomic_ordering.rs) in the following ways: 1. Uses `Symbols` and `rustc_diagnostic_item`s instead of string literals. - It's possible I should have just invoked Symbol::intern for some of these instead? Seems better to use symbol, but it did require adding several. 2. The functions are moved to static methods inside the lint struct, as a way to namespace them. - There's a lot of other code in that file — which I picked as the location for this lint because `@jyn514` told me that seemed reasonable. 3. Supports unstable AtomicU128/AtomicI128. - I did this because it was almost easier to support them than not — not supporting them would have (ideally) required finding a way not to give them a `rustc_diagnostic_item`, which would have complicated an already big macro. - These don't have tests since I wasn't sure if/how I should make tests conditional on whether or not the target has the atomic... This is to a certain extent an issue of 64bit atomics too, but 128-bit atomics are much less common. Regardless, the existing tests should be *more* than thorough enough here. 4. Minor changes like: - grammar tweaks ("loads cannot have `Release` **and** `AcqRel` ordering" => "loads cannot have `Release` **or** `AcqRel` ordering") - function renames (`match_ordering_def_path` => `matches_ordering_def_path`), - avoiding clippy-specific helper methods that don't exist in rustc_lint and didn't seem worth adding for this case (for example `cx.struct_span_lint` vs clippy's `span_lint_and_help` helper). ## Potential issues (This is just about the code in this PR, not conceptual issues with the lint or anything) 1. I'm not sure if I should have used a diagnostic item for `Ordering` and its variants (I couldn't figure out how really, so if I should do this some pointers would be appreciated). - It seems possible that failing to do this might possibly mean there are more cases this lint would miss, but I don't really know how `match_def_path` works and if it has any pitfalls like that, so maybe not. 2. I *think* I deprecated the lint in clippy (CC `@flip1995` who asked to be notified about clippy changes in the future in [this comment](rust-lang#75671 (comment))) but I'm not sure if I need to do anything else there. - I'm kind of hoping CI will catch if I missed anything, since `x.py test src/tools/clippy` fails with a lot of errors with and without my changes (and is probably a nonsense command regardless). Running `cargo test` from src/tools/clippy also fails with unrelated errors that seem like refactorings that didnt update clippy? So, honestly no clue. 3. I wasn't sure if the description/example I gave good. Hopefully it is. The example is less thorough than the one from clippy here: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering. Let me know if/how I should change it if it needs changing. 4. It pulls in the `if_chain` crate. This crate was already used in clippy, and seems like it's used elsewhere in rustc, but I'm willing to rewrite it to not use this if needed (I'd prefer not to, all things being equal).
Uplift the invalid_atomic_ordering lint from clippy to rustc This is mostly just a rebase of rust-lang/rust#79654; I've copy/pasted the text from that PR below. r? `@lcnr` since you reviewed the last one, but feel free to reassign. --- This is an implementation of rust-lang/compiler-team#390. As mentioned, in general this turns an unconditional runtime panic into a (compile time) lint failure. It has no false positives, and the only false negatives I'm aware of are if `Ordering` isn't specified directly and is comes from an argument/constant/whatever. As a result of it having no false positives, and the alternative always being strictly wrong, it's on as deny by default. This seems right. In the [zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Uplift.20the.20.60invalid_atomic_ordering.60.20lint.20from.20clippy/near/218483957) `@joshtriplett` suggested that lang team should FCP this before landing it. Perhaps libs team cares too? --- Some notes on the code for reviewers / others below ## Changes from clippy The code is changed from [the implementation in clippy](https://github.com/rust-lang/rust-clippy/blob/68cf94f6a66e47234e3adefc6dfbe806cd6ad164/clippy_lints/src/atomic_ordering.rs) in the following ways: 1. Uses `Symbols` and `rustc_diagnostic_item`s instead of string literals. - It's possible I should have just invoked Symbol::intern for some of these instead? Seems better to use symbol, but it did require adding several. 2. The functions are moved to static methods inside the lint struct, as a way to namespace them. - There's a lot of other code in that file — which I picked as the location for this lint because `@jyn514` told me that seemed reasonable. 3. Supports unstable AtomicU128/AtomicI128. - I did this because it was almost easier to support them than not — not supporting them would have (ideally) required finding a way not to give them a `rustc_diagnostic_item`, which would have complicated an already big macro. - These don't have tests since I wasn't sure if/how I should make tests conditional on whether or not the target has the atomic... This is to a certain extent an issue of 64bit atomics too, but 128-bit atomics are much less common. Regardless, the existing tests should be *more* than thorough enough here. 4. Minor changes like: - grammar tweaks ("loads cannot have `Release` **and** `AcqRel` ordering" => "loads cannot have `Release` **or** `AcqRel` ordering") - function renames (`match_ordering_def_path` => `matches_ordering_def_path`), - avoiding clippy-specific helper methods that don't exist in rustc_lint and didn't seem worth adding for this case (for example `cx.struct_span_lint` vs clippy's `span_lint_and_help` helper). ## Potential issues (This is just about the code in this PR, not conceptual issues with the lint or anything) 1. I'm not sure if I should have used a diagnostic item for `Ordering` and its variants (I couldn't figure out how really, so if I should do this some pointers would be appreciated). - It seems possible that failing to do this might possibly mean there are more cases this lint would miss, but I don't really know how `match_def_path` works and if it has any pitfalls like that, so maybe not. 2. I *think* I deprecated the lint in clippy (CC `@flip1995` who asked to be notified about clippy changes in the future in [this comment](rust-lang/rust#75671 (comment))) but I'm not sure if I need to do anything else there. - I'm kind of hoping CI will catch if I missed anything, since `x.py test src/tools/clippy` fails with a lot of errors with and without my changes (and is probably a nonsense command regardless). Running `cargo test` from src/tools/clippy also fails with unrelated errors that seem like refactorings that didnt update clippy? So, honestly no clue. 3. I wasn't sure if the description/example I gave good. Hopefully it is. The example is less thorough than the one from clippy here: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering. Let me know if/how I should change it if it needs changing. 4. It pulls in the `if_chain` crate. This crate was already used in clippy, and seems like it's used elsewhere in rustc, but I'm willing to rewrite it to not use this if needed (I'd prefer not to, all things being equal).
The general consensus seems to be that this lint covers a common enough mistake to warrant inclusion in rustc.
The diagnostic message might need some tweaking, as I'm not sure the use of second-person perspective matches the rest of rustc, but I'd like to hear others' thoughts on that.
(cc #53224).
r? @oli-obk