Add revisions for -Zlower-impl-trait-in-trait-to-assoc-ty fixed tests #109193

spastorino · 2023-03-15T21:43:45Z

Needs to go on top of #109198

compiler-errors · 2023-03-15T21:54:47Z

Moving forward, can you not commit a separate "Add revisions to fixed tests" commit? If any of the first 3 commits fixes a UI test, then that UI test should be associated with each commit that fixes it. If the commit doesn't fix any UI tests, then it can probably be squashed together with the other commits that fully fix a test or not.

Basically I want to see--

Commit A

Changes some files
Fixes some UI tests

Commit B

Changes some files
Fixes some UI tests

It's hard to tell what is fixed by what, especially because there are not many comments in this PR really.

compiler/rustc_hir_analysis/src/check/check.rs

compiler-errors · 2023-03-15T21:57:15Z

compiler/rustc_infer/src/infer/opaque_types.rs

@@ -396,10 +396,29 @@ impl<'tcx> InferCtxt<'tcx> {
    /// defining scope.
    #[instrument(skip(self), level = "trace", ret)]
    fn opaque_type_origin_unchecked(&self, def_id: LocalDefId) -> OpaqueTyOrigin {


It makes me uncomfortable that we're calling opaque_type_origin_unchecked on an associated type. That's probably wrong...

Going to check why is being called.

I think this is fixed alternatively by #109198, and should make it so that we don't need to modify opaque_type_origin_unchecked at all.

Can you tell me how #109198 avoid calling this function?. I see in the logs that this is coming from check_fn.

Given some -> impl Trait in trait, let's call that opaque def-id#1 (DefKind::Opaque). We later synthesize a def-id#2, which is an associated type (DefKind::AssocTy).

In typeck_with_fallback, we normalize ty::Alias(ty::Projection, def-id#2) in the original trait signature to ty::Alias(ty::Opaque, def-id#2) instead of ty::Alias(ty::Opaque, def-id#1), because of this FIXME:

rust/compiler/rustc_ty_utils/src/ty.rs

Lines 271 to 272 in c5c4340

// FIXME(-Zlower-impl-trait-in-trait-to-assoc-ty) need to project to the opaque, could

// get it via type_of + subst.

rust/compiler/rustc_ty_utils/src/ty.rs

Lines 287 to 295 in c5c4340

self.predicates.push(

ty::Binder::bind_with_vars(

ty::ProjectionPredicate {

projection_ty: alias_ty,

term: self.tcx.mk_alias(ty::Opaque, alias_ty).into(),

},

self.bound_vars,

)

.to_predicate(self.tcx),

That's wrong, because def-id#2 is an associated type.

That passes a signature with the return type of ty::Alias(ty::Opaque, def-id#2) to check_fn, which passes it to replace_opaque_types_with_inference_vars. That tries to call opaque_type_origin on def-id#2 (reminder: an associated type) which ICEs.

That is what I fixed in #109198.

compiler-errors · 2023-03-15T21:58:05Z

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

@@ -2231,6 +2231,10 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
        // ambiguous impls. The latter *ought* to be a
        // coherence violation, so we don't report it here.

+        if self.tcx.opt_rpitit_info(obligation.cause.body_id.to_def_id()).is_some() {


It's not generally sound to skip reporting an error here, without at least delaying a bug or something else. Can you explain why you're doing this?

spastorino · 2023-03-15T21:59:27Z

Moving forward, can you not commit a separate "Add revisions to fixed tests" commit? If any of the first 3 commits fixes a UI test, then that UI test should be associated with each commit that fixes it. If the commit doesn't fix any UI tests, then it can probably be squashed together with the other commits that fully fix a test or not.

We need all the changes to make the working tests pass. Each commit was making this tests ICE in a new part of the code :).

spastorino · 2023-03-16T20:20:10Z

@compiler-errors only the last commit is relevant. Maybe you can even fetch that one up and place it in #109198 ?

compiler-errors · 2023-03-16T20:25:54Z

my pr already got rolled up into a rollup lol

r=me when it's landed

@bors rollup

spastorino · 2023-03-16T21:45:46Z

This thing is going to fail until #109198 is merged and we rebase on top of it.

spastorino · 2023-03-17T18:34:51Z

@bors r=compiler-errors

bors · 2023-03-17T18:34:53Z

📌 Commit ae78e9b has been approved by compiler-errors

It is now in the queue for this repository.

…er-errors Add revisions for -Zlower-impl-trait-in-trait-to-assoc-ty fixed tests Needs to go on top of rust-lang#109198 r? `@compiler-errors`

spastorino · 2023-03-17T18:59:52Z

@bors r-

Something is odd here.

spastorino · 2023-03-17T22:48:00Z

Now yes ...

@bors r=compiler-errors

bors · 2023-03-17T22:48:02Z

📌 Commit e0302bb has been approved by compiler-errors

It is now in the queue for this repository.

…er-errors Add revisions for -Zlower-impl-trait-in-trait-to-assoc-ty fixed tests Needs to go on top of rust-lang#109198 r? `@compiler-errors`

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#107416 (Error code E0794 for late-bound lifetime parameter error.) - rust-lang#108772 (Speed up tidy quite a lot) - rust-lang#109193 (Add revisions for -Zlower-impl-trait-in-trait-to-assoc-ty fixed tests) - rust-lang#109234 (Tweak implementation of overflow checking assertions) - rust-lang#109238 (Fix generics mismatch errors for RPITITs on -Zlower-impl-trait-in-trait-to-assoc-ty) - rust-lang#109283 (rustdoc: reduce allocations in `visibility_to_src_with_space`) - rust-lang#109287 (Use `size_of_val` instead of manual calculation) - rust-lang#109288 (Stabilise `unix_socket_abstract`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned compiler-errors Mar 15, 2023

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 Mar 15, 2023

compiler-errors reviewed Mar 15, 2023

View reviewed changes