Add `missing_transmute_annotations` lint by GuillaumeGomez · Pull Request #12239... - JOYK Joy of Geek, Geek News, Link all geek

Conversation

Member

Fixes #715.

r? @blyxyas

changelog: Add missing_transmute_annotations lint

rustbot

added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label

Feb 7, 2024

GuillaumeGomez

changed the title ~~Add missing_transmute_annotation lint~~

Add missing_transmute_annotations lint

Feb 7, 2024

Member

Author

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.

Member

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

Member

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

Member

Author

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

Member

Author

#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.

Member

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 and GuillaumeGomez reacted with thumbs up emojiGuillaumeGomez reacted with heart emoji

Member

y21

left a comment

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 :)

Member

Author

Applied suggestions.

Collaborator

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

Member

Author

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

Member

Author

Anything else to be done here?

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

Show resolved

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.

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. ;)

Member

Author

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?

Member

Author

And updated ui test.

Collaborator

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

Member

Author

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?

Member

Author

Updated!

Member

y21

left a comment

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

clippy_lints/src/transmute/missing_transmute_annotations.rs

Outdated

		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.

Member

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!

Member

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?

Member

Author

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. 😉

y21 reacted with thumbs up emoji

Member

Author

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.

Member

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.

Member

Author

Thanks for confirming!

@y21 time for r+? :3

Member

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

@bors delegate+

Collaborator

✌️ @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

Member