Allow impl on projection #107263

GuillaumeGomez · 2023-01-24T14:56:20Z

Got this one from #103985 as well.

Issue (solved) with privacy check

However, a bug appeared from the privacy check as the CI will show: it has some false positive with "private type in public interface". Not sure why though. The backtrace for it is here:

  12:     0x7fef586494c5 - <rustc_privacy[dc2692dda3b059d]::SearchInterfaceForPrivateItemsVisitor as rustc_privacy[dc2692dda3b059d]::DefIdVisitor>::visit_def_id
  13:     0x7fef5863f6ff - <rustc_privacy[dc2692dda3b059d]::DefIdVisitorSkeleton<rustc_privacy[dc2692dda3b059d]::SearchInterfaceForPrivateItemsVisitor> as rustc_middle[2d95345cc9d1265e]::ty::visit::TypeVisitor>::visit_ty
  14:     0x7fef5863f9d2 - <rustc_privacy[dc2692dda3b059d]::DefIdVisitorSkeleton<rustc_privacy[dc2692dda3b059d]::SearchInterfaceForPrivateItemsVisitor> as rustc_middle[2d95345cc9d1265e]::ty::visit::TypeVisitor>::visit_ty
  15:     0x7fef586484c2 - <rustc_privacy[dc2692dda3b059d]::SearchInterfaceForPrivateItemsVisitor>::ty
  16:     0x7fef586497c5 - <rustc_privacy[dc2692dda3b059d]::PrivateItemsInPublicInterfacesChecker>::check_assoc_item
  17:     0x7fef5864bba5 - rustc_privacy[dc2692dda3b059d]::check_private_in_public
  18:     0x7fef59c15a66 - rustc_query_system[a3c6a6fb37b6854f]::query::plumbing::try_execute_query::<rustc_query_impl[eee68f6de69b6b9]::queries::check_private_in_public, rustc_query_impl[eee68f6de69b6b9]::plumbing::QueryCtxt>
  19:     0x7fef598fc292 - <rustc_query_impl[eee68f6de69b6b9]::Queries as rustc_middle[2d95345cc9d1265e]::ty::query::QueryEngine>::check_private_in_public
  20:     0x7fef58179756 - <core[3d15cbc1632fd665]::panic::unwind_safe::AssertUnwindSafe<rustc_interface[2af05e4c67454c01]::passes::analysis::{closure#5}::{closure#1}> as core[3d15cbc1632fd665]::ops::function::FnOnce<()>>::call_once
  21:     0x7fef580e26c5 - <rustc_session[da714fc9e163a7bd]::session::Session>::time::<(), rustc_interface[2af05e4c67454c01]::passes::analysis::{closure#5}>
  22:     0x7fef580b9dac - rustc_interface[2af05e4c67454c01]::passes::analysis
  23:     0x7fef59c644b2 - rustc_query_system[a3c6a6fb37b6854f]::query::plumbing::try_execute_query::<rustc_query_impl[eee68f6de69b6b9]::queries::analysis, rustc_query_impl[eee68f6de69b6b9]::plumbing::QueryCtxt>
  24:     0x7fef598cfb95 - <rustc_query_impl[eee68f6de69b6b9]::Queries as rustc_middle[2d95345cc9d1265e]::ty::query::QueryEngine>::analysis
  25:     0x7fef57f91c03 - <rustc_interface[2af05e4c67454c01]::passes::QueryContext>::enter::<rustc_driver[de5d577c9fbc1a01]::run_compiler::{closure#1}::{closure#2}::{closure#2}, core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>>
  26:     0x7fef57f7383a - <rustc_interface[2af05e4c67454c01]::queries::QueryResult<rustc_interface[2af05e4c67454c01]::passes::QueryContext>>::enter::<core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>, rustc_driver[de5d577c9fbc1a01]::run_compiler::{closure#1}::{closure#2}::{closure#2}>
  27:     0x7fef57fddc76 - <rustc_interface[2af05e4c67454c01]::interface::Compiler>::enter::<rustc_driver[de5d577c9fbc1a01]::run_compiler::{closure#1}::{closure#2}, core[3d15cbc1632fd665]::result::Result<core[3d15cbc1632fd665]::option::Option<rustc_interface[2af05e4c67454c01]::queries::Linker>, rustc_errors[c6c462034af138b3]::ErrorGuaranteed>>
  28:     0x7fef5800406a - rustc_span[29a73c65a118f2f3]::with_source_map::<core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>, rustc_interface[2af05e4c67454c01]::interface::run_compiler<core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>, rustc_driver[de5d577c9fbc1a01]::run_compiler::{closure#1}>::{closure#0}::{closure#0}>
  29:     0x7fef57fd44c5 - <scoped_tls[535ca24a62cbd8c4]::ScopedKey<rustc_span[29a73c65a118f2f3]::SessionGlobals>>::set::<rustc_interface[2af05e4c67454c01]::interface::run_compiler<core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>, rustc_driver[de5d577c9fbc1a01]::run_compiler::{closure#1}>::{closure#0}, core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>>
  30:     0x7fef57f9ad30 - std[1177681641634ed6]::sys_common::backtrace::__rust_begin_short_backtrace::<rustc_interface[2af05e4c67454c01]::util::run_in_thread_pool_with_globals<rustc_interface[2af05e4c67454c01]::interface::run_compiler<core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>, rustc_driver[de5d577c9fbc1a01]::run_compiler::{closure#1}>::{closure#0}, core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>>
  31:     0x7fef57f7bdbd - <<std[1177681641634ed6]::thread::Builder>::spawn_unchecked_<rustc_interface[2af05e4c67454c01]::util::run_in_thread_pool_with_globals<rustc_interface[2af05e4c67454c01]::interface::run_compiler<core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>, rustc_driver[de5d577c9fbc1a01]::run_compiler::{closure#1}>::{closure#0}, core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[3d15cbc1632fd665]::result::Result<(), rustc_errors[c6c462034af138b3]::ErrorGuaranteed>>::{closure#1} as core[3d15cbc1632fd665]::ops::function::FnOnce<()>>::call_once::{shim:vtable#0}

cc @compiler-errors
r? @oli-obk

tests/ui/projection/impl-projection-on-primitive.stderr

GuillaumeGomez · 2023-01-24T19:37:06Z

Fixed the privacy bug. With this it should be complete.

GuillaumeGomez · 2023-01-24T19:44:42Z

I added another UI tests for implementations on foreign types (not primitive types).

compiler-errors · 2023-01-24T19:45:48Z

tests/ui/projection/impl-projection.rs

+
+pub struct S;
+
+impl <S as Identity>::Identity {


Sorry, this is what I wanted to mention. I'm not sure if we want to be able to normalize impl headers when considering inherent impls.

That's also something I wondered but after talking with @oli-obk, they seemed to think it was ok so I made this PR. I think it'll require an FCP in any case so you can see with your team whether or not it's something you want.

My words were "why should we not allow it", I don't know the answer to that. Mostly I don't know the use case for it

Sorry for the confusion. Well in any case, I'll let your team discuss about it.

I remember having written code exactly like that before and then being surprised that it didn't compile. I can't remember where that was and I must've found a different approach, but it seems like something that is consistent with how other parts of the language works. And it's kind of surprising/unexpected that it doesn't compile.

tests/ui/privacy/private-in-public-ill-formed.rs

compiler/rustc_hir_analysis/src/coherence/inherent_impls.rs

oli-obk · 2023-01-25T11:02:02Z

compiler/rustc_hir_analysis/src/coherence/inherent_impls.rs

@@ -186,7 +186,11 @@ impl<'tcx> InherentCollect<'tcx> {
            return;
        };

-        let self_ty = self.tcx.type_of(item.owner_id);
+        let mut self_ty = self.tcx.type_of(item.owner_id);
+        if matches!(self_ty.kind(), ty::Alias(..)) {


I guess this should either specifically check for projections or not have any check at all. The logic in rustc_privacy should be kept in sync with this

If I remove the check, I have this panic:

error[E0391]: cycle detected when finding all inherent impls defined in crate | = note: ...which requires normalizing `I8<{i8::MIN}>`... note: ...which requires evaluating type-level constant... --> fake-test-src-base/symbol-names/const-generics.rs:20:9 | LL | impl I8<{i8::MIN}> { | ^^^^^^^^^

Also, doesn't it make sense to normalize it for type aliases too?

And final question: you mean that if we keep this check, we should have the same in privacy where I added it (just to confirm so I can update accordingly).

If I remove the check, I have this panic:

that's an error, and I think it can be avoided by using the try_normalize variant, just like at the other normalization added in this PR. The check you are doing is useless if the projection resolves to a type that would have errored without your check (as normalization is recursive/deep)

Also, doesn't it make sense to normalize it for type aliases too?

there are no type aliases in the type system yet ;) that's your other PR

you mean that if we keep this check, we should have the same in privacy where I added it (just to confirm so I can update accordingly).

I just mean that they should work as similarly as possible, so if both use the try variant, that seems okay.

petrochenkov · 2023-01-25T12:03:00Z

Could you assign this PR to me when it's otherwise ready?
The relevant logic in rustc_privacy is subtle and I need to refresh it in memory and check whether it's still correct.

GuillaumeGomez · 2023-01-25T13:54:12Z

So we discovered that the normalization bug also appeared on this code:

pub trait Identity {
    type Identity: ?Sized;
}

impl<T: ?Sized> Identity for T {
    type Identity = Self;
}

pub struct I8<const F: i8>;

impl <I8<{i8::MIN}> as Identity>::Identity {
    pub fn foo(&self) {}
}

meaning that the if matches!(...) condition is completely useless and that the normalization bug should be fixed instead. Before digging into it, I'll wait to know if the team wants this feature or not.

GuillaumeGomez · 2023-02-08T16:40:48Z

We discovered a use case in gtk-rs.

#[properties(wrapper_type = super::Foo)] is only needed so we can do impl super::Foo { ... }. Ideally we would just do impl <Foo as glib::ObjectSubclass>::Type { ... } from this code:

https://gtk-rs.org/gtk-rs-core/git/docs/glib_macros/derive.Properties.html#example

cc @sdroege

…tions, r=oli-obk Add ui test for implementation on projection The error in full can be seen in rust-lang#107263 and is part of why the PR is blocked (it still requires the approval from the team for supporting it). r? `@oli-obk`

…tions, r=oli-obk Add ui test for implementation on projection The error in full can be seen in rust-lang#107263 and is part of why the PR is blocked (it still requires the approval from the team for supporting it). r? ``@oli-obk``

bors · 2023-02-14T03:32:44Z

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

lcnr · 2023-05-08T14:43:09Z

going to talk about this in a types team deep dive rust-lang/types-team#91

rustbot assigned oli-obk Jan 24, 2023

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 Jan 24, 2023

compiler-errors reviewed Jan 24, 2023

View reviewed changes

tests/ui/projection/impl-projection-on-primitive.stderr Show resolved Hide resolved

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the allow-impl-on-projection branch 2 times, most recently from 6d33252 to 075fb58 Compare January 24, 2023 19:36

GuillaumeGomez force-pushed the allow-impl-on-projection branch from 075fb58 to 836bf7b Compare January 24, 2023 19:43

compiler-errors reviewed Jan 24, 2023

View reviewed changes

tests/ui/privacy/private-in-public-ill-formed.rs Outdated Show resolved Hide resolved

compiler/rustc_hir_analysis/src/coherence/inherent_impls.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the allow-impl-on-projection branch from 836bf7b to 30a3e71 Compare January 24, 2023 21:49

compiler-errors added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Jan 25, 2023

oli-obk reviewed Jan 25, 2023

View reviewed changes

GuillaumeGomez added 2 commits January 25, 2023 14:22

Allow to implement on projections

cd59848

Fix invalid privacy computation for associated type on impl blocks

8ecab44

GuillaumeGomez force-pushed the allow-impl-on-projection branch from 30a3e71 to 143b2d3 Compare January 25, 2023 13:22

GuillaumeGomez added 2 commits January 25, 2023 14:37

Update UI tests and add new UI tests for implementations on projections

b010dd8

Add failing UI test

45185aa

GuillaumeGomez force-pushed the allow-impl-on-projection branch from 143b2d3 to 45185aa Compare January 25, 2023 13:37

oli-obk added T-types Relevant to the types team, which will review and decide on the PR/issue. I-types-nominated Nominated for discussion during a types team meeting. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 25, 2023

GuillaumeGomez mentioned this pull request Feb 9, 2023

Add ui test for implementation on projection #107857

Merged

lcnr mentioned this pull request May 8, 2023

projections in impl headers rust-lang/types-team#91

Closed

lcnr removed the I-types-nominated Nominated for discussion during a types team meeting. label May 8, 2023

oli-obk removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2023

Dylan-DPC added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Oct 12, 2023

Dylan-DPC added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 26, 2024

oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow impl on projection #107263

Allow impl on projection #107263

GuillaumeGomez commented Jan 24, 2023 •

edited

Loading

This comment has been minimized.

GuillaumeGomez commented Jan 24, 2023

GuillaumeGomez commented Jan 24, 2023

compiler-errors Jan 24, 2023

GuillaumeGomez Jan 24, 2023 •

edited

Loading

oli-obk Jan 25, 2023

GuillaumeGomez Jan 25, 2023

sdroege Jan 25, 2023

oli-obk Jan 25, 2023

GuillaumeGomez Jan 25, 2023

oli-obk Jan 25, 2023

petrochenkov commented Jan 25, 2023 •

edited

Loading

GuillaumeGomez commented Jan 25, 2023

GuillaumeGomez commented Feb 8, 2023 •

edited

Loading

bors commented Feb 14, 2023

lcnr commented May 8, 2023

Allow impl on projection #107263

Are you sure you want to change the base?

Allow impl on projection #107263

Conversation

GuillaumeGomez commented Jan 24, 2023 • edited Loading

This comment has been minimized.

GuillaumeGomez commented Jan 24, 2023

GuillaumeGomez commented Jan 24, 2023

compiler-errors Jan 24, 2023

Choose a reason for hiding this comment

GuillaumeGomez Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

oli-obk Jan 25, 2023

Choose a reason for hiding this comment

GuillaumeGomez Jan 25, 2023

Choose a reason for hiding this comment

sdroege Jan 25, 2023

Choose a reason for hiding this comment

oli-obk Jan 25, 2023

Choose a reason for hiding this comment

GuillaumeGomez Jan 25, 2023

Choose a reason for hiding this comment

oli-obk Jan 25, 2023

Choose a reason for hiding this comment

petrochenkov commented Jan 25, 2023 • edited Loading

GuillaumeGomez commented Jan 25, 2023

GuillaumeGomez commented Feb 8, 2023 • edited Loading

bors commented Feb 14, 2023

lcnr commented May 8, 2023

GuillaumeGomez commented Jan 24, 2023 •

edited

Loading

GuillaumeGomez Jan 24, 2023 •

edited

Loading

petrochenkov commented Jan 25, 2023 •

edited

Loading

GuillaumeGomez commented Feb 8, 2023 •

edited

Loading