[WIP] reject more impossible trivial bounds (HRTBs and trivial after normalization)

[compiler-errors]

We really should be rejecting bounds that are global, even if they have higher-ranked lifetimes. We have the ability to prove whether for<'a> &'a mut String: Clone is true or not, so there's no reason to silently allow this to typecheck, especially because it's inconsistent with a non-HRTB like &'static mut String: Clone and is the source of ICEs

For now, though, just add a warning that it may be denied in the future.

[compiler-errors]

@bors try

[bors]

⌛ Trying commit 2d05446c6dd15d6df2efbbbbd2e586666055b281 with merge eeb2054ba7aab4629538fba83b4971e554d70ee9...

[bors]

☀️ Try build successful - checks-actions
Build commit: eeb2054ba7aab4629538fba83b4971e554d70ee9 (eeb2054ba7aab4629538fba83b4971e554d70ee9)

[compiler-errors]

@craterbot check

[craterbot]

👌 Experiment pr-95611 created and queued.
🤖 Automatically detected try build eeb2054ba7aab4629538fba83b4971e554d70ee9
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[jackh726]

I don't think a crater run makes sense for just a warning. (It won't cause crates to not compile.)
If you want to know the impact, I would use an error for a crater run.

[compiler-errors]

Good point. Totally forgot that this needs to filter out crates for the signal to be visible lol

[compiler-errors]

@craterbot cancel

[craterbot]

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[compiler-errors]

@craterbot abort

[craterbot]

🗑️ Experiment pr-95611 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[compiler-errors]

@bors try

[bors]

⌛ Trying commit e23c11b4384d3f6ab0f21ac0ca9e75febd006be2 with merge 751cdbaf3a893bc71b2c617bb7be22dfea5f0862...

[bors]

☀️ Try build successful - checks-actions
Build commit: 751cdbaf3a893bc71b2c617bb7be22dfea5f0862 (751cdbaf3a893bc71b2c617bb7be22dfea5f0862)

[compiler-errors]

@craterbot check

[craterbot]

👌 Experiment pr-95611 created and queued.
🤖 Automatically detected try build 751cdbaf3a893bc71b2c617bb7be22dfea5f0862
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-95611 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-95611 is completed!
📊 15 regressed and 5 fixed (223759 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[compiler-errors]

This crater run looks relatively clean. In fact, there are only a few relevant failure cases I could find.

---

The first is in utter-step/advent-utils:

pub fn parse_file<P, T>(p: P) -> Result<Vec<T>, Box<dyn Error>>
where
    P: AsRef<Path>,
    T: FromStr<Err = Box<dyn Error>>,
    <T as FromStr>::Err: Error + 'static,
{
    let contents = read_file(p)?;

    parse_raw_data(contents)
}

This is because Box<dyn Error> does not implement Error, so this method is un-callable. This trivial bound is only revealed when we normalize <T as FromStr>::Err: Error + 'static.

---

Another failure is in one of the amadeus tests. Specifically, one of the generated methods:

#[derive(Data, Clone, PartialEq, Serialize, Deserialize, Debug)]
struct Row {
	a: String,
	b: u64,
	c: f64,
	// d: List<Row>,
	e: List<Value>,
}

#[inline(always)]
fn hash_a<H>(&self, _state: &mut H)
where
    H: __::Hasher,
    for<'a> __::Wrapper<'a, Row>: __::Hash,
{ ... }

Is not satisfied because Wrapper<'a, T>: Hash only if T: Hash, and Row is not Hash (and, in fact, can't derive Hash because of the f64 member, lol).

This API isn't even fleshed out in the Amadeus API: constellation-rs/amadeus#69
(see: some functionality -- hashing -- isn't fully implemented)

---

The rest of the crates are due to derive macros hiding unsatisfisable for<'a> &'a Struct: diesel::Identifiable bound, since they have #[derive(Associations)], and the docs for the macro say:

Both the parent and child must implement Identifiable.

Adding derive(Identifiable) to those structs fixes the errors here. Again, we're probably unable to actually use this derived trait due to the bound being unsatisfiable.

[compiler-errors]

r? @nikomatsakis (feel free to redirect)

Also, does this need T-lang sign-off?

[nikomatsakis]

We really should be rejecting bounds that are global, even if they have higher-ranked lifetimes.

I'm somewhat surprised by this statement. I actually think the opposite! I expect us to move towards permitting any where-clause, no matter how nonsensical.

[compiler-errors]

@nikomatsakis: Hm. I actually am a fan of warning the user early if their WC is nonsense, since except for macro-generated code, they're very unlikely to have written something like this. And even in macro-generated code, it sometimes reveals something that the user of the macro forgot to do, like implementing another trait they were required to implement in tandem with a derive.

It feels strange that the user would have to wait until the function/impl is used to receive feedback that their code is wrong, and somewhat inconsistent, too, since this only currently happens when HRTBs are involved.

but if not, then what's the alternative? like, should we be making the places that assume these bounds hold (after erasing lifetimes) fallible so they can bubble up errors when evaluated during codegen (or suppress them)? should we be avoiding generating MIR at all, since some MIR construction steps and optimizations ICE when given bounds like this?

Or maybe we should remove this delay_span_bug and be more careful about unwrapping the result from codegen_fulfill_obligation... 🤔

Anywho, I'd still like to fix ICEs like #94651 somehow, and remove special-casing from specific MIR optimizations like in #95398. Thoughts would be appreciated.

[nikomatsakis]

@compiler-errors

Warning early is one thing, errors are another.

Note though that with approaches like this it might be less clear whether a where-clause is actually wrong.

[nikomatsakis]

since this only currently happens when HRTBs are involved.

Yes, I would change that so that the code is always accepted. I do thing a warning is a good idea, just not hard errors.

[nikomatsakis]

Anywho, I'd still like to fix ICEs like #94651 somehow, and remove special-casing from specific MIR optimizations like in #95398. Thoughts would be appreciated.

Now this is a good question! I'd like to dig into this a bit more. I agree we should not ICE! If I recall, was part of the problem having to do with MIR inlining into a context where those where clauses were not available? We talked about this at some point.

[compiler-errors]

was part of the problem having to do with MIR inlining into a context where those where clauses were not available?

Not exactly. This is due to the fact when we have a WC like for<'a> &mut 'a (): Clone, then we try to use that to select &ReErased mut () in a RevealAll param-env, we fail a codegen_fulfill_obligation call, because the param-env gets removed due to this optimization that throws away the caller bounds.

And ICEs of this nature can show up in many different places. MIR inlining is one, but #95398 shows that it also manifests in the NoopMethodCall lint, the ConstProp MIR pass. In #94651, it's due to the MIR ElaborateDrops step. So it feels somewhat hacky to fix up all these spots, just for more to be added later.

[bors]

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

[pnkfelix]

compiler-errors says that this is still in development and should be tagged as either S-experimental or S-blocked.

@rustbot label: -S-waiting-on-review +S-experimental