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

Use derive attributes for uninteresting traversals #108040

Merged
merged 1 commit into from
Feb 15, 2023
Merged

Use derive attributes for uninteresting traversals #108040

merged 1 commit into from
Feb 15, 2023

Conversation

eggyal
Copy link
Contributor

@eggyal eggyal commented Feb 14, 2023

It appears that visiting and folding was implemented on BitMatrix solely so that the derive macros could be used on GeneratorLayout, however such implementation would not necessarily be correct for other uses (if there were any). Adding attributes to the derive macro is more correct and potentially more generally useful.

r? @oli-obk

@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. labels Feb 14, 2023
oli-obk
oli-obk requested changes Feb 14, 2023
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

These attributes should be documented. On the trait maybe? not sure where we document the derives.

compiler/rustc_macros/src/type_foldable.rs Outdated Show resolved Hide resolved
@eggyal
Copy link
Contributor Author

eggyal commented Feb 14, 2023

We can document the macro.

@eggyal eggyal force-pushed the attributes_for_uninteresting_traversals branch from 3a83e62 to f394c13 Compare February 14, 2023 14:31
@eggyal eggyal requested a review from oli-obk February 14, 2023 14:31
@oli-obk
Copy link
Contributor

oli-obk commented Feb 14, 2023

Great thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 14, 2023

📌 Commit f394c1332119cef1919795af7f1abe0460f793c0 has been approved by oli-obk

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 Feb 14, 2023
@eggyal eggyal force-pushed the attributes_for_uninteresting_traversals branch from f394c13 to 3b510e8 Compare February 14, 2023 15:10
@eggyal
Copy link
Contributor Author

eggyal commented Feb 14, 2023 •
edited
Loading

Apologies, it failed tidy check. Have re-pushed.

@bors r-

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 14, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Feb 14, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 14, 2023

📌 Commit 3b510e8 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 14, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 14, 2023
@matthiaskrgr
Rollup merge of rust-lang#108040 - eggyal:attributes_for_uninterestin…
5aaaac8 
…g_traversals, r=oli-obk

Use derive attributes for uninteresting traversals

It appears that visiting and folding was implemented on `BitMatrix` solely so that the derive macros could be used on `GeneratorLayout`, however such implementation would not necessarily be correct for other uses (if there were any).  Adding attributes to the derive macro is more correct and potentially more generally useful.

r? `@oli-obk`
This was referenced Feb 14, 2023
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 14, 2023
@bors
Auto merge of rust-lang#108056 - matthiaskrgr:rollup-oa6bxvh, r=matth…
0416b1a 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#107573 (Update the minimum external LLVM to 14)
 - rust-lang#107626 (Fix `x fix` on the standard library itself)
 - rust-lang#107673 (update ICU4X to 1.1.0)
 - rust-lang#107733 (Store metrics from `metrics.json` to CI PGO timer)
 - rust-lang#108007 (Use `is_str` instead of string kind comparison)
 - rust-lang#108033 (add an unstable `#[rustc_coinductive]` attribute)
 - rust-lang#108039 (Refactor refcounted structural_impls via functors)
 - rust-lang#108040 (Use derive attributes for uninteresting traversals)
 - rust-lang#108044 (interpret: rename Pointer::from_addr → from_addr_invalid)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9b39568 into rust-lang:master Feb 15, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 15, 2023
@eggyal eggyal deleted the attributes_for_uninteresting_traversals branch February 15, 2023 00:05
@eggyal eggyal mentioned this pull request Feb 20, 2023
Closed
3 tasks
Jarcho pushed a commit to Jarcho/rust that referenced this pull request Feb 26, 2023
@bors
Auto merge of rust-lang#108056 - matthiaskrgr:rollup-oa6bxvh, r=matth…
5a8b288 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#107573 (Update the minimum external LLVM to 14)
 - rust-lang#107626 (Fix `x fix` on the standard library itself)
 - rust-lang#107673 (update ICU4X to 1.1.0)
 - rust-lang#107733 (Store metrics from `metrics.json` to CI PGO timer)
 - rust-lang#108007 (Use `is_str` instead of string kind comparison)
 - rust-lang#108033 (add an unstable `#[rustc_coinductive]` attribute)
 - rust-lang#108039 (Refactor refcounted structural_impls via functors)
 - rust-lang#108040 (Use derive attributes for uninteresting traversals)
 - rust-lang#108044 (interpret: rename Pointer::from_addr → from_addr_invalid)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
