Skip to content

return NoSolution for default assoc items #111994

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

Closed
wants to merge 1 commit into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 26, 2023

still return ambiguity in coherence as this would otherwise be unsound.

r? @BoxyUwU

@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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels May 26, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 26, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@@ -0,0 +1,26 @@
// check-pass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

want a second test:

normalizes-to is NoSolution if there is no specializing impl, projection remains rigid

normalizes-to chooses the specializing impl over the default impl where applicable

Comment on lines 140 to 186
SolverMode::Normal => return Err(NoSolution),
SolverMode::Coherence => {
return ecx.evaluate_added_goals_and_make_canonical_response(
Certainty::AMBIGUOUS,
);
}
},
Copy link
Member

Choose a reason for hiding this comment

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

Would it even be unsound to return Err(NoSolution) inside the solver during coherence, its very much true that a deafult impl will never allow us to actually normalize. For normalization outside of the solver this is wrong since we normalize shit even though we dont have to so making progress based of "this is required to actually normalize instead of just be wf" would be wrong.

@lcnr lcnr force-pushed the default-item-err branch from 21872b2 to 3f5d673 Compare June 13, 2023 13:21
@BoxyUwU
Copy link
Member

BoxyUwU commented Jun 13, 2023

I think we should have a comment on compute_projection_goal explaining that the impl shouldn't make inference progress based on the idea that normalization must occur since that would be wrong for cases where we normalize "because why not" outside of the solver. It should also probably explain that currently the impl doesn't live up to this because of normalizing from specializing impls FIXME(specialization). should probably have a similar comment in project_goals.rs in the code you changed saying that the current solution is not ideal 😅

@bors
Copy link
Collaborator

bors commented Jun 19, 2023

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

still return ambiguity in coherence as this would otherwise be unsound.
@lcnr lcnr force-pushed the default-item-err branch from 3f5d673 to be4a817 Compare June 27, 2023 07:30
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:c85c95e3d7251135ab7dc9ce3241c5835cc595a9)
Download action repository 'actions/upload-artifact@v3' (SHA:0b7f8abb1508181956e8e162db84b466c27e18ce)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: mingw-check-tidy
---
Building wheels for collected packages: reuse
  Building wheel for reuse (pyproject.toml): started
  Building wheel for reuse (pyproject.toml): finished with status 'done'
  Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=180116 sha256=351235b2326fb4db7a18e257e13ce7896c5f77339521e2c2612e71e154800a19
  Stored in directory: /tmp/pip-ephem-wheel-cache-ktlzbogx/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
  Attempting uninstall: setuptools
    Found existing installation: setuptools 59.6.0
    Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
Successfully tagged rust-ci:latest
Built container sha256:c5f2b8df33be4fe1a97a8e4921a65e4aee239fabdf1ef961871cc53f125e5e2a
Uploading finished image to https://ci-caches.rust-lang.org/docker/39289e18f1ac1b1a573d1a3c658528cab0cbfec4029e2def8ed615ecd4b1295e23213ab5b134f4a50a4bd45af0a8a4266b7571ba256dc217696d0f040efcd003

<botocore.awsrequest.AWSRequest object at 0x7f862af6ce90>
gzip: stdout: Broken pipe
xargs: docker: terminated by signal 13
[CI_JOB_NAME=mingw-check-tidy]
[CI_JOB_NAME=mingw-check-tidy]
---
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 15.49s
##[endgroup]
fmt check
##[error]Diff in /checkout/compiler/rustc_trait_selection/src/solve/project_goals.rs at line 151:
             return Err(NoSolution);
 
