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

Derive noop impls of TypeFoldable/Visitable #593

Closed
1 of 3 tasks
eggyal opened this issue Feb 20, 2023 · 4 comments
Closed
1 of 3 tasks

Derive noop impls of TypeFoldable/Visitable #593

eggyal opened this issue Feb 20, 2023 · 4 comments
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@eggyal
Copy link

eggyal commented Feb 20, 2023

Proposal

Move "trivial" (no-op) implementations of TypeFoldable and TypeVisitable from macro-by-example (currently rustc_middle::macros::TrivialTypeTraversalImpls into the existing derive macros when a new attribute (such as #[contains_nothing_interesting_for_traversers], still subject to bike-shedding) is present.

This work has already been undertaken as part of rust-lang/rust#108214—but because inappropriate use of that attribute will generate impls that are erroneously no-ops, I think (though I'm far from certain) it could result in unsoundness. Accordingly, @RalfJung was rightly concerned that this change is not without risk and for that reason I thought an MCP may be wise.

For historical context, the existing TrivialTypeTraversalImpls macro started out as CopyImpls in 8403b82. The current comment that types need implement Copy appears to be a hangover from that, and ceased being relevant when folding was changed to take self by value in rust-lang/rust#78313; the comment that they must not care (sic) arena allocated data is more relevant, but I think it is more correct to say they must not transitively contain types that may be of interest to folders/visitors: other arena-allocated data is irrelevant, and those interesting types would not cease being interesting if they were no longer arena-allocated.

Mentors or Reviewers

The implementing PR itself was not motivated by this change, but rather by a move to generalise such no-op implementations over the interner. This was suggested to me by @oli-obk, who I therefore already nominated as the reviewer for the PR.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@eggyal eggyal added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Feb 20, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2023

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Feb 20, 2023
@eggyal
Copy link
Author

eggyal commented Feb 20, 2023

I should add that in rust-lang/rust#108040 I already added support for fields to be ignored during derived folding/visiting. This issue raises the question of whether that too is dangerous, perhaps moreso than the entire derivation being a no-op (because removing the attributes from such fields may get overlooked if an "interesting type" is added into the field's type). It did however have the advantage of not requiring the field's type to implement the traversable traits only for those implementations to be no-ops.

@eggyal
Copy link
Author

eggyal commented Feb 23, 2023

In the Zulip topic for this MCP, @lcnr suggested using an auto-trait to blanket impl no-op traversals for types that do not contain the types of interest, and require explicit implementation (typically using the derive macros) for types that do contain the types of interest.

This is an excellent idea, but generic types might or might not contain types of interest (depending on the concrete types with which they are parameterised); we need some way for the compiler to know that an explicitly implemented traversal only applies when the instantiation does contain types of interest... and this requires some form of negative reasoning or disjoint impls.

I actually managed to solve that with a hack that (ab)uses a few unstable features including GCE. It may be it's only permitted due to one or more unsoundness holes in those features, I'm not sure: it certainly feels a bit dubious and contrary to what the type system otherwise permits. But it'd be really fantastic if we could accept an auto-trait solution based on that (or some other) approach.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 24, 2023
@eggyal
Copy link
Author

eggyal commented Feb 26, 2023

With huge thanks to @lcnr, we now have a solution using auto-deref specialisation so I'm going to close this MCP as no longer required.

@eggyal eggyal closed this as not planned Won't fix, can't repro, duplicate, stale Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

3 participants