Add new lint: `strlen_on_c_strings`#7243

mgacek8 · 2021-05-18T15:24:23Z

This is WIP, linting in case of CString has been added, but for CStr, its diagnostic item needs to be available for clippy.
PR that adds diagnostic item for CStr on rust repo.
Ready for the review. Please take a look.
fixes #7145
changelog: Add new lint: strlen_on_c_strings, that lints on libc::strlen(some_cstring.as_ptr())

rust-highfive · 2021-05-18T15:24:26Z

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

camsteffen · 2021-05-19T16:01:01Z

clippy_lints/src/strlen_on_c_strings.rs

+        }
+
+        if_chain! {
+            if let hir::ExprKind::Call(func, args) = expr.kind;


drive-by nit

Suggested change

if let hir::ExprKind::Call(func, args) = expr.kind;

if let hir::ExprKind::Call(func, [recv]) = expr.kind;

bors · 2021-05-20T12:45:44Z

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

giraffate · 2021-06-09T00:24:23Z

ping from triage @mgacek8. rust-lang/rust#85439 has been merged, so is this ready for review?

mgacek8 · 2021-06-09T08:06:55Z

ping from triage @mgacek8. rust-lang/rust#85439 has been merged, so is this ready for review?

Not yet, I resolved conflicts, but I need to investigate why is_type_diagnostic_item(cx, ty, sym::CStr) doesn't return true when type is CStr and fix it.

bors · 2021-06-10T00:42:59Z

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

mgacek8 · 2021-06-24T09:55:07Z

It is ready for the review, please take a look. Can we change the label ?

giraffate

Overall looks good, thanks! I made some comments.

clippy_lints/src/strlen_on_c_strings.rs

bors · 2021-06-30T18:29:59Z

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

giraffate · 2021-07-01T14:08:24Z

tests/ui/strlen_on_c_strings.stderr

+  --> $DIR/strlen_on_c_strings.rs:11:24
+   |
+LL |     let len = unsafe { libc::strlen(cstring.as_ptr()) };
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `cstring.as_bytes().len()`


The suggested code don't need unsafe block. Can unsafe block be checked? Or this comments needs to be improved to include removing unsafe block.

Good catch, yes, unsafe block is redundant here. It can be removed when it's the only one unsafe operation inside of it.
What about for now to lint like this: "help: try this: cstring.as_bytes().len(), you might also need to get rid of unsafe block in some cases"?

Yes, it looks good as a first step! That unsafe block might be removed is one of purposes of this lint.

If unsafe block has only libc::strlen(cstr.as_ptr()), it would be better to remove unsafe block in suggestion. But I think it can be fixed with another follow-up PR.

Created the issue for that: #7436

giraffate · 2021-07-05T07:55:15Z

tests/ui/strlen_on_c_strings.stderr

+  --> $DIR/strlen_on_c_strings.rs:11:24
+   |
+LL |     let len = unsafe { libc::strlen(cstring.as_ptr()) };
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `cstring.as_bytes().len(), you might also need to get rid of `unsafe` block in some cases`


Can this comment be fixed like this?

Suggested change

| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `cstring.as_bytes().len(), you might also need to get rid of `unsafe` block in some cases`

| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `cstring.as_bytes().len()`, you might also need to get rid of `unsafe` block in some cases

Seems like span_lint_and_sugg doesn't allow that.
What about: "help: try this (you might also need to get rid of unsafe block in some cases): cstring.as_bytes().len()" ?

Ok, it looks good!

giraffate · 2021-07-05T13:12:38Z

@bors r+

Thanks!

bors · 2021-07-05T13:12:39Z

📌 Commit 59a164e has been approved by giraffate

bors · 2021-07-05T13:12:48Z

⌛ Testing commit 59a164e with merge 64d74df...

bors · 2021-07-05T13:26:24Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 64d74df to master...

rust-highfive assigned giraffate May 18, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 18, 2021

mgacek8 marked this pull request as draft May 18, 2021 15:25

mgacek8 force-pushed the issue7145_strlen_on_c_strings branch 2 times, most recently from 8f0a09a to c3de3a9 Compare May 18, 2021 18:54

camsteffen reviewed May 19, 2021

View reviewed changes

giraffate added 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 May 25, 2021

mgacek8 force-pushed the issue7145_strlen_on_c_strings branch from c3de3a9 to 441420d Compare June 9, 2021 07:52

mgacek8 force-pushed the issue7145_strlen_on_c_strings branch 2 times, most recently from 3fb1c7d to 9feae0d Compare June 13, 2021 12:58

mgacek8 marked this pull request as ready for review June 13, 2021 13:01

mgacek8 force-pushed the issue7145_strlen_on_c_strings branch from 9feae0d to 724da32 Compare June 13, 2021 13:37

giraffate reviewed Jun 25, 2021

View reviewed changes

clippy_lints/src/strlen_on_c_strings.rs Show resolved Hide resolved

clippy_lints/src/strlen_on_c_strings.rs Show resolved Hide resolved

clippy_lints/src/strlen_on_c_strings.rs Outdated Show resolved Hide resolved

clippy_lints/src/strlen_on_c_strings.rs Outdated Show resolved Hide resolved

mgacek8 force-pushed the issue7145_strlen_on_c_strings branch from 724da32 to 9ef47c9 Compare June 30, 2021 07:20

mgacek8 force-pushed the issue7145_strlen_on_c_strings branch from 9ef47c9 to e02d468 Compare June 30, 2021 20:42

giraffate reviewed Jul 1, 2021

View reviewed changes

mgacek8 force-pushed the issue7145_strlen_on_c_strings branch from e02d468 to 5cad4f5 Compare July 5, 2021 07:16

giraffate reviewed Jul 5, 2021

View reviewed changes

Add new lint: strlen_on_c_strings

59a164e

mgacek8 force-pushed the issue7145_strlen_on_c_strings branch from 5cad4f5 to 59a164e Compare July 5, 2021 09:10

giraffate approved these changes Jul 5, 2021

View reviewed changes

bors merged commit 64d74df into rust-lang:master Jul 5, 2021

bors mentioned this pull request Jul 5, 2021

New lint: [self_named_constructor] #7403

Merged

mgacek8 mentioned this pull request Jul 5, 2021

strlen_on_c_strings lint to remove unsafe block in suggestion if it has only libc::strlen(cstr.as_ptr()) inside of it #7436

Closed

	if let hir::ExprKind::Call(func, args) = expr.kind;
	if let hir::ExprKind::Call(func, [recv]) = expr.kind;

	\| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `cstring.as_bytes().len(), you might also need to get rid of `unsafe` block in some cases`
	\| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `cstring.as_bytes().len()`, you might also need to get rid of `unsafe` block in some cases

Conversation

mgacek8 commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented May 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented May 20, 2021

Uh oh!

giraffate commented Jun 9, 2021

Uh oh!

mgacek8 commented Jun 9, 2021

Uh oh!

bors commented Jun 10, 2021

Uh oh!

mgacek8 commented Jun 24, 2021

Uh oh!

giraffate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Jun 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giraffate commented Jul 5, 2021

Uh oh!

bors commented Jul 5, 2021

Uh oh!

bors commented Jul 5, 2021

Uh oh!

bors commented Jul 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mgacek8 commented May 18, 2021 •

edited

Loading