This was referenced Nov 5, 2023
Closed
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2023
@bors
Auto merge of rust-lang#117726 - eggyal:automatically_make_trivial_ty…
793a8bf 
…pes_noop-traversable, r=<try>

Automatically make trivial types noop-traversable

This is a second reincarnation of rust-lang#108214 (or at least, what that PR ultimately became), the previous reincarnation in rust-lang#117620 having been closed accidentally.  It may be easier to review by commit, albeit there are quite a few of them.

## Terminology

I refer to:
* folds and visits as "traversals"
* the types that are folded or visited as "traversables"
* the folders and visitors as "traversers"
* traversables on which traversers can act as "interesting" (there are currently only five: binders, types, consts, regions and predicates) and all others as "trivial" (traversals of which are no-ops)
* the `TypeFoldable` and `TypeVisitable` traits (and only these traits) as the "traversable traits"

## Content

This PR:

* introduces a macro, `rustc_type_ir::noop_if_trivially_traversable` that uses [auto-deref specialisation](http://lukaskalbertodt.github.io/2019/12/05/generalized-autoref-based-specialization.html) to:
    * perform a no-op traversal on values of type `T` if the interner implements `rustc_type_ir::TriviallyTraverses<T>` (thereby obviating all need for most trivial types to implement the traversable traits—and indeed such implementations are removed) or
    * delegate to the relevant traversable trait implementation on `T` otherwise;
* introduces an auto-trait, `rustc_middle::ty::TriviallyTraversable`, that is then "unimplemented" on the interesting types (and thereby remains implemented only on, but on all, trivial types);
* implements `rustc_type_ir::TriviallyTraverses<T>` on the `TyCtxt<'tcx>` interner for all `T: rustc_middle::ty::TriviallyTraversable` (thus ensuring that, for that interner at least, trivial types do not require implementations of the traversable traits);
* updates the traversable traits' derive macros to:
    * skip fields that reference neither any generic type parameters nor, if present, the `'tcx` lifetime parameter
    * use the above specialisation macro when traversing fields
    * if the traversable is not parameterised by a `'tcx` lifetime, generate implementations that are generic over the interner;
* replaces those derive macros' distinct helper attributes (introduced in rust-lang#108040) with a unified `#[skip_traversal]` helper attribute that requires a justifying reason to be provided (this does mean that the different types of traversal can no longer be independently skipped, but nowhere is that currently required or ever expected to be);
    * the derive macros by default refuse to be implemented on trivial types as specialisation usually negates any need—but this can be overridden with a `#[skip_traversal(impl_despite_trivial_because = "<reason>")]` attribute;
* uses those derive macros in place of remaining usages of the `TrivialTypeTraversal` macros and some explicit implementations; and
* a few other minor relevant refactorings.

## Commentary

One limitation of auto-deref specialisation is that it cannot work for unconstrained generic types.  Therefore, absent real specialisation, implementations of the traversable traits must constrain their generics:
* by default, the derive macros constrain every field type `T` that references one or more generic type parameters to implementors of the relevant traversable trait—however this can be modified on a field (or variant) basis with a `#[skip_traversal(because_trivial)]` attribute so that instead the interner is constrained to implement `TriviallyTraverses<T>` (and the field is not traversed)
    * the constraints are applied to the field types rather than the generic type parameters to achieve a "[perfect derive](https://smallcultfollowing.com/babysteps/blog/2022/04/12/implied-bounds-and-perfect-derive/)" that not only avoids having to propagate `#[skip_traversal]` annotations into wrapping types but also works with associated types and other esoteric situations—however this could result in trait solver cycles if interesting generic field types are one day involved in recursive type definitions;
    * for exceptionally rare cases where traversal of an item/variant/field should be a no-op despite it being (potentially, if generic) interesting, it can be annotated with the explicit and deliberately cumbersome `#[skip_traversal(despite_potential_miscompilation_because = "<reason>")]` (which produces a no-op without adding any constraint).
* (the few remaining) explicit implementations of the traversable traits over generic types are similarly constrained
    * indeed, for generic tuples *all element types* must implement the trait—however, since (most) trivial types no longer do, this unfortunately means that the implementations are not applicable to tuples with trivial elements and hence some such tuples have been replaced with more concrete newtypes that have appropriate constraints: c5e9447 is a particularly hefty example of this (where tuples containing `Span` are replaced with `Spanned<T>` instead).

r? `@lcnr`
cc `@oli-obk`

`@rustbot` label A-query-system T-compiler WG-trait-system-refactor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk Awaiting requested review from oli-obk

Assignees

@oli-obk oli-obk

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
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.
4 participants
@eggyal @oli-obk @bors @rustbot