-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Add a new mismatched-lifetime-syntaxes
lint
#138677
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
Add a new mismatched-lifetime-syntaxes
lint
#138677
Conversation
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
This comment has been minimized.
This comment has been minimized.
Thanks to:
|
478a98e
to
e15c083
Compare
This comment has been minimized.
This comment has been minimized.
e15c083
to
ca86221
Compare
mismatched_elided_lifetime_styles
lintmismatched-lifetime-syntaxes
lint
This comment has been minimized.
This comment has been minimized.
ca86221
to
6b4ba02
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as off-topic.
This comment was marked as off-topic.
6b4ba02
to
7ff70d4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
7ff70d4
to
3a95f66
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
3a95f66
to
1625496
Compare
This comment has been minimized.
This comment has been minimized.
1625496
to
1cd8999
Compare
Based on #138677 (comment), AFAICT this is ready on the lang side, and it looks good on the impl side, with concerns addressed. There's an outstanding non-blocking follow-up that's tracked in a separate issue. So: @bors r=traviscross,jieyouxu rollup=never Thanks a lot @shepmaster for working on this and addressing all the feedback 🩵 |
Agreed, and to echo it, thanks to @shepmaster for pushing this forward. |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 076ec59 (parent) -> ccf3198 (this PR) Test differencesShow 314 test diffsStage 1
Stage 2
Additionally, 296 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard ccf3198de316b488ee17441935182e9d5292b4d3 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (ccf3198): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 1.8%, secondary 2.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 2.6%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 750.189s -> 750.435s (0.03%) |
Those regressions are really bad... 9% on stm32f4xx-hal opt builds should amount to pretty all gains that have been painfully obtained the last 5 years... Any plans to address this? @jieyouxu Are those triggers based on |
As per the summary, this lint should not be triggered when the styles match, which makes sense. However, this lint triggers on functions taking a reference and returning a lifetimed adt when both use the 'hidden lifetimes' style. Is this intentional? In Bevy this triggers 231 warnings of this form and 0 true positives. Edit: Ah, lang team decided that for references, hidden lifetimes count as elided instead. fn function(&self) -> Lifetimed<'_>; However, this doesn't mean the following should be forbidden by this lint: fn function(&self) -> Lifetimed; Of course, it may make sense to lint against the later case, such as via a |
Does the lint documentation clarify the language team's perspective as to why those are two different categories? |
@therealprof are you seeing any other regressions in your builds or just looking at the perf report? note that the 9% is incremental, which will not be hit for cargo dependencies. |
(perf results are being discussed in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Perf.20regression.20for.20warn-by-default.20lint) |
It clarifies that a reference without a marked lifetime is classified as As far as I can tell based on the comments and meeting notes, this classification is there to avoid linting against that, while potentially stylistically incongruent, are not confusing. That this reclassification causes this lint to trigger in my second example seems unintentional, which is why I asked. |
I too wonder about if triggering this lint on Yes, in general it would seem confusing to use different syntaxes. Yes, But is the effect of these decisions a desirable one? Is forbidding/discouraging Lifetime annotations are already tricky for beginners, so does this specific case actually add any clarity for the compiler or the reader? In my opinion it does not. In fact, to a naive relative newbie such as myself, the linter proposed solution is more inconsistent: Isn't it more inconsistent to force a lifetime annotation on the output, when the input doesn't have one? |
I agree and I made the same comment some hours ago on the Zulip discussion. |
For ongoing discussion about the semantics and behavior of the lint, not performance, please use the #t-lang > Feedback on mismatched_lifetime_syntaxes Zulip thread. |
Both for making separate discussions manageable (because PR comments often get collapsed or becomes impossible to find) and visible (to not just only people subscribed to this PR), please redirect further feedback/issues to one of:
I'm going to lock this PR because it becomes impossible to follow multiple distinct discussions, all intermixed as top-level comments. |
The lang-team discussed this and I attempted to summarize their decision. The summary-of-the-summary is:
Using two different kinds of syntax for elided lifetimes is confusing. In rare cases, it may even lead to unsound code! Some examples:
Matching up references with no lifetime syntax, references with anonymous lifetime syntax, and paths with anonymous lifetime syntax is an exception to the simplest possible rule:
Having a lint for consistent syntax of elided lifetimes will make the future goal of warning-by-default for paths participating in elision much simpler.
This new lint attempts to accomplish the goal of enforcing consistent syntax. In the process, it supersedes and replaces the existing
elided-named-lifetimes
lint, which means it starts out life as warn-by-default.EDIT(jieyouxu): for the bigger picture, this lint is part of a set of lints that tries to encourage more consistent usage of lifetime syntaxes to make it less confusing for users. The language team FCP'd the overall direction for the set of lints:
(Note that "in the PR" in the quote refers to #120808, where this PR #138677 splits out from #120808 the
elided_named_lifetimes
->mismatched_lifetime_syntaxes
case.)