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

More Typefoldable/TypeVisitable cleanups #110838

Merged
merged 3 commits into from
Apr 27, 2023

Conversation

nnethercote
Copy link
Contributor

r? @lcnr

@rustbot 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Apr 26, 2023
@nnethercote nnethercote changed the title More folder visitable cleanups More Typefoldable/TypeVisitable cleanups Apr 26, 2023
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

one idea which you may ignore if you don't think it's worth it, apart from that r=me

}
}
// Not implemented because there's no good way to produce a new `&[T]`.
// impl<I: Interner, T: TypeFoldable<I>> TypeFoldable<I> for &[T] { ... }
Copy link
Contributor

Choose a reason for hiding this comment

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

could make this a negative impl 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work because we actually have two impls:

error[E0751]: found both positive and negative implementation of trait `rustc_type_ir::fold::TypeFoldable<context::TyCtxt<'_>>` for type `&[InlineAsmTemplatePiece]`:
  --> compiler/rustc_middle/src/mir/type_foldable.rs:31:1
   |
31 | impl<'tcx> TypeFoldable<TyCtxt<'tcx>> for &'tcx [InlineAsmTemplatePiece] {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ positive implementation here
   |
   = note: negative implementation in crate `rustc_type_ir`

error[E0751]: found both positive and negative implementation of trait `rustc_type_ir::fold::TypeFoldable<context::TyCtxt<'_>>` for type `&[rustc_span::Span]`:
  --> compiler/rustc_middle/src/mir/type_foldable.rs:40:1
   |
40 | impl<'tcx> TypeFoldable<TyCtxt<'tcx>> for &'tcx [Span] {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ positive implementation here
   |
   = note: negative implementation in crate `rustc_type_ir`

It was these two impls that triggered me to write this commit, because I wondered why there weren't corresponding TypeVisitable impls.

Copy link
Contributor

@lcnr lcnr Apr 26, 2023

Choose a reason for hiding this comment

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

that's unfortunate 😅 these impls should be unnecessary with #108214 which I am really looking forward to, but until then I guess this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I realize there is a better way to do this. I have changed the third commit, please take a look :)

@lcnr
Copy link
Contributor

lcnr commented Apr 26, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 26, 2023

📌 Commit 69cbe914f8e6080bba536a7bae3025955c759a7e has been approved by lcnr

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 Apr 26, 2023
@nnethercote nnethercote force-pushed the more-Folder-Visitable-cleanups branch from 69cbe91 to a9f6cde Compare April 26, 2023 10:36
@@ -141,6 +141,14 @@ impl<I: Interner, T: TypeVisitable<I>> TypeVisitable<I> for Vec<T> {
}
}

// This trivial implementation that returns the given slice unchanged is the
// only one that makes sense, because we can't return a new slice.
impl<I: Interner, T: TypeFoldable<I>> TypeFoldable<I> for &[T] {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't that dangerous because we just ignore T here? I would have expected the impl to require T: !TypeFoldable<I> to check that it is trivial.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, what is this impl used for? This is certainly not the right implementation (but arguably there should not be one).

Copy link
Contributor Author

@nnethercote nnethercote Apr 26, 2023

Choose a reason for hiding this comment

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

It is used for the impls that the third commit removes: &'tcx [InlineAsmTemplatePiece] and &'tcx [Span]. These are things carried over from HIR, and treated as constants.

The negative bounds suggestion is a good one, except that they're not supported?

error: negative bounds are not supported
   --> compiler/rustc_type_ir/src/structural_impls.rs:146:20
    |
146 | impl<I: Interner, T: !TypeFoldable<I>> TypeFoldable<I> for &[T] {
    |                    ^^^^^^^^^^^^^^^^^^ negative bounds are not supported

At this point I think I'll go back to the original commit which just added a comment explaining why TypeFoldable isn't impl'd for &[T].

@compiler-errors compiler-errors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 26, 2023
@nnethercote nnethercote force-pushed the more-Folder-Visitable-cleanups branch from a9f6cde to 4b85aa9 Compare April 27, 2023 06:21
@nnethercote
Copy link
Contributor Author

I have gone back to just a comment in the third commit, which is what @lcnr approved yesterday.

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Apr 27, 2023

📌 Commit 4b85aa9 has been approved by lcnr

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 Apr 27, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 27, 2023
…cleanups, r=lcnr

More `Typefoldable`/`TypeVisitable` cleanups

r? `@lcnr`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#105745 (docs(std): clarify remove_dir_all errors)
 - rust-lang#106456 (Correct `std::prelude` comment)
 - rust-lang#106599 (Change memory ordering in System wrapper example)
 - rust-lang#110838 (More `Typefoldable`/`TypeVisitable` cleanups)
 - rust-lang#110851 (compiletest: emit assembly-output header in error)
 - rust-lang#110853 (compiletest: add bpf-linker assembly support)
 - rust-lang#110878 (Add `known-bug` tests for 4 unsound issues)
 - rust-lang#110886 (`DepGraph` cleanups)
 - rust-lang#110905 (Remove invalid value from scraped-examples.md)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 57e9a4b into rust-lang:master Apr 27, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 27, 2023
@nnethercote nnethercote deleted the more-Folder-Visitable-cleanups branch April 28, 2023 06:10
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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants