Consider const fn with &self while checking liveness

[gurry]

Fixes #128272

The issue occurred because we weren't considering for liveness check builder methods that can be called from a static context (e.g. static X: Foo = X.new()). Now we do.

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[compiler-errors]

What is happening here? What does the &self receiver have anything to do with this? Why is this only being special-cased for consts?

Specifically, why isn't this a problem for pub const fn new() -> Foo? You can also call that in a const. Why isn't this a problem outside of consts?

I feel like this suggests that there is something more fundamentally wrong with the dead code pass...

[mu001999]

What does the &self receiver have anything to do with this?

The dead code analysis for public methods is based on the assumption that we must have a value of Self before we call a method with a receiver like &self. So we can check public methods later only after the Self is constructed elsewhere.

Why is this only being special-cased for consts?

Because the above assumption not works well for static items.

why isn't this a problem for pub const fn new() -> Foo?

pub const fn new() -> Foo may construct Foo, so we will check it.

[compiler-errors]

The dead code analysis for public methods is based on the assumption that we must have a value of Self before we call a method with a receiver like &self. So we can check public methods later only after the Self is constructed elsewhere.

For the record, this seems incredibly fragile and likely to break if we stabilize arbitrary self types in the future.

Because the above assumption not works well for static items.

This isn't really what I'm asking. I'm asking -- why is this check only needed for consts? You can call associated items by &self outside of consts.

[mu001999]

likely to break if we stabilize arbitrary self types in the future

For now, there are also methods with self: T, it's not a problem I think. The analysis only works for methods with implicit self.

I'm asking -- why is this check only needed for consts? You can call associated items by &self outside of consts.

Because we must use the Self and mark it live firstly, then we can check the used associated items outside of consts successfully. But for the static item whose initializer is a constant expression, the const constructor can have the receiver with static lifetime. And we have no other public constructors, so that this check is only needed for const methods. I'm not sure if I understand you.

[gurry]

The issue here is that the earlier code checked only builder methods without receivers (for reasons explained above by @mu001999). However, in #128272 someone constructed a case which wasn't covered by that analysis. In this case a builder method that did take a receiver was being called from a static context (e.g. static X: Foo = X.builder()). This PR attempts to cover that case.

The reason I'm limiting this only to const methods is nothing other than efficiency. Because only const methods can be called from a static context, checking others will be wasteful.

[compiler-errors]

I think you should not be special-casing things unnecessarily, since it complicates the implementation and makes it more complex to maintain. You said this is wasteful, but I don't think that this special-case on the compiler is worth taking on unless there is a good reason to (i.e. a perf run). This is especially true since the comment doesn't really demonstrate why this is important -- it's just restating what the code right below it does.

I still don't think either of y'all have explained why this is specific to statics. What does a static have anything to do with this? Because it represents a value that be referenced by name before it's created? I think there are other ways of creating code that "conjures" a value without a constructor having been created, i.e. None::<Type>.unwrap(). The reason I say this is because this feels just like it's special casing a more fundamental underlying issue with this analysis.

[chrisnc]

I am surprised that the approach here is to allow my weird code as a special case, along with the other much-less-special-but-still-special case of repr(C)/repr(transparent) in #127104, without rethinking the approach and starting over. Can the behavior from 1.79.0 be restored so the expansion of dead_code can be put through the proper process that's intended to avoid situations like this? #122759 (edit: I see this process is not yet official, but it seems like it could have helped a lot here)

[compiler-errors]

I agree with @chrisnc that all of this should probably be reverted and reworked, since then we're not pressured to put up hacky fixes in favor of getting it correct and well documented the first time 👍

[gurry]

Thanks for your inputs guys. I'll close this PR and let @mu001999 take another shot at the original analysis.