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

don't add implied bounds after normalization #99832

Closed
wants to merge 2 commits into from

Conversation

aliemjay
Copy link
Member

Forked from #99217 because it is a preexisting problem. cc @lcnr

We should not add implied bounds for the normalized type because we don't know for sure if the normalization holds.

Consider this example:

trait Tr {
    type Ty;
}

impl<T: 'static> Tr for T {
    type Ty = ();
}

fn test(_: <&u8 as Tr>::Ty) {}

This now fails in stable as expected, but with this change it passes!

-    type Ty = ();
+    type Ty = &'static M;

r? @ghost

lcnr and others added 2 commits July 25, 2022 18:18
this allows us to soundly use unnormalized projections for wf
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 27, 2022
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
---- [ui] src/test/ui/nll/issue-52057.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/nll/issue-52057.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "-O" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Cstrip=debuginfo" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/nll/issue-52057/a" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/nll/issue-52057/auxiliary"
stdout: none
--- stderr -------------------------------
error[E0311]: the parameter type `I` may not live long enough
   |
note: the parameter type `I` must be valid for the anonymous lifetime defined here...
   |
LL |     fn parse_first(_: &mut Self::Input) {}
   |                       ^^^^^^^^^^^^^^^^
Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
   = note: ...so that the type `I` will meet its required lifetime bounds
help: consider adding an explicit lifetime bound...
   |
LL | impl<'a, I: 'b, P: ?Sized> Parser for &'a mut P

error: aborting due to previous error
------------------------------------------

@jackh726
Copy link
Member

Is this only a problem in #99217, not on master? If so, this needs to be merged into that PR.

I don't think we should not treat normalized types as implied, but rather I think we need to fully prove (including region obligations) that the projection ty normalizes.

@lcnr
Copy link
Contributor

lcnr commented Jul 29, 2022

Is this only a problem in #99217, not on master? If so, this needs to be merged into that PR.

It's a problem on master, or well, I am not sure if it is a problem, because while surprising, i wasn't able to cause ub with it

@aliemjay
Copy link
Member Author

aliemjay commented Jul 29, 2022

I agree this is not a soundness problem. Moreover I think with #99217 we can even ignore the region obligations generated by normalizing <&u8 as Tr>::Ty when borrow-checking test because we can be sure that they're checked by the caller.

It was an experiment to resolve this surprising behavior, but I missed the case where the projection is normalized to a type parameter in src/test/ui/nll/issue-52057.rs. Closing.

@aliemjay aliemjay closed this Jul 29, 2022
@aliemjay aliemjay deleted the patch-3 branch July 29, 2022 12:05
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 10, 2023
Fix implied outlives bounds logic for projections

The logic here is subtly wrong. I put a bit of an explanation in a767d7b5165cea8ee5cbe494a4a636c50ef67c9c.

TL;DR: we register outlives predicates to be proved, because wf code normalizes projections (from the unnormalized types) to type variables. This causes us to register those as constraints instead of implied. This was "fine", because we later added that implied bound in the normalized type, and delayed registering constraints. When I went to cleanup `free_region_relations` to *not* delay adding constraints, this bug was uncovered.

cc. `@aliemjay` because this caused your test failure in rust-lang#99832 (I only realized as I was writing this)

r? `@nikomatsakis`
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants