Add `missing_transmute_annotations` lint #12239

GuillaumeGomez · 2024-02-07T21:02:41Z

Fixes #715.

changelog: Add missing_transmute_annotations lint

GuillaumeGomez · 2024-02-07T21:20:45Z

And as usual I discover that there is already a transmute module which does pretty much all I originally did manually. Moved the lint there.

blyxyas · 2024-02-07T21:27:06Z

#715?? Guillaume, that issue was open 8 years ago. Really going on a journey back inntime

Centri3 · 2024-02-07T23:17:48Z

There's also #11047 for basically an identical lint, it's inactive though

GuillaumeGomez · 2024-02-08T10:24:29Z

Which also provides type suggestions. Maybe I should add that.

GuillaumeGomez · 2024-02-08T10:45:19Z

#715?? Guillaume, that issue was open 8 years ago. Really going on a journey back inntime

Told you I was going through issues. :p

I generally pick a random page and start taking a look there.

There's also #11047 for basically an identical lint, it's inactive though

Didn't see that one. ^^' Oh well, since it's inactive I think it's fine. I added suggestions as well.

blyxyas · 2024-02-09T09:10:45Z

I'll have to reroll this one, we are at Feb. 9 and I have practically no progress on performance (which is kinda really important), I've also set myself on vacation for this exact reason (I'd love to not have my busyness status in version control D:).

r? rust-lang/clippy

y21

Looks like a really nice lint to have! Implementation looks pretty good already. Not sure about the category and if we want to have it be warn-by-default though - that might deserve discussion :)

clippy_lints/src/transmute/mod.rs

clippy_lints/src/transmute/missing_transmute_annotations.rs

tests/ui/missing_transmute_annotations.rs

GuillaumeGomez · 2024-02-10T11:41:10Z

Applied suggestions.

tests/ui/missing_transmute_annotations.stderr

bors · 2024-02-11T13:44:06Z

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

GuillaumeGomez · 2024-02-19T14:11:25Z

Updated, cleaned up the commit history as well and implemented the two limitations:

not emitting if it comes from an external macro
not emitting if the transmute returned value is put into a let binding with type annotations

GuillaumeGomez · 2024-02-23T19:53:40Z

Anything else to be done here?

Centri3 · 2024-02-24T13:01:41Z

clippy_lints/src/transmute/missing_transmute_annotations.rs

+        last.ident.span.with_hi(path.span.hi()),
+        "transmute used without annotations",
+        "consider adding missing annotations",
+        format!("{}::<{from_ty}, {to_ty}>", last.ident.as_str()),


If one/more of the types can be left inferred, can we keep it like that here? e.g., let _: T.

I'd rather not.

Why not? Is it because it'd require rewriting it? I don't see why this shouldn't be done.

Maybe I misunderstood what you meant but this lint is about making it explicit what the transmute types are (both input and output). So if we provide annotation for one of the two, better provide for both directly.

What I meant was that, if it's within a let statement for example, we could keep Dst as _. If it's better to provide both directly, and this is still an issue with let, then we shouldn't special case let at all, no?

I see what you mean that providing them is better, but special-casing it then not suggesting it is a bit contradictory.

The idea behind my position is that if you ever remove the let binding or change the type annotation, if we originally suggested both types, it'll prevent compilation and require user to check that the code is still valid. I think it's pretty important considering how easy it is to have unexpected behaviours with transmute.

tests/ui/missing_transmute_annotations.rs

Centri3 · 2024-02-24T13:05:04Z

tests/ui/missing_transmute_annotations.rs

