Refactor inner allocation logic of temp dangling pointer lint

[Urgau]

This PR refactors the inner allocation logic of the dangling_pointers_from_temporaries lint by removing the hardcoded list of types and instead taking advantage of the semantics of the as_ptr{,_mut} methods and derefs.

cc @GrigorenkoPV
r? compiler

[GrigorenkoPV]

Okay, I've checked, [&u8; 10] gets linted, as it should.

The MaybeUninit<&u8> does not, even though I think it should. See above.

Anyways, with this change we can now remove the diagnostic item that was added for SyncUnsafeCell in #132732.

Also cc #132281 & #128985.

[GrigorenkoPV]

Anyways, with this change we can now remove the diagnostic item that was added for SyncUnsafeCell in #132732.

And Cell too, added in #128985.
https://github.com/rust-lang/rust/pull/128985/files#diff-75a679ddd1aae61c8b60e45cea1fb213a086caee3600993c68af9f7a09785e9eR307
Well, unless someone else started using it since then.

[BoxyUwU]

Having spent the past few days looking over this lint I don't really want to merge this PR as is without a more thorough rework of this lint. It's very hard to review or approve this PR when everything generally seems very adhoc without any clear logic in mind.

is_temporary_rvalue is completely wrong if the behaviour is actually supposed to do what it says it does. It returns false for many things that are not place expressions, e.g. consts, paths to non-statics, array/repeat exprs. It also uses rvalue/lvalue terminology which is not something we tend to use in rust, instead opting for talking about things in terms of "places".

is_inner_allocation_owned seems very misguided. You cannot tell whether ownership of the rustc_as_ptr type was created in the reciever expr just from looking at the unadjusted type. For example given the type &Vec<u8> the reciever expr could either be &vec![10] which should be linted on, or it could have been &my_vec which should not be linted on.

The intention seems to be that if there is a reference anywhere in the adjustments to get to the final &AllocationOwner then we don't have to lint as we are referencing some allocation that was created outside of this statement? This is just incorrect as demonstrated by &vec![10].

I honestly don't see a way to implement this lint correctly in the general case without turning this into a MIR lint and doing some kind of psuedo-borrowck that attempts to determine what places should be considered "used" by the returned pointer and then checking if they're all live when the returned pointer is used. Other than rewriting this to MIR lint I think a viable path forward would be to stop trying to make the lint so complex and just make it work for "obvious" stuff.

You should recurse on the reciever expr looking for anything that constructs a value of the same type that the rustc_as_ptr is on the method for. Handling arbitrary block expressions seems complex so I would suggest not recursing into them and just checking if their type is that of the rustc_as_ptr marked type and linting if that is the case. Though it occurs to me that the is_inner_allocation_owned logic would actually be correct to use in this case as any references in the type of block expr cannot possibly point to locals of the block expr.

We should also make sure not to lint if there are any Deref/Mut trait based derefs as there is no way to correctly handle those, e.g. (&Foo(vec![10], &my_vec)).as_ptr() could either return a borrow of the temporary or the borrow of my_vec.

On an unrelated note I think it's probably fine to not handle const promotion. Most cases with as_ptr() are not going to be const promotable anyway as most allocation owning types have Drop impls, and if they dont it's probably for the best for people to explicitly write const { &/* sus stuff */ }.as_ptr().

Finally, the fact that (&vec![10]).as_ptr() doesn't result in a lint says to me that there is not enough tests for this lint. Before anything here is merged there should be significantly more tests added (especially one for all the cases where we were returning false from is_temporary_rvalue incorrectly)

[BoxyUwU]

@rustbot author

[Urgau]

When I reviewed #128985 (the PR that generalized the temporary_cstring_as_ptr lint to this one) I had the same reflections about the lint having many false negatives and that making it a MIR lint would probably be the only way to make it a 100% accurate.

Which then lead me to the same reflection about the difficulty and complexity that it would be and the fact that we probably only want to handle "obvious" case. Which is why the current lint is quite imperfect and why I posted this PR to improve it bit by bit (which I recognized in my original review).

(&vec![]).as_ptr() is indeed a false negative and should probably be handled at some point, as for is_inner_allocation_owned being conservative it was a deliberate choice to avoid false positives, as we don't yet look at the receiver expression.

Yeah is_temporary_rvalue is a misnomer (should probably have been named is_maybe_temporary_expr or something like this) and is indeed wrong in the pure sense.

Recursing into the receiver expression seems like an interesting idea, not sure how complex that would be.

[Urgau]

Recursing into the receiver expression seems like an interesting idea, not sure how complex that would be.

I have looked at this proposal and came over an issue where the owned type is different from the deref type, this is the case for String where the as_ptr method is on &str (and not String).

We could have some sort of attribute to "link" them up but I fear that we will end up with even bigger perf regressions than the one fixed in #133509 where just looking if the method had #[rustc_as_ptr] had was already costing us some percent.

I'm not sure what to do. Maybe merge this PR (since I still think it's an improvement IMO) and open an issue about improving the implementation?

@rustbot ready

[BoxyUwU]

Can we try and find other ways to resolve that performance problem? If the problem is that getting attributes on every method call is too expensive then could we add a cross-crate query which returns a list of defids for methods (and associated owned self type) that we should lint on. That way we would be reading from metadata only once for a relatively small list of defids (which will be cached) compared to having a separate query and metadata reading for every single def you call has_attr on.