Check that impl method args are WF given implied-bounds from trait method #105483

compiler-errors · 2022-12-09T01:56:22Z

Check out my explanation of the issue here: #105295 (comment).

Basically, this PR registers some additional well-formed bounds to ensure that the argument implied bounds of an impl method are not stronger than those coming from the trait method.

r? types

This arguably could live in wfcheck, but it'd be somewhat of a pain to move it there since we need the hybrid param-env we construct above in compare_predicate_entailment. Also, this really isn't a well-formedness check of the impl method -- this really is a form of (implied bounds) predicate entailment comparison...

compiler-errors · 2022-12-09T01:57:28Z

compiler/rustc_hir_analysis/src/check/compare_method.rs

+            tcx,
+            cause.clone(),
+            param_env,
+            ty::Binder::dummy(ty::PredicateKind::WellFormed(arg.into())),


🤔 the only question I guess I have is whether or not we need to also check that the unnormalized impl arg is also WF...

Yes, almost certainly we do.

compiler-errors · 2022-12-09T01:59:50Z

this possibly needs a crater run...

@bors try

bors · 2022-12-09T01:59:59Z

⌛ Trying commit 66da457c472cca69d31a1b8abbbdd37592c63741 with merge 1055f710142b5b63c86f756e7b7fd3415974d1e8...

compiler-errors · 2022-12-09T03:25:56Z

@bors try

bors · 2022-12-09T03:26:05Z

⌛ Trying commit 503ac38db52ff3f72240e32fe5614909b28c6a4b with merge d4af306fdd6ab261dfeb0aa57301d08b31b2dd92...

compiler/rustc_hir_analysis/src/check/compare_method.rs

bors · 2022-12-09T05:39:33Z

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

compiler-errors · 2022-12-09T08:08:32Z

@craterbot check

craterbot · 2022-12-09T08:08:41Z

👌 Experiment pr-105483 created and queued.
🤖 Automatically detected try build d4af306fdd6ab261dfeb0aa57301d08b31b2dd92
🔍 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

lcnr

awaiting crater, FCP, and the change to only check WF(unnormalized_ty), r=me

craterbot · 2022-12-09T09:39:37Z

🚧 Experiment pr-105483 is now running

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

aliemjay · 2022-12-09T09:49:51Z

src/test/ui/implied-bounds/impl-implied-bounds-compatibility.rs

@@ -0,0 +1,18 @@
+use std::cell::RefCell;


Another test should be added for checking unnormalized types. Remarkably, such test can still be relevant even if we require equivalence between impl_fty and trait_fty, instead of the subtyping we currently do.

Here is a one derived from #105295 (comment):

trait Project { type Ty; } impl Project for &'_ &'_ () { type Ty = (); } trait Trait { fn get<'s>(s: &'s str, _: ()) -> &'static str; } impl Trait for () { fn get<'s>(s: &'s str, _: <&'static &'s () as Project>::Ty) -> &'static str { s } } fn main() { let val = <() as Trait>::get(&String::from("blah blah blah"), ()); println!("{}", val); }

aliemjay · 2022-12-09T13:23:03Z

This will break the following sound code because of #105495:

trait Trait {
    fn get(_: &'static &'static ());
}
impl<'a, 'b> Trait for &'a &'b () {
    fn get(_: &'a &'b ()) {}
}

We will probably need to fix #105495 first if this pattern appears in the wild.

craterbot · 2022-12-10T07:31:43Z

🎉 Experiment pr-105483 is completed!
📊 3190 regressed and 14 fixed (249769 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 · 2022-12-10T20:03:03Z

I'm gonna retry crater with @aliemjay's suggestion of assuming the impl's types are WF. Though here's an example failure that still fails (from biogarden):

struct Sequence;

impl<Idx> std::ops::Index<Idx> for Sequence
where
    Idx: std::slice::SliceIndex<[u8]>,
{
    type Output = Idx::Output;

    fn index(&self, index: Idx) -> &Self::Output {
        todo!()
    }
}

impl<'a,Idx> std::ops::Index<Idx> for &'a Sequence
where
    Idx: std::slice::SliceIndex<[u8]>,
{
    type Output = Idx::Output;

    fn index(&self, index: Idx) -> &'a Self::Output {
        todo!()
    }
}

I haven't sat down to think about how this could be weaponized, but I'm pretty sure the compiler is right rejecting this.

Haven't looked at the rest of the failures, but I'll probably have a better idea after this next crater run is finished.

compiler-errors · 2022-12-10T20:03:30Z

@bors try

bors · 2022-12-10T20:03:38Z

⌛ Trying commit 8c1df5a873dd63146cf7df01b9c4e374fbd6aa14 with merge bb2bccfb91f43c4b6f3320bbbfc96310078e139a...

oli-obk · 2022-12-11T19:43:02Z

Are there any recently (in the last two years?) updated crates affected by this?

Can we do a future incompat lint with reasonable effort?

compiler-errors · 2022-12-11T21:12:40Z

@oli-obk, yes, it can be made into a lint first by forking the inference context. I'll put that into a separate PR -- since it's a deny-by-default lint, I'm not sure if that needs an FCP?

comex · 2022-12-12T06:47:11Z

The biggest fallout of this change is that rustc_serialize now fails to compile. It has been deprecated for about six years, and was recently archived on github, so it's not possible to backwards-compatibly fix this unless we ask T-libs, I think.

GitHub repos can be un-archived, and even if that's not possible for some reason, it should still be possible to update the copy on crates.io.

compiler-errors · 2022-12-19T23:14:18Z

@bors try @rust-timer queue

bors · 2022-12-19T23:15:00Z

⌛ Trying commit 45d1e92 with merge d3e5f0ca41a62a336b366dad05ad6232129ba294...

bors · 2022-12-20T01:28:45Z

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

rust-timer · 2022-12-20T03:05:49Z

Finished benchmarking commit (d3e5f0ca41a62a336b366dad05ad6232129ba294): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.3%, 2.1%]	9
Regressions ❌ (secondary)	0.7%	[0.4%, 0.9%]	6
Improvements ✅ (primary)	-1.0%	[-1.1%, -0.9%]	2
Improvements ✅ (secondary)	-1.5%	[-2.6%, -0.4%]	10
All ❌✅ (primary)	0.6%	[-1.1%, 2.1%]	11

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.2%	[2.2%, 2.2%]	1

Add `IMPLIED_BOUNDS_ENTAILMENT` lint Implements a lint (rust-lang#105572) version of the hard-error introduced in rust-lang#105483. Context is in that PR. r? `@lcnr` cc `@oli-obk` who had asked for this to be a lint first Not sure if this needs to be an FCP, since it's a lint for now.

compiler-errors · 2022-12-20T04:22:21Z

@rfcbot cancel

rfcbot · 2022-12-20T04:22:30Z

@compiler-errors proposal cancelled.

compiler-errors · 2022-12-20T04:22:54Z

Closing this since it's landing as a lint first, will open a new PR to promote it to an error eventually.

rustbot assigned lcnr Dec 9, 2022

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 Dec 9, 2022

compiler-errors commented Dec 9, 2022

View reviewed changes

compiler-errors added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 9, 2022

compiler-errors force-pushed the impl-wf-given-trait branch from 66da457 to 503ac38 Compare December 9, 2022 03:24

aliemjay reviewed Dec 9, 2022

View reviewed changes

compiler/rustc_hir_analysis/src/check/compare_method.rs Outdated Show resolved Hide resolved

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2022

lcnr approved these changes Dec 9, 2022

View reviewed changes

aliemjay approved these changes Dec 9, 2022

View reviewed changes

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 10, 2022

compiler-errors force-pushed the impl-wf-given-trait branch from 503ac38 to 8c1df5a Compare December 10, 2022 19:59

compiler-errors mentioned this pull request Dec 11, 2022

Tracking issue for IMPLIED_BOUNDS_ENTAILMENT lint #105572

Closed

compiler-errors mentioned this pull request Dec 11, 2022

Add IMPLIED_BOUNDS_ENTAILMENT lint #105575

Merged

compiler-errors added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2022

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 19, 2022

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 20, 2022

rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Dec 20, 2022

compiler-errors closed this Dec 20, 2022

compiler-errors deleted the impl-wf-given-trait branch January 5, 2023 15:50

lcnr mentioned this pull request Jan 9, 2023

Bump IMPLIED_BOUNDS_ENTAILMENT to Deny + ReportNow #106465

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check that impl method args are WF given implied-bounds from trait method #105483

Check that impl method args are WF given implied-bounds from trait method #105483

compiler-errors commented Dec 9, 2022 •

edited

Loading

compiler-errors Dec 9, 2022

jackh726 Dec 9, 2022

compiler-errors Dec 9, 2022

compiler-errors commented Dec 9, 2022

bors commented Dec 9, 2022

compiler-errors commented Dec 9, 2022

bors commented Dec 9, 2022

bors commented Dec 9, 2022

compiler-errors commented Dec 9, 2022

craterbot commented Dec 9, 2022

lcnr left a comment

craterbot commented Dec 9, 2022

aliemjay Dec 9, 2022 •

edited

Loading

aliemjay commented Dec 9, 2022

craterbot commented Dec 10, 2022

compiler-errors commented Dec 10, 2022 •

edited

Loading

compiler-errors commented Dec 10, 2022

bors commented Dec 10, 2022

oli-obk commented Dec 11, 2022

compiler-errors commented Dec 11, 2022 •

edited

Loading

comex commented Dec 12, 2022 •

edited

Loading

compiler-errors commented Dec 19, 2022

This comment has been minimized.

bors commented Dec 19, 2022

bors commented Dec 20, 2022

This comment has been minimized.

rust-timer commented Dec 20, 2022

compiler-errors commented Dec 20, 2022

rfcbot commented Dec 20, 2022

compiler-errors commented Dec 20, 2022

Check that impl method args are WF given implied-bounds from trait method #105483

Check that impl method args are WF given implied-bounds from trait method #105483

Conversation

compiler-errors commented Dec 9, 2022 • edited Loading

compiler-errors Dec 9, 2022

Choose a reason for hiding this comment

jackh726 Dec 9, 2022

Choose a reason for hiding this comment

compiler-errors Dec 9, 2022

Choose a reason for hiding this comment

compiler-errors commented Dec 9, 2022

bors commented Dec 9, 2022

compiler-errors commented Dec 9, 2022

bors commented Dec 9, 2022

bors commented Dec 9, 2022

compiler-errors commented Dec 9, 2022

craterbot commented Dec 9, 2022

lcnr left a comment

Choose a reason for hiding this comment

craterbot commented Dec 9, 2022

aliemjay Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

aliemjay commented Dec 9, 2022

craterbot commented Dec 10, 2022

compiler-errors commented Dec 10, 2022 • edited Loading

compiler-errors commented Dec 10, 2022

bors commented Dec 10, 2022

oli-obk commented Dec 11, 2022

compiler-errors commented Dec 11, 2022 • edited Loading

comex commented Dec 12, 2022 • edited Loading

compiler-errors commented Dec 19, 2022

This comment has been minimized.

bors commented Dec 19, 2022

bors commented Dec 20, 2022

This comment has been minimized.

rust-timer commented Dec 20, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

compiler-errors commented Dec 20, 2022

rfcbot commented Dec 20, 2022

compiler-errors commented Dec 20, 2022

compiler-errors commented Dec 9, 2022 •

edited

Loading

aliemjay Dec 9, 2022 •

edited

Loading

compiler-errors commented Dec 10, 2022 •

edited

Loading

compiler-errors commented Dec 11, 2022 •

edited

Loading

comex commented Dec 12, 2022 •

edited

Loading