new lint: `manual_c_str_literals` #11919

y21 · 2023-12-03T22:55:42Z

With rust-lang/rust#117472 merged and c"" syntax stabilized, I think it'd be nice to have a lint for using CStr::from_ptr (and similar constructors) with a string literal as an argument.
We can probably also lint "foo\0".as_ptr() and suggest c"foo".as_ptr(). I might add that to this PR tomorrow if I find the time.

The byte string literal to c string literal rewriting is ugly but oh well.

changelog: new lint: manual_c_str_literals
#11919

rustbot · 2023-12-03T22:55:48Z

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

xFrednet · 2023-12-04T09:42:47Z

This PR is still marked as being a draft. Let me know when it's ready or if you have any questions, of course.

y21 · 2023-12-04T17:49:43Z

Alright, I think it should be ready for review now.

It now also lints bare "foo\0".as_ptr() (without a CStr::from_* call), inspired by rust-lang/rust#118566. This involved a little bit of extra logic because it shouldn't lint twice for something like CStr::from_ptr(b"f\0".as_ptr()) (once for the CStr::from_ptr expression and the .as_ptr() call).

This still needs #![feature(c_str_literals)]s in a few places but I think I'll be able to remove them after the next sync when the pinned nightly is bumped.

y21 · 2023-12-04T18:45:10Z

Did a lintcheck run on the top 300 crates and the only warnings are from the openssl-sys crate. They all look legitimate and c"" would work fine here.
https://docs.rs/openssl-sys/latest/src/openssl_sys/sha.rs.html#18
https://docs.rs/openssl-sys/latest/src/openssl_sys/sha.rs.html#35
https://docs.rs/openssl-sys/latest/src/openssl_sys/sha.rs.html#51

xFrednet

Looks good to me, a few small nits, but they should hopefully be simple to fix :)

clippy_lints/src/methods/manual_c_str_literals.rs

tests/ui/manual_c_str_literals.fixed

y21 · 2023-12-05T17:05:04Z

Do you think that we should wait with merging until c"" is actually usable on the stable channel (when 1.76 gets released)?
Because otherwise, nightly users will get this lint and changing it means that it won't build on the stable channel. 🤔

xFrednet

Tiny nit, and then everything should be good to go

clippy_lints/src/methods/manual_c_str_literals.rs

xFrednet · 2023-12-08T09:17:23Z

Do you think that we should wait with merging until c"" is actually usable on the stable channel (when 1.76 gets released)?
Because otherwise, nightly users will get this lint and changing it means that it won't build on the stable channel.

That's a good idea, can you ping me after the next release? Then I can r+ this :)

xFrednet

LGTM, now we're just waiting for the next release :)

y21 · 2023-12-12T01:18:13Z

Wouldn't we have to wait until 1.76, which is in two releases, since that's when c"" literals will be on stable? Or rather, ~two weeks before the 1.76 release, so that it makes it into nightly with the sync just in time.

If we merge when 1.75 gets released, we'd still have the problem that stable cannot yet use that syntax and nightly users likely don't want this lint because they still want their project to build on stable, right?

It's possible that I have the release cycle mixed up though

bors · 2023-12-16T23:55:31Z

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

y21 · 2023-12-17T00:09:08Z

Going to hold off fixing conflicts since we're gonna need to wait a bit anyway

xFrednet · 2023-12-17T21:23:31Z

If we merge when 1.75 gets released, we'd still have the problem that stable cannot yet use that syntax and nightly users likely don't want this lint because they still want their project to build on stable, right?

I'm a bit undecided on this one. It would be cool, to have a lint that suggests the new c"" literals on stable, one version after the stabilization of the feature. I think it would be better to merge it with 1.75. Alternatively, we could also flag this for a beta backport 🤔. Then it would be on beta at the same time as the new feature, while only blocking the nightly + stable users for a short time.

Actually, thinking about it, users can just set the minimal rust version, if they want to prevent the lint from triggering on nightly. With that config, I would actually say it's safe to merge it as is.

y21 · 2023-12-22T09:24:37Z

Looks like there's an ongoing discussion in the T-lang zulip for reverting the stabilization of c string literals, at least for 1.76
https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/rfc.203349.3A.20mixed.20utf8.20literals/near/409504826

I think if it does end up being reverted, we should probably really wait, since then the MSRV is also wrong. But I do agree with your last point, that if the MSRV is correct, users can set that to not get the lint

y21 · 2024-02-04T16:42:50Z

So, C-String literals will be stable in 1.77 and 1.76 will be stable in a few days. Do you think we should try to get this merged soon, or wait?

xFrednet · 2024-02-05T16:17:25Z

Let's merge this soon, if we merge it now, it should be on stable in 1.78.

For some context:

Version 1.76 is now on the stable branch in the rust repo
Version 1.77 is now on the beta branch in the rust repo
The next sync will be merged on to master, which will be branched to beta for 1.78

y21 · 2024-02-05T16:29:15Z

Squashed the commits and rebased on to master

xFrednet · 2024-02-05T17:43:40Z

clippy_config/src/msrvs.rs

@@ -17,7 +17,7 @@ macro_rules! msrv_aliases {

 // names may refer to stabilized feature flags or library items
 msrv_aliases! {
-    1,76,0 { PTR_FROM_REF }
+    1,76,0 { PTR_FROM_REF, C_STR_LITERALS }


It looks like they're planning to stabilize it in 1.77:

We discussed this in the triage call today and agreed, given that rust-lang/rust#119172 has landed, that we were happy to see this restabilize in Rust 1.77

rust-lang/rust#105723 (comment)

On nightly these strings already seem to work, so I think it's safe to merge this, once the version is updated :)

oh right, oops, of course 😅

xFrednet · 2024-02-05T17:57:16Z

Perfect, looks good to me, thank you for all the work you put into this. Let's get this on to master:

@bors r+

bors · 2024-02-05T17:57:20Z

📌 Commit 7f80b44 has been approved by xFrednet

It is now in the queue for this repository.

bors · 2024-02-05T17:58:28Z

⌛ Testing commit 7f80b44 with merge 3a0abe8...

new lint: `manual_c_str_literals` With rust-lang/rust#117472 merged and `c""` syntax stabilized, I think it'd be nice to have a lint for using `CStr::from_ptr` (and similar constructors) with a string literal as an argument. We can probably also lint `"foo\0".as_ptr()` and suggest `c"foo".as_ptr()`. I might add that to this PR tomorrow if I find the time. The byte string literal to c string literal rewriting is ugly but oh well. changelog: new lint: `manual_c_str_literals`

bors · 2024-02-05T18:03:34Z

💔 Test failed - checks-action_test

xFrednet · 2024-02-05T18:04:20Z

MacOS is not our friend rn. We just have to retry until it works.

@bors retry

@bors d+

new lint: `manual_c_str_literals` With rust-lang/rust#117472 merged and `c""` syntax stabilized, I think it'd be nice to have a lint for using `CStr::from_ptr` (and similar constructors) with a string literal as an argument. We can probably also lint `"foo\0".as_ptr()` and suggest `c"foo".as_ptr()`. I might add that to this PR tomorrow if I find the time. The byte string literal to c string literal rewriting is ugly but oh well. changelog: new lint: `manual_c_str_literals` [#11919](#11919)

bors · 2024-02-05T18:05:28Z

⌛ Testing commit 7f80b44 with merge 5bae9d4...

bors · 2024-02-05T18:12:07Z

💔 Test failed - checks-action_test

xFrednet · 2024-02-05T18:14:47Z

@bors retry

Please 🙏

bors · 2024-02-05T18:15:54Z

⌛ Testing commit 7f80b44 with merge 005b6c2...

bors · 2024-02-05T18:27:11Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 005b6c2 to master...

rustbot assigned xFrednet Dec 3, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 3, 2023

y21 force-pushed the manual_c_str_literals branch from 6e25e10 to 4be054f Compare December 3, 2023 23:12

y21 marked this pull request as draft December 4, 2023 04:47

y21 force-pushed the manual_c_str_literals branch from 4be054f to d66b5ae Compare December 4, 2023 07:45

y21 marked this pull request as ready for review December 4, 2023 17:49

xFrednet reviewed Dec 5, 2023

View reviewed changes

y21 force-pushed the manual_c_str_literals branch from 6a0998d to b394a90 Compare December 5, 2023 21:31

xFrednet approved these changes Dec 8, 2023

View reviewed changes

clippy_lints/src/methods/manual_c_str_literals.rs Show resolved Hide resolved

xFrednet approved these changes Dec 11, 2023

View reviewed changes

y21 force-pushed the manual_c_str_literals branch from 4fb28b0 to 1e3e96a Compare February 5, 2024 16:24

xFrednet reviewed Feb 5, 2024

View reviewed changes

new lint: manual_c_str_literals

7f80b44

y21 force-pushed the manual_c_str_literals branch from 1e3e96a to 7f80b44 Compare February 5, 2024 17:52

xFrednet approved these changes Feb 5, 2024

View reviewed changes

bors merged commit 005b6c2 into rust-lang:master Feb 5, 2024
8 checks passed

y21 mentioned this pull request Mar 26, 2024

Allow filter_map_identity when the closure is typed #12562

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new lint: `manual_c_str_literals` #11919

new lint: `manual_c_str_literals` #11919

y21 commented Dec 3, 2023 •

edited by xFrednet

Loading

rustbot commented Dec 3, 2023

xFrednet commented Dec 4, 2023

y21 commented Dec 4, 2023

y21 commented Dec 4, 2023

xFrednet left a comment

y21 commented Dec 5, 2023

xFrednet left a comment

xFrednet commented Dec 8, 2023

xFrednet left a comment

y21 commented Dec 12, 2023

bors commented Dec 16, 2023

y21 commented Dec 17, 2023

xFrednet commented Dec 17, 2023

y21 commented Dec 22, 2023

y21 commented Feb 4, 2024

xFrednet commented Feb 5, 2024

y21 commented Feb 5, 2024

xFrednet Feb 5, 2024

y21 Feb 5, 2024

xFrednet commented Feb 5, 2024

bors commented Feb 5, 2024

bors commented Feb 5, 2024

bors commented Feb 5, 2024

xFrednet commented Feb 5, 2024

bors commented Feb 5, 2024

bors commented Feb 5, 2024

xFrednet commented Feb 5, 2024

bors commented Feb 5, 2024

bors commented Feb 5, 2024

new lint: manual_c_str_literals #11919

new lint: manual_c_str_literals #11919

Conversation

y21 commented Dec 3, 2023 • edited by xFrednet Loading

rustbot commented Dec 3, 2023

xFrednet commented Dec 4, 2023

y21 commented Dec 4, 2023

y21 commented Dec 4, 2023

xFrednet left a comment

Choose a reason for hiding this comment

y21 commented Dec 5, 2023

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet commented Dec 8, 2023

xFrednet left a comment

Choose a reason for hiding this comment

y21 commented Dec 12, 2023

bors commented Dec 16, 2023

y21 commented Dec 17, 2023

xFrednet commented Dec 17, 2023

y21 commented Dec 22, 2023

y21 commented Feb 4, 2024

xFrednet commented Feb 5, 2024

y21 commented Feb 5, 2024

xFrednet Feb 5, 2024

Choose a reason for hiding this comment

y21 Feb 5, 2024

Choose a reason for hiding this comment

xFrednet commented Feb 5, 2024

bors commented Feb 5, 2024

bors commented Feb 5, 2024

bors commented Feb 5, 2024

xFrednet commented Feb 5, 2024

bors commented Feb 5, 2024

bors commented Feb 5, 2024

xFrednet commented Feb 5, 2024

bors commented Feb 5, 2024

bors commented Feb 5, 2024

new lint: `manual_c_str_literals` #11919

new lint: `manual_c_str_literals` #11919

y21 commented Dec 3, 2023 •

edited by xFrednet

Loading