Use attributes for dangling_pointers_from_temporaries lint

[gavincrawford]

Checking for dangling pointers by function name isn't ideal, and leaves out certain pointer-returning methods that don't follow the as_ptr naming convention. Using an attribute for this lint cleans things up and allows more thorough coverage of other methods, such as UnsafeCell::get().

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Urgau (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Could you also please add test cases for the newly marked methods to tests/ui/lint/dangling-pointers-from-temporaries/types.rs?

Other than the 3 points above, LGTM (after the CI is green). But maybe Urgau has something add too. (Were they assigned randomly by the way? That's nice)

Attribute validation turned out to be way simpler than I expected, which is a pleasant surprise.

@gavincrawford Could you add the missing #[rustc_as_ptr] to the slice methods, so that the tests passes.

As well as doing @GrigorenkoPV comment:

Could you also please add test cases for the newly marked methods to tests/ui/lint/dangling-pointers-from-temporaries/types.rs?

To locally run the tests you can use ./x.py test --stage 1 tests/lint/dangling/, don't hesitate to look at the relevant chapter on the rustc-dev-guide for more guidance.

@rustbot author

Fixed the slice methods, working on the test cases, and had a quick question. In order for the lint to apply to UnsafeCell, we have to include it in this function(renamed to owns_allocation in this PR):

rust/compiler/rustc_lint/src/dangling.rs

Lines 202 to 223 in 328b759

	// Array, Vec, String, CString, MaybeUninit, Cell, Box<[_]>, Box<str>, Box<CStr>,
	// or any of the above in arbitrary many nested Box'es.
	fn is_interesting(tcx: TyCtxt<'_>, ty: Ty<'_>) -> bool {
	if ty.is_array() {
	true
	} else if let Some(inner) = ty.boxed_ty() {
	inner.is_slice()
	|| inner.is_str()
	|| inner.ty_adt_def().is_some_and(|def| tcx.is_lang_item(def.did(), LangItem::CStr))
	|| is_interesting(tcx, inner)
	} else if let Some(def) = ty.ty_adt_def() {
	for lang_item in [LangItem::String, LangItem::MaybeUninit] {
	if tcx.is_lang_item(def.did(), lang_item) {
	return true;
	}
	}
	tcx.get_diagnostic_name(def.did())
	.is_some_and(|name| matches!(name, sym::cstring_type | sym::Vec | sym::Cell))
	} else {
	false
	}
	}

Which works fine, and causes the test to error out on UnsafeCell::get() as expected, but I'm struggling to find how to apply the same logic to SyncUnsafeCell. As I understand it, the synchronous version is just a wrapper around a normal UnsafeCell, and thus owns its inner allocation, but it does not have a definition that I can find in LangItem, nor a diagnostic name I can match against. Any ideas on how to handle this as of now?

but it does not have a definition that I can find in LangItem, nor a diagnostic name I can match against. Any ideas on how to handle this as of now?

I think you can make it into a new diagnostic item.

You will need to mark the struct & add the new tag to a list of symbols.

Also you can probably remove as_mut_ptr & as_ptr from the list as well. The former is most likely used only in the lint that you have now made use attributes instead. IDK about the latter, though. It might be used somewhere else.

as_ptr had a couple other uses in other spots, so I left that one for now. All tests passed on my machine, so I guess let's just hope the CI agrees.

Please, update this comment and, more importantly, the doc comment above that mentions that [Sync]UnsafeCell::get() methods are not accounted for.

Almost forgot about this, sorry.

The doc comment on lines 46–48

as_ptr had a couple other uses in other spots, so I left that one for now.

Okay, makes sense. I wonder where, probably will look at this later, as it sounds interesting.

UPD: nothing interesting, as it turns out, just some clippy usages, and they don't look like they can be improved by using the new attribute instead

All tests passed on my machine, so I guess let's just hope the CI agrees.

Probably some std ones will actually fail, as I've mentioned above #132732 (comment)

But other than what I've already mentioned, LGTM.

Once again, we'll hear from Urgau first (I do not have rights to approve PRs anyway),
and they will probably ask you to squash the commits a bit, as that's what's usually done here.

Okay, makes sense. I wonder where, probably will look at this later, as it sounds interesting.

It's used in a couple of Clippy lints, didn't look much more in to it then that.

Once again, we'll hear from Urgau first (I do not have rights to approve PRs anyway), and they will probably ask you to squash the commits a bit, as that's what's usually done here.

Yeah, I noticed the mess. Rebased and squashed the commits down.

As for the other stuff, sync_unsafe_cell will get CamelCase and I'll fix that comment as soon as I have some time to work on it.

(Back ping to #132281)

@gavincrawford Is the PR ready for review? (I ask because it's still marked as waiting on author, do @rustbot review to change it, cf. #132732 (comment))

With the addition of the attribute we should able to simplify this owns_allocation method to a simple check of the type and it's generic arguments.

Something like this would be a good starting point (the check should probably be further refined):

    match ty.kind() {
        ty::Array(_, _) => true,
        ty::Adt(_def, args) => args.iter().filter_map(|g| g.as_type()).all(|ty| !ty.is_any_ptr()),
        _ => false,
    }

I think a Box<Box<&[T]>> would pass the check that you suggest, even though it should not

Yeah, we still need to take the derefs into account.

Details

Probably something more like this:

    let adjs = cx.typeck_results().expr_adjustments(receiver);
    let origin_ty = cx.typeck_results().expr_ty(receiver);

    adjs.iter()
        .filter_map(|adj| match adj.kind {
            Adjust::Deref(_) => Some(adj.target),
            Adjust::NeverToAny
            | Adjust::Borrow(_)
            | Adjust::Pointer(_)
            | Adjust::ReborrowPin(_) => None,
        })
        .all(|ty| !ty.is_any_ptr())
        && match origin_ty.kind() {
            ty::Adt(_def, args) => {
                args.iter().filter_map(|g| g.as_type()).all(|ty| !ty.is_any_ptr())
            }
            _ => !origin_ty.is_any_ptr(),
        }

but, let's discussed that on the follow-up issue/pr.

LGTM. Let's fix the nit #132732 (comment) and defer the simplification to a follow-up PR.

@rustbot author

Just fixed that comment. I squashed it with the other comment commit for clarity, as they're both just comment edits.

@bors r+

📌 Commit 01fd384 has been approved by Urgau

It is now in the queue for this repository.

This PR seems to have caused a compile-time regression in the nalgebra crate (#133234 (comment)). Looks like some queries are now called more. If you have an idea on how to improve this, it would be nice to take a look. If not, I don't think that it's too terrible, and we can just eat the cost.