+
+macro_rules! local_bad_transmute {
+    ($e:expr) => {
+        std::mem::transmute($e)


Can you special case _ here in all macros? The type can't always be specified like this automatically

In such cases, I think it means the macro should be rewritten instead of us dropping the level of detection.

Centri3 · 2024-02-24T13:10:32Z

clippy_lints/src/transmute/missing_transmute_annotations.rs

+    if !missing_generic {
+        return false;
+    }
+    // If it's being set as a local variable value...


We should include closures here imo, if they have type annotations. This could include the parameter if it's specified as well, but not something like:

// i64 is known, but [i32; 2] is never specified |x: i32, y: i32| -> i64 unsafe { transmute::<[i32; 2], _>([x, y]) };

I don't think we should special case closures either. It adds a lot of complexity to detect such cases for a very marginal gain imo.

Not really. The return type part is easy, and the parameter is too - Get the Res from the QPath, the HirId of the local, then the parent's parent should include the types in FnDecl.

I wouldn't block it on this so if it's not done I can add it after, eventually.

Not in this sense. I meant, for the same reason we want to enforce having type annotations when transmute is used as return expr, we want to enforce for it for closures as well.

I don't agree with that as closures are usually private and not public. It's more akin to let than functions. I don't think you can fold a closure either in IDEs.

I don't see why closures being private or public changes anything. If you want to bring this back to debate, let's continue on zulip. ;)

I'll bring up my current points on zulip sometime.

👍 There is an open thread already, don't hesitate to comment there directly. ;)

clippy_lints/src/transmute/missing_transmute_annotations.rs

GuillaumeGomez · 2024-02-25T17:17:17Z

I added check for let var: _ =, I also extended the check to look up if the transmute call parent is a block that it is assigned in a variable with type annotations (or not). And finally, I added ADT tests as well to check suggestions look correct.

With this I think I didn't forget anything?

GuillaumeGomez · 2024-02-25T17:25:56Z

And updated ui test.

bors · 2024-03-14T17:33:30Z

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

GuillaumeGomez · 2024-03-15T19:41:27Z

As discussed on zulip, if the transmute call is the only thing in the function, then the lint won't be emitted.

Anything else to be done?

clippy_lints/src/transmute/missing_transmute_annotations.rs

GuillaumeGomez · 2024-03-16T20:52:54Z

Updated!

y21

Looks fine to me. The implementation seems more lax now (e.g. it allows a tail transmute expression in a function no matter how many other statements there are), but that's fine by me and should also make it less controversial.

It would probably be useful to have an optional configuration variable for disallowing inferred transmute node args everywhere if a user specifically wants that behavior, but I don't want to block the (initial) PR adding it for too much longer. It could always be improved separately

y21 · 2024-03-23T13:07:12Z

clippy_lints/src/transmute/missing_transmute_annotations.rs

+    if let Some(local) = get_parent_local_binding_ty(cx, expr_hir_id) {
+        // ... which does have type annotations.
+        if let Some(ty) = local.ty
+            // If this is a `let x: _ =`, we shouldn't lint.


Suggested change

// If this is a `let x: _ =`, we shouldn't lint.

// If this is a `let x: _ =`, we should lint.

?
Otherwise this comment is a bit confusing, because the opposite happens: we do lint specifically let x: _ because of the !matches!(ty.kind, TyKind::Infer) below

Forgot to update the comment, good catch!

y21 · 2024-03-23T14:02:31Z

The history here is a bit "convoluted", I tried cleaning it up a little bit by resolving my previous comments, but I don't know about @Centri3 's comments. Some of them aren't implemented, but I'm also not sure if you both agreed that its okay to not do it or if there's something left from your side?

GuillaumeGomez · 2024-03-23T14:04:50Z

Fixed comment.

It would probably be useful to have an optional configuration variable for disallowing inferred transmute node args everywhere if a user specifically wants that behavior, but I don't want to block the (initial) PR adding it for too much longer. It could always be improved separately

Please open an issue and assign me to it so it's not forgotten. 😉

GuillaumeGomez · 2024-03-23T14:07:37Z

The history here is a bit "convoluted", I tried cleaning it up a little bit by resolving my previous comments, but I don't know about @Centri3 's comments. Some of them aren't implemented, but I'm also not sure if you both agreed that its okay to not do it or if there's something left from your side?

I think it was resolved based on their comment in the zulip conversation.

EDIT: It would still be worth confirming with @Centri3 though.

Centri3 · 2024-03-23T20:37:57Z

I didn't bring up anything, though I don't think it's worth it to block on anything. It was mostly supposed to be about allowing some edge cases (like closures) which don't need to be added, but my rationale behind them.

Everything else is good.

GuillaumeGomez · 2024-03-23T22:14:45Z

Thanks for confirming!

@y21 time for r+? :3

y21 · 2024-03-23T23:28:41Z

Sounds good, can you squash the commits? r=me with that done

@bors delegate+

bors · 2024-03-23T23:28:44Z

✌️ @GuillaumeGomez, you can now approve this pull request!

If @y21 told you to "r=me" after making some further change, please make that change, then do @bors r=@y21

…r: _ = transmute`

… the only expr in the function

GuillaumeGomez · 2024-03-24T00:19:22Z

@bors r=y21

bors · 2024-03-24T00:19:24Z

📌 Commit ee25582 has been approved by y21

It is now in the queue for this repository.

bors · 2024-03-24T00:20:31Z

⌛ Testing commit ee25582 with merge 95c62ff...

bors · 2024-03-24T00:28:22Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 95c62ff to master...

rustbot assigned blyxyas Feb 7, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 7, 2024

GuillaumeGomez force-pushed the missing_transmute_annotation branch from b1980d1 to 72d7430 Compare February 7, 2024 21:20

GuillaumeGomez changed the title ~~Add missing_transmute_annotation lint~~ Add missing_transmute_annotations lint Feb 7, 2024

GuillaumeGomez force-pushed the missing_transmute_annotation branch from 72d7430 to 6374508 Compare February 8, 2024 10:44

rustbot assigned y21 and unassigned blyxyas Feb 9, 2024

y21 reviewed Feb 9, 2024

View reviewed changes

clippy_lints/src/transmute/mod.rs Show resolved Hide resolved

clippy_lints/src/transmute/mod.rs Show resolved Hide resolved

clippy_lints/src/transmute/missing_transmute_annotations.rs Outdated Show resolved Hide resolved

tests/ui/missing_transmute_annotations.rs Show resolved Hide resolved

GuillaumeGomez force-pushed the missing_transmute_annotation branch 2 times, most recently from 8a5b75a to e55f017 Compare February 10, 2024 11:19

Centri3 reviewed Feb 10, 2024

View reviewed changes

tests/ui/missing_transmute_annotations.stderr Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the missing_transmute_annotation branch 4 times, most recently from f40a339 to 235c9f3 Compare February 19, 2024 14:05

Centri3 reviewed Feb 24, 2024

View reviewed changes

y21 reviewed Feb 25, 2024

View reviewed changes

clippy_lints/src/transmute/missing_transmute_annotations.rs Show resolved Hide resolved

clippy_lints/src/transmute/missing_transmute_annotations.rs Show resolved Hide resolved

GuillaumeGomez force-pushed the missing_transmute_annotation branch from 235c9f3 to e2376e5 Compare February 25, 2024 17:15

GuillaumeGomez force-pushed the missing_transmute_annotation branch from e2376e5 to 1ad95f1 Compare February 25, 2024 17:20

GuillaumeGomez force-pushed the missing_transmute_annotation branch from 1ad95f1 to b016af5 Compare March 15, 2024 17:43

Add missing_transmute_annotation lint

7976657

GuillaumeGomez force-pushed the missing_transmute_annotation branch from b016af5 to 9fbabde Compare March 15, 2024 19:34

y21 reviewed Mar 15, 2024

View reviewed changes

clippy_lints/src/transmute/missing_transmute_annotations.rs Show resolved Hide resolved

clippy_lints/src/transmute/missing_transmute_annotations.rs Outdated Show resolved Hide resolved

y21 approved these changes Mar 23, 2024

View reviewed changes

GuillaumeGomez added 3 commits March 24, 2024 00:47

Add ui test for missing_transmute_annotations

8e04961

Correctly handle transmute as return value from block and `let va…

ffa1279

…r: _ = transmute`

Do no emit missing_transmute_annotations lint if the transmute is…

ee25582

… the only expr in the function

GuillaumeGomez force-pushed the missing_transmute_annotation branch from e52a7dd to ee25582 Compare March 23, 2024 23:51

bors merged commit 95c62ff into rust-lang:master Mar 24, 2024
8 checks passed

GuillaumeGomez deleted the missing_transmute_annotation branch March 24, 2024 10:46

	// If this is a `let x: _ =`, we shouldn't lint.
	// If this is a `let x: _ =`, we should lint.

Add missing_transmute_annotations lint #12239

Add missing_transmute_annotations lint #12239

Conversation

GuillaumeGomez commented Feb 7, 2024 • edited Loading

GuillaumeGomez commented Feb 7, 2024

blyxyas commented Feb 7, 2024

Centri3 commented Feb 7, 2024

GuillaumeGomez commented Feb 8, 2024

GuillaumeGomez commented Feb 8, 2024

blyxyas commented Feb 9, 2024

y21 left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 10, 2024

bors commented Feb 11, 2024

GuillaumeGomez commented Feb 19, 2024

GuillaumeGomez commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 25, 2024

GuillaumeGomez commented Feb 25, 2024

bors commented Mar 14, 2024

GuillaumeGomez commented Mar 15, 2024

GuillaumeGomez commented Mar 16, 2024

y21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y21 commented Mar 23, 2024

GuillaumeGomez commented Mar 23, 2024

GuillaumeGomez commented Mar 23, 2024 • edited Loading

Centri3 commented Mar 23, 2024

GuillaumeGomez commented Mar 23, 2024

y21 commented Mar 23, 2024

bors commented Mar 23, 2024

GuillaumeGomez commented Mar 24, 2024

bors commented Mar 24, 2024

bors commented Mar 24, 2024

bors commented Mar 24, 2024

Add `missing_transmute_annotations` lint #12239

Add `missing_transmute_annotations` lint #12239

GuillaumeGomez commented Feb 7, 2024 •

edited

Loading

GuillaumeGomez commented Mar 23, 2024 •

edited

Loading