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

orphan check: rationalize our handling of constants #99861

Merged
merged 2 commits into from
Aug 14, 2022

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Jul 28, 2022

cc @rust-lang/types @rust-lang/project-const-generics on whether you agree with this reasoning.

r? types

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 28, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2022
Comment on lines 750 to 751
// All possible values for a constant parameter already exist
// in the crate defining the trait, so they are always non-local.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm is const Foo: &dyn Bar ever even possible? What about <T: Trait, const Foo: T>?

I think this statement holds today, but if it could ever change in the future, do we have some way to ensure we don't forget about this?

Copy link
Contributor Author

@lcnr lcnr Jul 29, 2022

Choose a reason for hiding this comment

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

🤔

T: Trait, const VALUE: T isn't an issue, because T has to be forward declared, so if T is a local type, then the orphan check already succeeds the moment it sees T

for trait objects that's more difficult 😁 or well, it depends on whether we already have local values for const T: fn(). going to extend that comment.

feel like the value of allowing uncovered const params is still greater then allowing impls to be guarded by a local function pointer 😁 so this impl is still the right one imo

extern crate trait_with_const_param;
use trait_with_const_param::*;

// Trivial case, const param after local type.
Copy link
Member

Choose a reason for hiding this comment

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

Wrong description? "Impl for local type"?

Copy link
Contributor Author

@lcnr lcnr Jul 29, 2022

Choose a reason for hiding this comment

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

in the substs of that impl trait ref we have [Local1, N, T], so const param after local type holds 😁

want me to change that comment? don't think that "impl for local type" is the right descr because the same also holds for impl<const N: usize, T> OtherTrait<Local1, N, T> for ForeignTy

@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 13, 2022

📌 Commit 2634309 has been approved by jackh726

It is now in the queue for this repository.

@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 13, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#99582 (Delay a span bug if we see ty/const generic params during writeback)
 - rust-lang#99861 (orphan check: rationalize our handling of constants)
 - rust-lang#100026 (Add `Iterator::array_chunks` (take N+1))
 - rust-lang#100115 (Suggest removing `let` if `const let` or `let const` is used)
 - rust-lang#100126 (rustc_target: Update some old naming around self contained linking)
 - rust-lang#100487 (`assert_{inhabited,zero_valid,uninit_valid}` intrinsics are safe)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 92344e3 into rust-lang:master Aug 14, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 14, 2022
@lcnr lcnr deleted the orphan-check-cg branch August 14, 2022 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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