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

Try filtering out non-const impls when we expect const impls #87375

Merged
merged 20 commits into from
Aug 14, 2021

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Jul 22, 2021

TL;DR: Associated types on const impls are now bounded; we now disallow calling a const function with bounds when the specified type param only has a non-const impl.

r? @oli-obk

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2021
@rust-log-analyzer

This comment has been minimized.

@@ -285,6 +285,7 @@ impl AutoTraitFinder<'tcx> {
def_id: trait_did,
substs: infcx.tcx.mk_substs_trait(ty, &[]),
},
constness: hir::Constness::NotConst,
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly here, do we want Const if we are in a const context?

@@ -702,7 +702,7 @@ impl<'tcx> LowerInto<'tcx, Option<chalk_solve::rust_ir::QuantifiedInlineBound<Ru
let (predicate, binders, _named_regions) =
collect_bound_vars(interner, interner.tcx, self.kind());
match predicate {
ty::PredicateKind::Trait(predicate, _) => Some(chalk_ir::Binders::new(
ty::PredicateKind::Trait(predicate) => Some(chalk_ir::Binders::new(
binders,
chalk_solve::rust_ir::InlineBound::TraitBound(
predicate.trait_ref.lower_into(interner),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to start tracking this information in chalk, too

@oli-obk
Copy link
Contributor

oli-obk commented Jul 22, 2021

only got half way through, will look more tomorrow

@rust-log-analyzer

This comment has been minimized.

@fee1-dead fee1-dead force-pushed the move-constness-to-traitpred branch from 2ea3e1d to 663dfe6 Compare July 24, 2021 11:13
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@fee1-dead fee1-dead force-pushed the move-constness-to-traitpred branch from 6e9dbcb to 52d8288 Compare July 26, 2021 03:16
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member Author

Aaaah why do we have these cursed tests of const + async

@rust-log-analyzer

This comment has been minimized.

@fee1-dead fee1-dead force-pushed the move-constness-to-traitpred branch from 9023ae5 to 5f19f12 Compare July 26, 2021 06:48
@rust-log-analyzer

This comment has been minimized.

@fee1-dead fee1-dead marked this pull request as ready for review July 26, 2021 09:29
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 26, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

impl<K, V> BTreeMap<K, V> {
// N.B. bound not on function to avoid unnecessary
// const bound on `Ord`: see #87375
impl<K: Ord, V> BTreeMap<K, V> {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not make a difference at all. All bounds on the impl must apply for functions defined inside, too.

if this truly fixes things, that is a bug. Instead we should use a ?const Ord bound

Copy link
Member Author

@fee1-dead fee1-dead Aug 13, 2021

Choose a reason for hiding this comment

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

Hmmmm, are you saying bounds on impls should be treated as const bounds when the functions inside them are const? That would break a lot of existing code I think.

impl<T: Clone> MyType {
    fn a() { ... }; // T: Clone
    const fn b() { ... }; // T: const Clone? 
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Only code using feature gates, stable code already doesn't allow this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=428faddc9d2bce802ab7423fdfa7db33

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, I really didn't know this. But can we use ?const inside liballoc? That feature is marked as incomplete.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm... I guess we are just going to break ppl using btreemap in const contexts? Or try to remove the bound in a separate PR first?

Copy link
Member Author

@fee1-dead fee1-dead Aug 13, 2021

Choose a reason for hiding this comment

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

From the crater run I think not many people are using these functions. I don't think we should remove the bound because it might require more work when re-adding the bound back. I'm fine with breaking BTree{Map,Set}::new for now.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 14, 2021

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Aug 14, 2021

📌 Commit 74627c1 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2021
@@ -293,6 +316,18 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
self.infcx.tcx
}

/// returns `true` if the predicate is considered `const` to
Copy link

Choose a reason for hiding this comment

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

Suggested change
/// returns `true` if the predicate is considered `const` to
/// Returns `true` if the predicate is considered `const` to

Copy link
Member Author

Choose a reason for hiding this comment

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

I will change this after this lands or on this PR if CI fails

fn select_where_possible(
&mut self,
infcx: &InferCtxt<'_, 'tcx>,
) -> Result<(), Vec<FulfillmentError<'tcx>>>;

// FIXME this should not provide a default body for chalk as chalk should be updated
Copy link

Choose a reason for hiding this comment

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

Suggested change
// FIXME this should not provide a default body for chalk as chalk should be updated
// FIXME(fee1-dead): this should not provide a default body for chalk as chalk should be updated

fn prove_predicate(&mut self, mut predicate: Predicate<'tcx>) {
if let PredicateKind::Trait(mut tr) = predicate.kind().skip_binder() {
if let hir::Constness::Const = tr.constness {
// FIXME check if we actually want to prove const predicates inside AscribeUserType
Copy link

Choose a reason for hiding this comment

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

Suggested change
// FIXME check if we actually want to prove const predicates inside AscribeUserType
// FIXME(fee1-dead): check if we actually want to prove const predicates inside AscribeUserType

#[allow(dead_code)]
pub const MY_STRING: String = String::new();

// FIXME remove this struct once we put `K: ?const Ord` on BTreeMap::new.
Copy link

Choose a reason for hiding this comment

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

Suggested change
// FIXME remove this struct once we put `K: ?const Ord` on BTreeMap::new.
// FIXME(fee1-dead): remove this struct once we put `K: ?const Ord` on BTreeMap::new.

@@ -23,6 +23,10 @@
#![feature(slice_partition_dedup)]
#![feature(vec_spare_capacity)]
#![feature(string_remove_matches)]
#![feature(const_btree_new)]
#![feature(const_trait_impl)]
// FIXME remove this when const_trait_impl is not incomplete anymore
Copy link

Choose a reason for hiding this comment

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

Suggested change
// FIXME remove this when const_trait_impl is not incomplete anymore
// FIXME(fee1-dead): remove this when const_trait_impl is not incomplete anymore

@bors
Copy link
Contributor

bors commented Aug 14, 2021

⌛ Testing commit 74627c1 with merge 136eaa1...

@bors
Copy link
Contributor

bors commented Aug 14, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 136eaa1 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.