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.

Sign up for GitHub

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

Jump to bottom

orphan check: rationalize our handling of constants #99861

Merged
merged 2 commits into from
Aug 14, 2022
Merged

orphan check: rationalize our handling of constants #99861

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9b21b0
orphan check: remove const generics fixme
lcnr Jul 28, 2022
2634309
update comment
lcnr Jul 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion compiler/rustc_trait_selection/src/traits/coherence.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,8 +746,17 @@ impl<'tcx> TypeVisitor<'tcx> for OrphanChecker<'tcx> {
result
}

// FIXME: Constants should participate in orphan checking.
fn visit_const(&mut self, _c: ty::Const<'tcx>) -> ControlFlow<Self::BreakTy> {
// 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 •
edited
Loading

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

//
// Because there's no way to have an impl where the first local
// generic argument is a constant, we also don't have to fail
// the orphan check when encountering a parameter or a generic constant.
//
// This means that we can completely ignore constants during the orphan check.
//
// See `src/test/ui/coherence/const-generics-orphan-check-ok.rs` for examples.
ControlFlow::CONTINUE
}
}
1 change: 1 addition & 0 deletions src/test/ui/coherence/auxiliary/trait-with-const-param.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub trait Trait<const N: usize, T> {}
28 changes: 28 additions & 0 deletions src/test/ui/coherence/const-generics-orphan-check-ok.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// check-pass
// aux-build:trait-with-const-param.rs
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 •
edited
Loading

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

struct Local1;
impl<const N: usize, T> Trait<N, T> for Local1 {}

// Concrete consts behave the same as foreign types,
// so this also trivially works.
impl Trait<3, Local1> for i32 {}

// This case isn't as trivial as we would forbid type
// parameters here, we do allow const parameters though.
//
// The reason that type parameters are forbidden for
// `impl<T> Trait<T, LocalInA> for i32 {}` is that another
// downstream crate can add `impl<T> Trait<LocalInB, T> for i32`.
// As these two impls would overlap we forbid any impls which
// have a type parameter in front of a local type.
//
// With const parameters this issue does not exist as there are no
// constants local to another downstream crate.
struct Local2;
impl<const N: usize> Trait<N, Local2> for i32 {}

fn main() {}