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

Warn on elided lifetimes in associated constants (ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT) #115011

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

compiler-errors
Copy link
Member

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

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

This PR alternatively implements a lint against this behavior, and I'm hoping to bump this to deny in a few versions.

@rustbot
Copy link
Collaborator

rustbot commented Aug 19, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@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 Aug 19, 2023
@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot self-assigned this Aug 21, 2023
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

r=me on the implementation
I wonder if we should notify t-lang somehow.

};
self.r.lint_buffer.buffer_lint_with_diagnostic(
lint::builtin::ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT,
node_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not lifetime.id here?

Copy link
Member Author

@compiler-errors compiler-errors Aug 21, 2023

Choose a reason for hiding this comment

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

Elided lifetimes are not present in the AST, so buffered lints on that node ID (lifetime.id) will not get processed later, since that walks through the AST. This results in ICEs.

@compiler-errors
Copy link
Member Author

I consider this to be fully a compiler bug, so I'll cc @rust-lang/lang here, but I don't think there's a decision for them to make.

If we wanted to, for example, stabilize a correct behavior here (e.g. lower elided to 'static, or a fresh lifetime param(?!)) then that should be done starting from correct behavior, not "moving horizontally" here, i.e. from "accidentally works" to "stablilize".

@compiler-errors
Copy link
Member Author

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Aug 21, 2023

📌 Commit fad7d22 has been approved by cjgillot

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 Aug 21, 2023
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.
@bors
Copy link
Contributor

bors commented Aug 21, 2023

⌛ Testing commit fad7d22 with merge dcf0c926f6e689a7e93b263785ee0f95d65bd097...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 21, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 21, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 21, 2023

Some changes occurred in src/tools/rust-analyzer

cc @rust-lang/rust-analyzer

@compiler-errors
Copy link
Member Author

Sorry for the in-tree fix rust-analyzer, but can't land if RA fails CI 😅

@matthiaskrgr
Copy link
Member

Does that also fix #115068 (comment) ?

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 21, 2023
@bors
Copy link
Contributor

bors commented Aug 21, 2023

⌛ Testing commit c5f9d42fac0c5a9b00a2391bce4750f27656b294 with merge 8b702df6d255c1f5992e312e5765517a6ceccc52...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 21, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 21, 2023
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Aug 21, 2023
Copy link
Member

@lnicola lnicola left a comment

Choose a reason for hiding this comment

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

RA change seems okay.

@compiler-errors
Copy link
Member Author

whoops forgot to reapprove

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Aug 22, 2023

📌 Commit b1c609e has been approved by cjgillot

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 Aug 22, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2023
…mpiler-errors

Rollup of 6 pull requests

Successful merges:

 - rust-lang#114959 (fix rust-lang#113702 emit a proper diagnostic message for unstable lints passed from CLI)
 - rust-lang#115011 (Warn on elided lifetimes in associated constants (`ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT`))
 - rust-lang#115077 (Do not emit invalid suggestion in E0191 when spans overlap)
 - rust-lang#115087 (Add disclaimer on size assertion macro)
 - rust-lang#115090 (Always use `os-release` rather than `/lib` to detect `NixOS` (bootstrap))
 - rust-lang#115101 (triagebot: add dependency licensing pings)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e9897c3 into rust-lang:master Aug 22, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Aug 22, 2023
celinval added a commit to model-checking/kani that referenced this pull request Aug 24, 2023
tautschnig pushed a commit to tautschnig/kani that referenced this pull request Aug 30, 2023
fmease added a commit to fmease/rust that referenced this pull request Sep 2, 2023
…rough, r=cjgillot

Fall through when resolving elided assoc const lifetimes

`@QuineDot` makes a good point in rust-lang#115010 (comment) that we probably should not accept *more* code due to rust-lang#115011 even though that code will eventually become a forbid-warning in a few versions (rust-lang#115010 (comment)).

Fall through when walking thru the `AnonymousWarnToStatic` (renamed to `AnonymousWarn`) rib so that we can resolve as a fresh lifetime like we did before.
fmease added a commit to fmease/rust that referenced this pull request Sep 2, 2023
…rough, r=cjgillot

Fall through when resolving elided assoc const lifetimes

``@QuineDot`` makes a good point in rust-lang#115010 (comment) that we probably should not accept *more* code due to rust-lang#115011 even though that code will eventually become a forbid-warning in a few versions (rust-lang#115010 (comment)).

Fall through when walking thru the `AnonymousWarnToStatic` (renamed to `AnonymousWarn`) rib so that we can resolve as a fresh lifetime like we did before.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 2, 2023
…ugh, r=cjgillot

Fall through when resolving elided assoc const lifetimes

`@QuineDot` makes a good point in rust-lang#115010 (comment) that we probably should not accept *more* code due to rust-lang#115011 even though that code will eventually become a forbid-warning in a few versions (rust-lang#115010 (comment)).

Fall through when walking thru the `AnonymousWarnToStatic` (renamed to `AnonymousWarn`) rib so that we can resolve as a fresh lifetime like we did before.
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.
Warn,
"elided lifetimes cannot be used in associated constants in impls",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseError,
Copy link
Member

Choose a reason for hiding this comment

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

FWIW this FutureIncompatibilityReason will make the warning only show up in locally built crates, but not in cargo's future breakage report that also shows warnings from dependencies. Just making sure that this is a deliberate choice and not an accident. :)

Copy link
Member Author

@compiler-errors compiler-errors Sep 22, 2023

Choose a reason for hiding this comment

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

Not deliberate, but isn't it conventional to bump it later? Possibly at the same time that this lint gets bumped to Deny? Not sure if there's a definite process for how to gradually raise FCW severity 🫠

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have a standard process. :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants