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

Improve diagnostics regarding how static_in_const doesn't work for associated constants #38831

Closed
clarfonthey opened this issue Jan 4, 2017 · 11 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Jan 4, 2017

Example code:

#![feature(associated_consts, static_in_const)]
struct Test;
impl Test {
    const CONST: &str = "Hello, world!";
}

Would default CONST to &'static str if this actually worked for associated constants. The original RFC decided to not do this, so, it makes sense to document the behaviour.

EDIT: the error message should be better, not the docs.

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 5, 2017

This is intentional, but it seems like the RFC text wasn't updated to accommodate this detail.

@clarfonthey
Copy link
Contributor Author

@petrochenkov I feel that if that's the case, then the error message should be improved and the RFC text should be updated.

@petrochenkov petrochenkov added A-diagnostics Area: Messages for errors, warnings, and lints A-docs labels Feb 19, 2017
@steveklabnik steveklabnik added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools P-low Low priority and removed A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Mar 9, 2017
@steveklabnik
Copy link
Member

Doc team triage: tagging as p-low because this is for an unstable feature

@frewsxcv
Copy link
Member

frewsxcv commented May 4, 2017

Would extending static_in_const to work with associated constants be a controversial move? Is there any reason it shouldn't be done other than not explicitly being spelled out in the RFC?

@clarfonthey
Copy link
Contributor Author

I personally would be careful to only default to 'static if the impl doesn't have any lifetime parameters. But otherwise it'd make sense imho.

@clarfonthey clarfonthey changed the title static_in_const doesn't work for associated constants Document that static_in_const doesn't work for associated constants May 30, 2017
@clarfonthey
Copy link
Contributor Author

Should this still be P-low considering how this feature got stabilised? @steveklabnik

@steveklabnik
Copy link
Member

@clarcharr you're right! We should change the prioritization. I'm going to nominate this to be discussed at the docs meeting today.

@steveklabnik steveklabnik added I-nominated and removed A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools I-nominated P-low Low priority labels May 31, 2017
@steveklabnik
Copy link
Member

Doc team triage: the RFC issue in the reference repo rust-lang/reference#9 should be tracking adding this to the reference, so that takes care of the doc requirement.

As such, leaving this to diagnostics now. Removing the p-tag because that's for @rust-lang/compiler to decide.

@steveklabnik steveklabnik changed the title Document that static_in_const doesn't work for associated constants Improve diagnostics regarding how static_in_const doesn't work for associated constants May 31, 2017
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jun 2, 2017

@frewsxcv

Would extending static_in_const to work with associated constants be a controversial move? Is there any reason it shouldn't be done other than not explicitly being spelled out in the RFC?

We decided against it because, in that case, there might be other lifetimes you would want. For example:

trait Foo<'a> {
    const T: &'a str;
}

that said, revisiting this explanation, it feel a bit weak, since the value of an associated constant would still have to be all consisting of static data. So it seems like 'static is a pretty good default if you don't say otherwise.

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@shepmaster
Copy link
Member

So it seems like 'static is a pretty good default if you don't say otherwise.

Yes, please!

@clarfonthey
Copy link
Contributor Author

Going through old issues on GitHub, I think we can close this. The latest diagnostics for this seem good to me:

error[[E0106]](https://doc.rust-lang.org/stable/error-index.html#E0106): missing lifetime specifier
 [--> src/lib.rs:3:18
](https://play.rust-lang.org/#)  |
3 |     const CONST: &str = "Hello, world!";
  |                  ^ expected named lifetime parameter
  |
help: consider using the `'static` lifetime
  |
3 |     const CONST: &'static str = "Hello, world!";
  |                   +++++++
help: consider introducing a named lifetime parameter
  |
2 ~ impl<'a> Test {
3 ~     const CONST: &'a str = "Hello, world!";
  |

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue 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 issue 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 issue 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
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants