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

Avoid follow-up errors if the number of generic parameters already doesn't match #125608

Merged
merged 9 commits into from
Jun 4, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 27, 2024

fixes #125604

best reviewed commit-by-commit

@rustbot
Copy link
Collaborator

rustbot commented May 27, 2024

r? @BoxyUwU

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 27, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 27, 2024

HIR ty lowering was modified

cc @fmease

@rust-log-analyzer

This comment has been minimized.

Comment on lines 219 to 221
if !trait_ref.self_ty().references_error() {
assert_eq!(trait_ref.self_ty(), dummy_self);
}
Copy link
Member

@BoxyUwU BoxyUwU Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit nervous about this change. I feel like the rest of the compiler ought to be able to assume that trait objects are represented "correctly", especially with how we're starting to not end the compilation as early when hitting errors nowadays

Is it possible to make sure we still use the right self ty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted the change and instead only marked those generic params as errors that were actually erroneous

Copy link
Member

@BoxyUwU BoxyUwU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool

@bors
Copy link
Contributor

bors commented Jun 3, 2024

☔ The latest upstream changes (presumably #125778) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk force-pushed the subsequent_lifetime_errors branch from 374cfb1 to adb2ac0 Compare June 3, 2024 13:21
Comment on lines -572 to +564
let args = args.unwrap();
if args.iter().any(|arg| match arg.unpack() {
GenericArgKind::Type(ty) => ty.references_error(),
_ => false,
}) {
if let Some(prev) =
preceding_args.iter().find_map(|arg| match arg.unpack() {
GenericArgKind::Type(ty) => ty.error_reported().err(),
_ => None,
})
{
// Avoid ICE #86756 when type error recovery goes awry.
return Ty::new_misc_error(tcx).into();
return Ty::new_error(tcx, prev).into();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fishy ICE workaround, but I rather preserved it for now instead of looking into it

@@ -1275,6 +1275,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

fn provided_kind(
&mut self,
_preceding_args: &[ty::GenericArg<'tcx>],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could actually allow generic const generics now here, but there's probably other work required elsewhere to support it properly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah basically nothing is set up for that at all lol

gen_args.args[max_expected_args..provided_args].iter().map(|arg| arg.span()),
);
};
invalid_args.extend(min_expected_args..provided_args);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this not give a backwards range (i.e. 2..1) if you have a fn foo<'a: 'a, 'b: 'b> and you call it foo::<'static>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that's the prev behaviour. I just merged the two loops

@BoxyUwU
Copy link
Member

BoxyUwU commented Jun 3, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jun 3, 2024

📌 Commit d498eb5 has been approved by BoxyUwU

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2024
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Jun 4, 2024
…, r=BoxyUwU

Avoid follow-up errors if the number of generic parameters already doesn't match

fixes rust-lang#125604

best reviewed commit-by-commit
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#122804 (Item bounds can reference self projections and still be object safe)
 - rust-lang#124486 (Add tracking issue and unstable book page for `"vectorcall"` ABI)
 - rust-lang#125504 (Change pedantically incorrect OnceCell/OnceLock wording)
 - rust-lang#125608 (Avoid follow-up errors if the number of generic parameters already doesn't match)
 - rust-lang#125690 (ARM Target Docs Update)
 - rust-lang#125750 (Align `Term` methods with `GenericArg` methods, add `Term::expect_*`)
 - rust-lang#125818 (Handle no values cfgs with `--print=check-cfg`)
 - rust-lang#125909 (rustdoc: add a regression test for a former blanket impl synthesis ICE)
 - rust-lang#125919 (Remove stray "this")

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#124486 (Add tracking issue and unstable book page for `"vectorcall"` ABI)
 - rust-lang#125504 (Change pedantically incorrect OnceCell/OnceLock wording)
 - rust-lang#125608 (Avoid follow-up errors if the number of generic parameters already doesn't match)
 - rust-lang#125690 (ARM Target Docs Update)
 - rust-lang#125750 (Align `Term` methods with `GenericArg` methods, add `Term::expect_*`)
 - rust-lang#125818 (Handle no values cfgs with `--print=check-cfg`)
 - rust-lang#125909 (rustdoc: add a regression test for a former blanket impl synthesis ICE)
 - rust-lang#125919 (Remove stray "this")

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0dc6550 into rust-lang:master Jun 4, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
Rollup merge of rust-lang#125608 - oli-obk:subsequent_lifetime_errors, r=BoxyUwU

Avoid follow-up errors if the number of generic parameters already doesn't match

fixes rust-lang#125604

best reviewed commit-by-commit
@oli-obk oli-obk deleted the subsequent_lifetime_errors branch June 4, 2024 13:10
Comment on lines -1 to -20
//@ known-bug: #121134
trait Output<'a> {
type Type;
}

struct Wrapper;

impl Wrapper {
fn do_something_wrapper<O, F>(&mut self, do_something_wrapper: F)
where
FnOnce:,
F: for<'a> FnOnce(<F as Output<i32, _>>::Type),
{
}
}

fn main() {
let mut wrapper = Wrapper;
wrapper.do_something_wrapper::<i32, _>(|value| ());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this issue is not closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 14, 2024
Add regression test for a gce + effects ICE

Fixes rust-lang#125770

I'm not *exactly* sure why this stopped ICEing, I assume its something to do with the fact that there used to be a generic parameter on `Add` for the host generic and we have mismatched args here, which rust-lang#125608 made no longer later cause issues. But now the desugaring is also different so? 🤷‍♀️

r? `@fee1-dead`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2024
Rollup merge of rust-lang#127711 - BoxyUwU:add_effects_test, r=fee1-dead

Add regression test for a gce + effects ICE

Fixes rust-lang#125770

I'm not *exactly* sure why this stopped ICEing, I assume its something to do with the fact that there used to be a generic parameter on `Add` for the host generic and we have mismatched args here, which rust-lang#125608 made no longer later cause issues. But now the desugaring is also different so? 🤷‍♀️

r? `@fee1-dead`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants