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

false positive implied_bounds_entailment lint #108544

Closed
aliemjay opened this issue Feb 27, 2023 · 5 comments · Fixed by #109356
Closed

false positive implied_bounds_entailment lint #108544

aliemjay opened this issue Feb 27, 2023 · 5 comments · Fixed by #109356
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Milestone

Comments

@aliemjay
Copy link
Member

aliemjay commented Feb 27, 2023

The lint is triggered by the following code (from #108345, cc @EqualMa) :

use std::borrow::Cow;

pub trait Trait {
    fn method(self) -> Option<Cow<'static, str>>
    where
        Self: Sized;
}

impl<'a> Trait for Cow<'a, str> {
    fn method(self) -> Option<Cow<'static, str>> {
        None
    }
}

In the environment <'a> where Cow<'a, str>: Sized, the type Option<Cow<'static, str>> surprisingly requires 'a == 'static for well-formedness (see #108345 (comment)), but this constraint is not used in implied bounds, so I believe it can't be exploited for unsoundness.

The lint was upgraded to deny-by-default in the current beta (1.68.0-beta.5 2023-02-15 003b6f2). So this counts as regression?

cc @compiler-errors.

@rustbot label C-bug T-types T-compiler regression-from-stable-to-beta

@rustbot rustbot added C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 27, 2023
@compiler-errors
Copy link
Member

🤦

I think this is due to us preferring param-env candidates for Sized.

@compiler-errors
Copy link
Member

compiler-errors commented Feb 27, 2023

So the issue here is that WF(Option<Cow<'static, T>>) requires Cow<'static, T>: Sized.

We try to prove that in the hybrid param-env of the impl's WC + trait method's WC, being Cow<'a, T>: Sized after substituting Self.

Due to some frustrating precedence rules around candidate assembly in the trait solver, we try to match Cow<'static, T>: Sized against the param-env candidate Cow<'a, T>: Sized and get 'a == 'static. We also assemble a built-in sized bound, but always prefer the param-env candidate over that.

That requires us to prove 'a: 'static, which is not known to be true in the hybrid param-env, triggering the lint.

@compiler-errors
Copy link
Member

I wonder if we could get away with not checking WF(return-type), but just the implied lifetime bounds or something 🤔

@apiraino
Copy link
Contributor

apiraino commented Mar 1, 2023

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 1, 2023
@Mark-Simulacrum Mark-Simulacrum added this to the 1.68.0 milestone Mar 3, 2023
@jackh726
Copy link
Member

Been thinking about this a little bit. One thought I had was to check that the impl method is WF given only the trait method, but I think that would break this example:

pub trait Trait<'a, 'b> {
    fn method(self, _: &'static &'static ())
    where
        'a: 'b;
}

impl<'a> Trait<'a, 'static> for () {
    fn method(self, _: &'static &'a ()) {}
}

I wonder if we could get away with not checking WF(return-type), but just the implied lifetime bounds or something 🤔

This is annoying difficult, because it's not straightforward to get "just the implied lifetime bounds" of a type. I'm playing around with using wf::obligations() and filtering outlives bounds, but that can generate more WellFormed predicates (which, I thought we had somewhere a function that recurses though those). We'll see if this works...

@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels May 26, 2023
@bors bors closed this as completed in f1b8548 Aug 12, 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. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
6 participants