-        ecx.probe(
-        ecx.probe(
-            |r| CandidateKind::Candidate { name: "impl".into(), result: *r }).enter(
-            |ecx| {
-                let impl_substs = ecx.fresh_substs_for_item(impl_def_id);
-                let impl_trait_ref = impl_trait_ref.subst(tcx, impl_substs);
+        ecx.probe(|r| CandidateKind::Candidate { name: "impl".into(), result: *r }).enter(|ecx| {
+            let impl_substs = ecx.fresh_substs_for_item(impl_def_id);
+            let impl_trait_ref = impl_trait_ref.subst(tcx, impl_substs);
 
-                ecx.eq(goal.param_env, goal_trait_ref, impl_trait_ref)?;
+            ecx.eq(goal.param_env, goal_trait_ref, impl_trait_ref)?;
-                let where_clause_bounds = tcx
-                let where_clause_bounds = tcx
-                    .predicates_of(impl_def_id)
-                    .instantiate(tcx, impl_substs)
-                    .predicates
-                    .into_iter()
-                    .map(|pred| goal.with(tcx, pred));
-                ecx.add_goals(where_clause_bounds);
+            let where_clause_bounds = tcx
+                .predicates_of(impl_def_id)
+                .instantiate(tcx, impl_substs)
+                .predicates
+                .into_iter()
+                .map(|pred| goal.with(tcx, pred));
+            ecx.add_goals(where_clause_bounds);
 
-                let assoc_def = match fetch_eligible_assoc_item_def(
-                    ecx,
-                    goal.param_env,
-                    goal_trait_ref,
-                    goal.predicate.def_id(),
-                ) {
-                ) {
-                    Ok(assoc_def) => assoc_def,
-                    Err(NotAvailableReason::ErrorGuaranteed(guar)) => {
-                        let error_term = ecx.term_error_of_kind(goal.predicate.term, guar);
-                        ecx.eq(goal.param_env, goal.predicate.term, error_term)
-                            .expect("expected goal term to be fully unconstrained");
-                        return ecx.evaluate_added_goals_and_make_canonical_response(Certainty::Yes);
-                    // In case the associated item is hidden due to specialization, we have to
-                    // In case the associated item is hidden due to specialization, we have to
-                    // return ambiguity during coherence as it would otherwise be incomplete,
-                    // resulting in unsoundness (#105782).
-                    //
-                    // Outside of coherence we want to fail here as we want to treat defaulted
-                    // associated items as opaque.
-                    Err(NotAvailableReason::DefaultItem) => match ecx.solver_mode() {
-                        SolverMode::Normal => return Err(NoSolution),
-                        SolverMode::Coherence => {
-                            return ecx.evaluate_added_goals_and_make_canonical_response(
-                                Certainty::AMBIGUOUS,
-                        }
-                    },
-                };
-
-
-                if !assoc_def.item.defaultness(tcx).has_value() {
-                    let guar = tcx.sess.delay_span_bug(
-                        tcx.def_span(assoc_def.item.def_id),
-                        "missing value for assoc item in impl",
-                    );
-                    let error_term = match assoc_def.item.kind {
-                        ty::AssocKind::Const => tcx
-                            .const_error(
-                                tcx.type_of(goal.predicate.def_id())
-                                    .subst(tcx, goal.predicate.projection_ty.substs),
-                                guar,
-                            .into(),
-                            .into(),
-                        ty::AssocKind::Type => tcx.ty_error(guar).into(),
-                        ty::AssocKind::Fn => unreachable!(),
-                    };
+            let assoc_def = match fetch_eligible_assoc_item_def(
+                ecx,
+                goal.param_env,
+                goal_trait_ref,
+                goal.predicate.def_id(),
+            ) {
+            ) {
+                Ok(assoc_def) => assoc_def,
+                Err(NotAvailableReason::ErrorGuaranteed(guar)) => {
+                    let error_term = ecx.term_error_of_kind(goal.predicate.term, guar);
                     ecx.eq(goal.param_env, goal.predicate.term, error_term)
                         .expect("expected goal term to be fully unconstrained");
                     return ecx.evaluate_added_goals_and_make_canonical_response(Certainty::Yes);
##[error]Diff in /checkout/compiler/rustc_trait_selection/src/solve/project_goals.rs at line 219:
-
-
-                // Getting the right substitutions here is complex, e.g. given:
-                // - a goal `<Vec<u32> as Trait<i32>>::Assoc<u64>`
-                // - the applicable impl `impl<T> Trait<i32> for Vec<T>`
-                // - and the impl which defines `Assoc` being `impl<T, U> Trait<U> for Vec<T>`
+                // In case the associated item is hidden due to specialization, we have to
+                // return ambiguity during coherence as it would otherwise be incomplete,
+                // resulting in unsoundness (#105782).
                 //
-                // We first rebase the goal substs onto the impl, going from `[Vec<u32>, i32, u64]`
-                // to `[u32, u64]`.
-                //
-                // And then map these substs to the substs of the defining impl of `Assoc`, going
-                // from `[u32, u64]` to `[u32, i32, u64]`.
-                let impl_substs_with_gat = goal.predicate.projection_ty.substs.rebase_onto(
-                    tcx,
-                    goal_trait_ref.def_id,
-                    impl_substs,
-                let substs = ecx.translate_substs(
-                let substs = ecx.translate_substs(
-                    goal.param_env,
-                    impl_substs_with_gat,
-                    assoc_def.defining_node,
-                );
-                );
+                // Outside of coherence we want to fail here as we want to treat defaulted
+                // associated items as opaque.
+                Err(NotAvailableReason::DefaultItem) => match ecx.solver_mode() {
+                    SolverMode::Normal => return Err(NoSolution),
+                    SolverMode::Coherence => {
+                        return ecx.evaluate_added_goals_and_make_canonical_response(
+                            Certainty::AMBIGUOUS,
+                    }
+                },
+            };
 
 
-                // Finally we construct the actual value of the associated type.
-                let term = match assoc_def.item.kind {
-                    ty::AssocKind::Type => tcx.type_of(assoc_def.item.def_id).map_bound(|ty| ty.into()),
-                    ty::AssocKind::Const => bug!("associated const projection is not supported yet"),
-                    ty::AssocKind::Fn => unreachable!("we should never project to a fn"),
+            if !assoc_def.item.defaultness(tcx).has_value() {
+                let guar = tcx.sess.delay_span_bug(
+                    tcx.def_span(assoc_def.item.def_id),
+                    "missing value for assoc item in impl",
+                );
+                let error_term = match assoc_def.item.kind {
+                    ty::AssocKind::Const => tcx
+                        .const_error(
+                            tcx.type_of(goal.predicate.def_id())
+                                .subst(tcx, goal.predicate.projection_ty.substs),
+                            guar,
+                        .into(),
+                        .into(),
+                    ty::AssocKind::Type => tcx.ty_error(guar).into(),
+                    ty::AssocKind::Fn => unreachable!(),
-
-
-                ecx.eq(goal.param_env, goal.predicate.term, term.subst(tcx, substs))
+                ecx.eq(goal.param_env, goal.predicate.term, error_term)
                     .expect("expected goal term to be fully unconstrained");
-                ecx.evaluate_added_goals_and_make_canonical_response(Certainty::Yes)
-        )
-        )
+                return ecx.evaluate_added_goals_and_make_canonical_response(Certainty::Yes);
+
+
+            // Getting the right substitutions here is complex, e.g. given:
+            // - a goal `<Vec<u32> as Trait<i32>>::Assoc<u64>`
+            // - the applicable impl `impl<T> Trait<i32> for Vec<T>`
+            // - and the impl which defines `Assoc` being `impl<T, U> Trait<U> for Vec<T>`
+            //
+            // We first rebase the goal substs onto the impl, going from `[Vec<u32>, i32, u64]`
+            // to `[u32, u64]`.
+            //
+            // And then map these substs to the substs of the defining impl of `Assoc`, going
+            // from `[u32, u64]` to `[u32, i32, u64]`.
+            let impl_substs_with_gat = goal.predicate.projection_ty.substs.rebase_onto(
+                tcx,
+                goal_trait_ref.def_id,
+                impl_substs,
+            let substs = ecx.translate_substs(
+            let substs = ecx.translate_substs(
+                goal.param_env,
+                impl_substs_with_gat,
+                assoc_def.defining_node,
+            );
+
+
+            // Finally we construct the actual value of the associated type.
+            let term = match assoc_def.item.kind {
+                ty::AssocKind::Type => tcx.type_of(assoc_def.item.def_id).map_bound(|ty| ty.into()),
+                ty::AssocKind::Const => bug!("associated const projection is not supported yet"),
+                ty::AssocKind::Fn => unreachable!("we should never project to a fn"),
+
+
+            ecx.eq(goal.param_env, goal.predicate.term, term.subst(tcx, substs))
+                .expect("expected goal term to be fully unconstrained");
+            ecx.evaluate_added_goals_and_make_canonical_response(Certainty::Yes)
     }
 
     fn consider_auto_trait_candidate(
     fn consider_auto_trait_candidate(
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/rustfmt/bin/rustfmt" "--config-path" "/checkout" "--edition" "2021" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_ast_pretty/src/pprust/state/item.rs" "/checkout/compiler/rustc_ast_pretty/src/pprust/state/delimited.rs" "/checkout/compiler/rustc_trait_selection/src/traits/object_safety.rs" "/checkout/compiler/rustc_trait_selection/src/traits/coherence.rs" "/checkout/compiler/rustc_trait_selection/src/solve/weak_types.rs" "/checkout/compiler/rustc_trait_selection/src/solve/opaques.rs" "/checkout/compiler/rustc_trait_selection/src/solve/project_goals.rs" "/checkout/compiler/rustc_ast_pretty/src/pprust/state/expr.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.

@bors
Copy link
Collaborator

bors commented Jul 3, 2023

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

@apiraino
Copy link
Contributor

I think there are some pending comments @lcnr ? Switching to waiting on author

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2023
@lcnr
Copy link
Contributor Author

lcnr commented Aug 10, 2023

closing this instead 😁 I am going to ignore specialization in the new solver for as long as possible

@lcnr lcnr closed this Aug 10, 2023
@apiraino apiraino removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 12, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 25, 2025
…r-errors

handle specialization in the new trait solver

fixes rust-lang/trait-system-refactor-initiative#187
also fixes the regression in `plonky2_field` from rust-lang/trait-system-refactor-initiative#188

cc rust-lang#111994

r? `@compiler-errors`
jhpratt added a commit to jhpratt/rust that referenced this pull request Apr 26, 2025
…r-errors

handle specialization in the new trait solver

fixes rust-lang/trait-system-refactor-initiative#187
also fixes the regression in `plonky2_field` from rust-lang/trait-system-refactor-initiative#188

cc rust-lang#111994

r? ``@compiler-errors``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 26, 2025
…r-errors

handle specialization in the new trait solver

fixes rust-lang/trait-system-refactor-initiative#187
also fixes the regression in `plonky2_field` from rust-lang/trait-system-refactor-initiative#188

cc rust-lang#111994

r? ```@compiler-errors```
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 26, 2025
Rollup merge of rust-lang#140306 - lcnr:specialization-new, r=compiler-errors

handle specialization in the new trait solver

fixes rust-lang/trait-system-refactor-initiative#187
also fixes the regression in `plonky2_field` from rust-lang/trait-system-refactor-initiative#188

cc rust-lang#111994

r? ```@compiler-errors```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants