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

Regression in Beta/Nightly: implementation of Trait is not general enough #106630

Closed
msrd0 opened this issue Jan 9, 2023 · 11 comments
Closed

Regression in Beta/Nightly: implementation of Trait is not general enough #106630

msrd0 opened this issue Jan 9, 2023 · 11 comments
Assignees
Labels
C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@msrd0
Copy link
Contributor

msrd0 commented Jan 9, 2023

Code

I tried this code:

https://github.com/gotham-rs/gotham/tree/3da82b63e36db81f73f0d88b7610a3979c62289a/examples/handlers/simple_async_handlers_await

I expected to see this happen: Code compiles (as it does on stable: https://github.com/gotham-rs/gotham/actions/runs/3873554934/jobs/6603712754)

Instead, this happened: Code fails to compile (on beta: https://github.com/gotham-rs/gotham/actions/runs/3873554934/jobs/6603712995 and nightly: https://github.com/gotham-rs/gotham/actions/runs/3873554934/jobs/6603713263)

Version it worked on

It most recently worked on: 1.66.0 (69f9c33 2022-12-12)

Version with regression

1.67.0-beta.6 (51b0345 2022-12-31) and rustc 1.68.0-nightly (cc47b06 2023-01-08)

@rustbot modify labels: +regression-from-stable-to-beta

@msrd0 msrd0 added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jan 9, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 9, 2023
@albertlarsan68
Copy link
Member

albertlarsan68 commented Jan 9, 2023

searched nightlies: from nightly-2022-10-29 to nightly-2022-12-10
regressed nightly: nightly-2022-12-10
searched commit range: 7632db0...dfe3fe7
regressed commit: 7701a7e (#105456)

bisected with cargo-bisect-rustc v0.6.5

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

cargo bisect-rustc --access=github --start=1.66.0 --end=2022-12-10 -- build -p=gotham_examples_handlers_simple_async_handlers_await

@rustbot label +regression-from-stable-to-beta -regression-untriaged

@rustbot rustbot added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-untriaged Untriaged performance or correctness regression. labels Jan 9, 2023
@lqd
Copy link
Member

lqd commented Jan 9, 2023

@albertlarsan68 The rollup can itself be checked with these artifacts. I'm pretty sure it's going to be #105255 again.

@albertlarsan68
Copy link
Member

It is in fact #105255 that makes this error, cc @cjgillot @compiler-errors

@cjgillot cjgillot self-assigned this Jan 9, 2023
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high T-compiler

@rustbot rustbot added P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 10, 2023
@compiler-errors compiler-errors self-assigned this Jan 12, 2023
@pnkfelix pnkfelix added E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Jan 26, 2023
@apiraino
Copy link
Contributor

apiraino commented Feb 15, 2023

Can this be closed since #106759 or it needs more work?

(beta backport in #107071)

@wesleywiser
Copy link
Member

I think we still would like an mcve and regression test to be added. Lowering priority since this was reverted.

@wesleywiser wesleywiser added P-medium Medium priority and removed P-high High priority labels Feb 16, 2023
@msrd0
Copy link
Contributor Author

msrd0 commented Feb 16, 2023

Here's a smaller version that fails to compile on nightly-2023-01-08 but compiles on rustc 1.67.1 (d5a82bbd2 2023-02-07) (Arch Linux rust 1:1.67.1-1):

mod gotham {
	use std::future::Future;

	pub struct State;

	pub trait IntoResponse {}

	pub struct Response;

	impl IntoResponse for Response {}

	mod private {
		use super::*;

		pub trait HandlerMarker {}

		pub trait AsyncHandlerFn<'a> {
			type Res;
		}

		impl<'a, Fut, R, F> AsyncHandlerFn<'a> for F
		where
			F: FnOnce(&'a mut State) -> Fut,
			Fut: Future<Output = R> + Send + 'a
		{
			type Res = R;
		}

		impl<F, R> HandlerMarker for F
		where
			R: IntoResponse + 'static,
			for<'a> F: AsyncHandlerFn<'a, Res = R> + Send
		{
		}
	}

	pub fn to_async_borrowing<F>(handler: F)
	where
		F: private::HandlerMarker
	{
		_ = handler;
	}
}

use gotham::*;

async fn sleep_handler(_: &mut State) -> impl IntoResponse {
	Response
}

fn main() {
	to_async_borrowing(sleep_handler);
}

@Mark-Simulacrum Mark-Simulacrum removed the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Mar 3, 2023
@Mark-Simulacrum
Copy link
Member

Untagging as a regression since I believe we expect this to already be fixed on beta + nightly as of #106759 (and its backport).

@msrd0
Copy link
Contributor Author

msrd0 commented Mar 3, 2023

I managed to get the code that triggers the regression even shorter:

use std::future::Future;

trait AsyncCallback<'a> {
    type Out;
}

impl<'a, Fut, T, F> AsyncCallback<'a> for F
where
    F: FnOnce(&'a mut ()) -> Fut,
    Fut: Future<Output = T> + Send + 'a,
{
    type Out = T;
}

trait CallbackMarker {}
impl<F, T> CallbackMarker for F
where
    T: 'static,
    for<'a> F: AsyncCallback<'a, Out = T> + Send,
{
}

fn do_sth<F: CallbackMarker>(_: F) {}

async fn callback(_: &mut ()) -> impl Send {}

fn main() {
    do_sth(callback);
}

Do you want me to create a PR that adds a test for this code? If so, is there some guide on how to add regression tests?

@kpreid
Copy link
Contributor

kpreid commented Dec 25, 2023

Triage: this has got a MCVE.
@rustbot label -E-needs-mcve

Do you want me to create a PR that adds a test for this code? If so, is there some guide on how to add regression tests?

@msrd0 I'm not an experienced compiler dev but I believe you simply add a test case file to tests/ui/ like in this commit for example.

@rustbot rustbot removed the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Dec 25, 2023
msrd0 added a commit to msrd0/rust that referenced this issue Dec 28, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 28, 2023
…trochenkov

Add regression test for rust-lang#106630

This PR adds a regression test for rust-lang#106630. I was unsure where exactly to place the test or how to test it locally so please let me know if I should change something.
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 28, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#119331 (rustdoc-search: count path edits with separate edit limit)
 - rust-lang#119359 (Simplify Parser::ident_or_error)
 - rust-lang#119376 (Add regression test for rust-lang#106630)
 - rust-lang#119379 (Update `parse_seq` doc)
 - rust-lang#119380 (Don't suggest writing a bodyless arm if the pattern can never be a never pattern)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 28, 2023
Rollup merge of rust-lang#119376 - msrd0:regression-test-106630, r=petrochenkov

Add regression test for rust-lang#106630

This PR adds a regression test for rust-lang#106630. I was unsure where exactly to place the test or how to test it locally so please let me know if I should change something.
@msrd0 msrd0 closed this as completed Dec 28, 2023
zetanumbers pushed a commit to zetanumbers/rust that referenced this issue Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests