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

generator: Elide lifetimes in impl blocks whenever they are redundant #969

Closed
wants to merge 1 commit into from

Conversation

MarijnS95
Copy link
Collaborator

Depends on (and is a partial alternative for) #967, depends on #968

Rust 1.83 suggests that whenever a named lifetime 'a in an impl<'a> (Trait for) Struct<'a> is specified, but not used anywhere else within that impl block, the naming can be removed and replaced with '_.

While we can unconditionally remove this from blocks like impl TaggedStructure, more generator logic is necessary to optionally omit them on builder blocks with special care around optional presence of push_next().


It is this complicated / error-prone generator logic that I am not sure warrants this feature. We should measure if there's any compile-time effect before committing to this. If there's not, I suggest we only handle trivial cases like removing it from impl TaggedStructure for Struct<'_>, perhaps via #967.

@MarijnS95 MarijnS95 requested a review from Ralith December 3, 2024 13:37
Rust 1.83 suggests that whenever a named lifetime `'a` in an `impl<'a>
(Trait for) Struct<'a>` is specified, but not used anywhere else within
that `impl` block, the naming can be removed and replaced with `'_`.

While we can unconditionally remove this from blocks like `impl
TaggedStructure`, more generator logic is necessary to optionally omit
them on builder blocks with special care around optional presence of
`push_next()`.
@MarijnS95
Copy link
Collaborator Author

@Rua I've been considering pulling this further to solve your issue #906, but one major drawback is that non-breaking Vulkan releases may add more structures that would reference ExternalMemoryImageCreateInfo in their structextends for example. In this case a fn push_next() would be retroactively added, but would also require a breaking change to add back the omitted lifetime to ExternalMemoryImageCreateInfo.

@Ralith
Copy link
Collaborator

Ralith commented Dec 3, 2024

It is this complicated / error-prone generator logic that I am not sure warrants this feature. We should measure if there's any compile-time effect before committing to this.

I am certain that rustc desugars the elided lifetimes to the explicit version immediately, so there should be no difference.

What's the motivation for this change? Normally elision is there to make reading and maintaining code easier in simple cases, but that doesn't seem very relevant to generated code.

@MarijnS95
Copy link
Collaborator Author

Having shorter checked-in code is marginal: if you say it is desugared there's no runtime benefit at all.

I prefer fixing clippy linte rather than allowing them (in case future violations show up that are relevant to clean up, i.e. when generated code is unnecessarily verbose like impl StructureType). As mentioned, this added complexity doesn't seem to outweigh that benefit.

@Ralith
Copy link
Collaborator

Ralith commented Dec 3, 2024

It's not terribly unusual for clippy lints to be overly opinionated or make assumptions that aren't justified in all cases. I think the cost of any additional complexity in the generator is quite high, so if there's no benefit to users I'd be inclined to stick with disabling the lint.

@MarijnS95
Copy link
Collaborator Author

Agreed to definitely keep this complexity out of the generator, but I do tend to appreciate clippy pointing out shorter, more concise ways to write the same thing. It's just that we can't use it most of the time here.

@MarijnS95 MarijnS95 closed this Dec 4, 2024
@MarijnS95 MarijnS95 deleted the elide-lifetimes branch December 4, 2024 08:49
Comment on lines -2390 to +2402
unsafe impl #lifetime TaggedStructure for #name #lifetime {
unsafe impl TaggedStructure for #name #opaque_lifetime {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the only lifetime that we could reasonably and unconditionally elide, but I've opted not to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants