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

Resolve function lifetime elision on the AST #97313

Merged
merged 4 commits into from
Jul 25, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented May 23, 2022

Based on #97720

Lifetime elision for functions is purely syntactic in nature, so can be resolved on the AST.
This PR replicates the elision logic and diagnostics on the AST, and replaces HIR-based resolution by a delay_span_bug.

This refactor allows for more consistent diagnostics, which don't have to guess the original code from HIR.

r? @petrochenkov

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 23, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2022
@petrochenkov
Copy link
Contributor

Blocked on #96296.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2022
@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot marked this pull request as draft May 29, 2022 20:04
@bors

This comment was marked as resolved.

@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jun 3, 2022
@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2022
@cjgillot cjgillot marked this pull request as ready for review June 19, 2022 15:55
@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 19, 2022
@bors
Copy link
Contributor

bors commented Jun 19, 2022

⌛ Trying commit 5d4053639c6a30859f6001324667a3ec32e32f33 with merge a5ac7517e73160a094c58d00ac8f79b4935456d5...

@rust-log-analyzer

This comment was marked as resolved.

@bors

This comment was marked as resolved.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 19, 2022
@petrochenkov petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 19, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6dbae3a): comparison url.

Instruction count

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
0.3% 0.3% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Max RSS (memory usage)

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
3.1% 3.1% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-3.0% -3.0% 1
Improvements 🎉
(secondary)
-2.7% -2.7% 1
All 😿🎉 (primary) 0.1% 3.1% 2

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
2.6% 2.6% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

Enselic added a commit to cargo-public-api/cargo-public-api that referenced this pull request Jul 26, 2022
Our nightly CI job
[found](https://github.com/Enselic/cargo-public-api/actions/runs/2736941493)
detected an upstream change which I think is caused by
rust-lang/rust#97313. I tried to git bisect it
but it didn't want to cooperate, so I have up.

In either case, the change looks reasonable, so let's just adopt it.
@cjgillot cjgillot deleted the ast-lifetimes-anon branch July 26, 2022 17:07
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 27, 2022
…r=petrochenkov

Clean up HIR-based lifetime resolution

Based on rust-lang#97313.

Fixes rust-lang#98932.

r? `@petrochenkov`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jul 28, 2022
…enkov

Clean up HIR-based lifetime resolution

Based on rust-lang/rust#97313.

Fixes #98932.

r? `@petrochenkov`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 28, 2022
…henkov

Resolve function lifetime elision on the AST

~Based on rust-lang#97720

Lifetime elision for functions is purely syntactic in nature, so can be resolved on the AST.
This PR replicates the elision logic and diagnostics on the AST, and replaces HIR-based resolution by a `delay_span_bug`.

This refactor allows for more consistent diagnostics, which don't have to guess the original code from HIR.

r? `@petrochenkov`
bors added a commit to rust-lang/cargo that referenced this pull request Aug 11, 2023
Fix elided lifetime in associated const

Fix an unelided lifetime in an associated const.

The old code was equivalent to:

```rust
impl<'a> RegistryConfig {
    /// File name of [`RegistryConfig`].
    const NAME: &'a str = "config.json";
}
```

and not `&'static str`, as it might be in a regular `const` item.

This "regressed" in rust-lang/rust#97313, which started allowing this behavior (inadvertently, as far as I can tell). It's not necessarily clear to me that this is sound (or at least, it's not something we intended to be able to express), but it's also preventing me from doing crater runs to investigate fallout of this issue (rust-lang/rust#114713 and rust-lang/rust#114716).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 21, 2023
…oc-ct-lt, r=cjgillot

Warn on elided lifetimes in associated constants (`ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT`)

Elided lifetimes in associated constants (in impls) erroneously resolve to fresh lifetime parameters on the impl since rust-lang#97313. This is not correct behavior (see rust-lang#38831).

I originally opened rust-lang#114716 to fix this, but given the time that has passed, the crater results seem pretty bad: rust-lang#114716 (comment)

This PR alternatively implements a lint against this behavior, and I'm hoping to bump this to deny in a few versions.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Aug 22, 2023
…oc-ct-lt, r=cjgillot

Warn on elided lifetimes in associated constants (`ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT`)

Elided lifetimes in associated constants (in impls) erroneously resolve to fresh lifetime parameters on the impl since rust-lang#97313. This is not correct behavior (see rust-lang#38831).

I originally opened rust-lang#114716 to fix this, but given the time that has passed, the crater results seem pretty bad: rust-lang#114716 (comment)

This PR alternatively implements a lint against this behavior, and I'm hoping to bump this to deny in a few versions.
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2023
…oc-ct-lt, r=cjgillot

Warn on elided lifetimes in associated constants (`ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT`)

Elided lifetimes in associated constants (in impls) erroneously resolve to fresh lifetime parameters on the impl since rust-lang#97313. This is not correct behavior (see rust-lang#38831).

I originally opened rust-lang#114716 to fix this, but given the time that has passed, the crater results seem pretty bad: rust-lang#114716 (comment)

This PR alternatively implements a lint against this behavior, and I'm hoping to bump this to deny in a few versions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants