Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reverse regression: unsized coercion from array to slice in Arc::from started compiling in 1.74.0 #120541

Closed
BGR360 opened this issue Feb 1, 2024 · 11 comments
Labels
A-array Area: `[T; N]` A-coercions Area: implicit and explicit `expr as Type` coercions A-inference Area: Type inference A-slice Area: `[T]` C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@BGR360
Copy link
Contributor

BGR360 commented Feb 1, 2024

Code

The following code compiles successfully on 1.74.0 and stable (1.75.0) (playground):

use std::sync::Arc;

fn takes_arc_slice(_: Arc<[u8]>) {}

fn no_binding() {
    takes_arc_slice(if true {
        Arc::from([255])
    } else {
        Arc::from([])
    });
}

fn with_binding() {
    let binding = if true {
        Arc::from([255])
    } else {
        Arc::from([])
    };
    takes_arc_slice(binding);
}

fn main() {
    no_binding();
    with_binding();
}

It fails to compile on 1.73.0 with the following error:

error[E0308]: `if` and `else` have incompatible types
  --> arc-slice.rs:17:9
   |
14 |       let buffers = if true {
   |  ___________________-
15 | |         Arc::from([255])
   | |         ---------------- expected because of this
16 | |     } else {
17 | |         Arc::from([])
   | |         ^^^^^^^^^^^^^ expected an array with a fixed size of 1 element, found one with 0 elements
18 | |     };
   | |_____- `if` and `else` have incompatible types
   |
   = note: expected struct `Arc<[{integer}; 1]>`
              found struct `Arc<[_; 0]>`

Only the with_binding version fails, interestingly.

This was reported in pola-rs/polars#14134. A user tried to compile the latest polars version on rustc 1.73.0 and got the above error.

Repro of the failure on godbolt.

Despite polars not having a documented MSRV, I feel that this kind of change in what is allowed to compile is non-obvious and wouldn't make the average MSRV-minded Rust developer think twice if it compiled on stable. The 1.74.0 release notes don't mention anything about a change here.

Request: at minimum, we should add a regression test to make sure this code continues to compile.

Version it worked on

The code failed to compile in 1.73.0.

Version with regression

The code started compiling in 1.74.0.

@rustbot modify labels: +regression-from-stable-to-stable -regression-untriaged +A-array +A-coercions +A-slice +A-inference +T-compiler

@BGR360 BGR360 added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Feb 1, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-array Area: `[T; N]` A-coercions Area: implicit and explicit `expr as Type` coercions A-inference Area: Type inference A-slice Area: `[T]` regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed regression-untriaged Untriaged performance or correctness regression. labels Feb 1, 2024
@BGR360
Copy link
Contributor Author

BGR360 commented Feb 1, 2024

@rustbot label +S-has-mcve

@rustbot rustbot added the S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue label Feb 1, 2024
@BGR360
Copy link
Contributor Author

BGR360 commented Feb 1, 2024

Possibly related:

@apiraino
Copy link
Contributor

apiraino commented Feb 1, 2024

Bisection points to #114041 cc @nvzqz @dtolnay

searched nightlies: from nightly-2023-07-08 to nightly-2024-02-01
regressed nightly: nightly-2023-09-29
searched commit range: e7c502d...7b4d9e1
regressed commit: 46da927

bisected with cargo-bisect-rustc v0.6.7

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start 1.72.0 --script ./test.sh 

@apiraino apiraino added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 1, 2024
@dtolnay
Copy link
Member

dtolnay commented Feb 1, 2024

This is not a regression.

@dtolnay dtolnay removed regression-from-stable-to-stable Performance or correctness regression from one stable version to another. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 1, 2024
@dtolnay dtolnay closed this as completed Feb 1, 2024
@BGR360
Copy link
Contributor Author

BGR360 commented Feb 1, 2024

Hi @dtolnay, feel free to remove tags that are incorrect, but please reopen the issue until a new regression test is added to verify this new functionality, as I clearly stated in the original issue in bold.

@BGR360
Copy link
Contributor Author

BGR360 commented Feb 1, 2024

There is non-obvious inference behavior here that I think makes this more interesting than just "this conversion was newly added in 1.74.0"

Also, closing people's issues with 5-word explanations is just really discouraging for people who put in the work to report issues.

@apiraino
Copy link
Contributor

apiraino commented Feb 1, 2024

@dtolnay I echo the sentiment that perhaps a bit more context would be helpful

@dtolnay
Copy link
Member

dtolnay commented Feb 1, 2024

A new API was added to the standard library. The new API has tests, is documented, is noted in the 1.74.0 release announcement, and did not break existing code.

The point of adding APIs in the standard library is that code can use them. If said API were to not exist, code could not use it. One can imagine that if code used some API, and that API did not exist, it makes sense the code would not compile. This is why, if you take code that uses an API, and you compile it with an old compiler in which the API does not exist, then you will observe that the code does not compile.

Beyond that, I am not sure what we are talking about.

@dtolnay dtolnay reopened this Feb 1, 2024
@BGR360
Copy link
Contributor Author

BGR360 commented Feb 1, 2024

A new API was added to the standard library.

This was all the context I needed. I would have felt much less discouraged if you had added that context.

I didn't know that the impl was new, and so I thought there might be something weird going on with inference and unsizing.

The new API [...] is noted in the 1.74.0 release announcement.

I see it now; I'll be sure to read more closely next time.

The point of adding APIs in the standard library is that code can use them.

Yes. Agreed on all that.

Beyond that, I am not sure what we are talking about.

Well, there is the confusing fact that the no_binding() version does compile on 1.73.0:

takes_arc_slice(if true {
    Arc::from([255])
} else {
    Arc::from([])
});

How is that possible if the impl didn't exist?

Before closing this issue, I seek to understand if/how inference is involved and understand if any additional regression test is warranted because of that. Otherwise I trust the doctests added with the new API.

@dtolnay
Copy link
Member

dtolnay commented Feb 1, 2024

This was all the context I needed. I would have felt much less discouraged if you had added that context.

I didn't know that the impl was new, and so I thought there might be something weird going on with inference and unsizing.

Good call. This new impl was linked in the previous comment above my misconceived one: #120541 (comment)

I believe there is no inference change worth testing that is at play here. As I understand it, this issue was reported under the understanding that some inference change had been made (intentionally or unintentionally) in the compiler, and after looking at the release notes, that no inference change was described there. However, no inference change was described in the release notes because no inference change was made in the compiler in that release. The same inference applies to 1.73 and 1.74 and is extensively tested.

In this code from with_binding:

let binding = if true {
    Arc::from([255])
} else {
    Arc::from([])
};

there is exactly one from that this could refer to in 1.73, which comes from impl<T> From<T> for Arc<T>. So the true branch's type is Arc<[_; 1]> and the false branch's type is Arc<[_; 0]> which are not the same type. This is an error.

In 1.74+ there are now 2 possible functions this from could be, which is the previous one plus impl<T, const N: usize> From<[T; N]> for Arc<[T]>. So the true branch's type is either Arc<[_; 1]> or Arc<[_]>, and the false branch's type is either Arc<[_; 0]> or Arc<[_]>. But we know the true branch's type and false branch's type must be identical, therefore both are Arc<[_]>.

Inference in function arguments (and also match expressions, and a small number of other syntaxes) works differently. In this code from no_binding:

takes_arc_slice(if true {
    Arc::from([255])
} else {
    Arc::from([])
});

takes_arc_slice's argument type is Arc<[_]>. So the if's type is Arc<[_]>. So the true branch's and false branch's type both are Arc<[_]>. In 1.73 the expression inside the true branch is type Arc<[_; 1]> and needs to be Arc<[_]> so an unsize coercion happens. In 1.74 it would directly use the available new From impl without any unsize coercion.

The inference relevant to no_binding and the inference relevant to with_binding are both extensively tested and there has been no change to it.

@BGR360
Copy link
Contributor Author

BGR360 commented Feb 1, 2024

Awesome, that clarification is exactly what I needed. Thanks for taking the time to write it out.

@BGR360 BGR360 closed this as completed Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-array Area: `[T; N]` A-coercions Area: implicit and explicit `expr as Type` coercions A-inference Area: Type inference A-slice Area: `[T]` C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants