Check for intrinsic to fn ptr casts in unified coercions

[maxdexh]

Fixes #149143 by ensuring that when coercing multiple expressions to a unified type, the same "intrinsic to fn ptr" check is applied as for other coercions.

[rustbot]

r? @chenyukang

rustbot has assigned @chenyukang.
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

[Kivooeo]

Can you please add regression test and fix commit description (remove #149143 from it)

The fix itself seems reasonable to me

[maxdexh]

Can you please add regression test

Sure, under which crate should I put the test? nvm I think I got it

[Kivooeo]

We don't store tests under crates but rather under tests/ui/ directory (there are some other directories under tests/ but for this case focus on ui/), so in your case I guess the most fit directory would be something tests/ui/coercion, just create a file there with name that shorts explain what it tests and put a issue number at the end of the file name like *-1234.rs, and put a link to issue in test, thanks

[maxdexh]

We don't store tests under crates

Yeah my bad I searched for regression in my file picker and every test that showed up was under some crate

[Kivooeo]

I searched for regression in my file picker and every test that showed up was under some crate

Very likely it were not a tests, but maybe a comments or something that explains that this part of code addresses an issue

[maxdexh]

For some reason my file picker really likes src/tools/clippy

[Kivooeo]

Is there something particular you searching for that I can help you with?

[maxdexh]

Alright I got it, I was just fighting with how the error messages need to be specified. I'm not sure I did it correctly but the test tool doesn't complain anymore.

[Kivooeo]

it looks like flake.nix got leaked in this pr :)

[maxdexh]

Hehe oops

[Kivooeo]

Let's wait on CI green (I know it will be green but just to make 1000% sure)

View changes since this review

[maxdexh]

It was kinda annoying getting nix to work with it, since I either had to use nix develop path:., which takes ages to copy the entire repo to a store, or use git add -f to force my not my flake to be picked up.

Is there any better way to do this?

[Kivooeo]

Hmm, I don't actually use Nix, so I can't help here, but, you might want to join the Rust Zulip, think of it as a Discord for Rust developers

You can create a topic there about Rust development on Nix, and you'll very likely get some answers from people who actually work with it

[maxdexh]

Alright will do, thanks for the help btw!

[maxdexh]

Oh I just realized the ICE still happens if I switch the branches 😄

[Kivooeo]

Hm, can you show the example

[maxdexh]

fn main() {
    unsafe {
        let f = if true {
            safe_transmute
        } else {
            std::mem::transmute
        };

        let _: i64 = f(3i64);
    }
}

unsafe fn safe_transmute<A, B>(x: A) -> B {
    panic!()
}

[Kivooeo]

I don't get ICE with your fix, it just panics, but... it should panic

gh-Kivooeo@dev-desktop-eu-1 ~/rust (pr-branch) [101]> ./build/aarch64-unknown-linux-gnu/stage1/bin/rustc ~/test_/src/main.rs
warning: unused variable: `x`
  --> /home/gh-Kivooeo/test_/src/main.rs:13:32
   |
13 | unsafe fn safe_transmute<A, B>(x: A) -> B {
   |                                ^ help: if this is intentional, prefix it with an underscore: `_x`
   |
   = note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default

warning: 1 warning emitted

gh-Kivooeo@dev-desktop-eu-1 ~/rust (pr-branch)> ./main

thread 'main' (3589235) panicked at /home/gh-Kivooeo/test_/src/main.rs:14:5:
explicit panic
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Hm, it don't give error for some reason

[maxdexh]

Now it works for me too, I am just confused at this point. I guess I should use cargo clean more often

[Kivooeo]

I don't know, should this give error in both cases or just in the one from issue?

[maxdexh]

Both cases should error

[Kivooeo]

At very first glance, it seems that you also should add this to next_adjustment

[maxdexh]

I'm not sure what you mean, it errors in both cases

[maxdexh]

Should I add a second test or just expand the other one?

[Kivooeo]

Just expand the current test, something like

        let f2 = if true { safe_transmute } else { transmute };
        //~^ ERROR `if` and `else` have incompatible types

        let _: i64 = f2(5i64);

[maxdexh]

Oh yeah I also had to add it in next_adjustment, stale compiler output confused the hell out of me

[maxdexh]

It's a bit weird that the note "cannot coerce intrinsics to function pointers" always points at the second expression, but that has apparently been the case before 1.84 too

[Kivooeo]

stale compiler output confused the hell out of me

well, I always use directly the compiled compiler to test something locally and not cargo, like this ./build/your-target/stage1/bin/rustc path/to/your/rs-file-you-want-test (have no idea if this even correct or not but works good)

[maxdexh]

well, I always use directly the compiled compiler to test something locally and not cargo

Does rustc not use incremental compilation?

[Kivooeo]

it does

[maxdexh]

Then I don't see how it would be immune to this, cargo clean is just a helper to delete target/, right?

[maxdexh]

I guess I had no reason to be using it on my 'experiment' folder anyway, since I could've just used a test :)

[Kivooeo]

cargo clean is just a helper to delete target/, right?

afaik yes

the point is you directly call latest builded locally compiler with your fix, and not having trouble with cache and other stuff

[maxdexh]

the point is you directly call latest builded locally compiler with your fix, and not having trouble with cache and other stuff

hmm good to know, more ergonomic than going through rustup toolchain link every time anyway

[Kivooeo]

(fyi, im not going to wait on CI one more hour to merge this right away because it's 4 in a morning to me, and I would not be able to merge this later today as well, because I will lost access to internet in 3 hours for few days (in perfect scenario), so you would have to wait for another reviwer, it's very likely will be Yukang that is already assigned by a rustbot)

[maxdexh]

it's 4 in a morning to me

You don't have to, I can wait. It's late for me too, have a good night :)

And thanks again!

[BoxyUwU]

Oh I didn't notice this in #148602 but I imagine my PR fixes this too.

@bors =Kivooeo,BoxyUwU rollup

[BoxyUwU]

@bors r=Kivooeo,BoxyUwU rollup

[bors]

📌 Commit ebf1c38 has been approved by Kivooeo,BoxyUwU

It is now in the queue for this repository.