Skip to content

Add default and latest stable edition to --edition in rustc #106094

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

Closed
wants to merge 3 commits into from
Closed

Add default and latest stable edition to --edition in rustc #106094

wants to merge 3 commits into from

Conversation

sigaloid
Copy link
Contributor

Fixes #106041

Boxes and leaks the formatted string as per #106041 (comment)

It will now output the default edition and latest stable edition in the help message for the --edition flag.

@rustbot
Copy link
Collaborator

rustbot commented Dec 23, 2022

r? @petrochenkov

(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 Dec 23, 2022
@kadiwa4
Copy link
Contributor

kadiwa4 commented Dec 25, 2022

I think you can avoid one allocation by writing Box::leak(format!(...).into_boxed_str())

@@ -1339,7 +1339,8 @@ pub fn rustc_short_optgroups() -> Vec<RustcOptGroup> {
opt::opt_s(
"",
"edition",
"Specify which edition of the compiler to use when compiling code.",
Box::leak(format!("Specify which edition of the compiler to use when compiling code.\
Copy link
Member

Choose a reason for hiding this comment

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

Could you please avoid this Box::leak call? It is possible to invoke rustc multiple times in the same process. Each time would leak memory. You should be able to inline the opt_s function to pub fn opt_s(a: S, b: S, c: &str, d: S) -> R { to allow strings with arbitrary lifetimes and then pass &format(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - I was actually pretty wary of this change but that's what I was recommended to do. I'll rewrite the type signature and see if it still works soon so we can avoid this leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I cannot refactor this out as the reference needs to outlive 'static

error[E0521]: borrowed data escapes outside of function
    --> compiler/rustc_session/src/config.rs:1290:9
     |
1289 |     pub fn opt_s(a: S, b: S, c: &str, d: S) -> R {
     |                  -           -  - let's call the lifetime of this reference `'1`
     |                  |           |
     |                  |           `c` is a reference that is only valid in the function body
     |                  `a` declared here, outside of the function body
1290 |         stable(longer(a, b), move |opts| opts.optopt(a, b, c, d))
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |         |
     |         `c` escapes the function body here
     |         argument requires that `'1` must outlive `'static`

There's got to be something I'm missing - the enum variant for the formatted string are all static and embedded, and there's got to be a better option than either allocating, or just manually writing in the strings and commenting above the original variables, "this must be updated in xyz string". I don't know how but I think there might be a way to construct this statically somehow - it's just concatenating all-static strings.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Another option would be to use std::sync::LazyLock to compute the description. If you put it in a static it should allow you to get a single &'static str without leaking anything.

@petrochenkov
Copy link
Contributor

r? rust-lang/compiler
I'd like to take some break from reviewing.

@rustbot rustbot assigned Noratrieb and unassigned petrochenkov Jan 2, 2023
@Noratrieb
Copy link
Member

Noratrieb commented Jan 2, 2023

Thank you for your PR! But I see you accidentally added a merge commit (probably by updating your branch with the github UI). This repository doesn't allow merge commit and you should get rid of it. There's an explanation in the dev guide, feel free to ask further questions if that doesn't help.

As for the Box::leak, I would prefer the solution suggested by bjorn3 to use a LazyLock instead of leaking the string. While leaking a few bytes here isn't a problem memory wise, we should still leak as few bytes as possible to help leak detectors.

@sigaloid
Copy link
Contributor Author

sigaloid commented Jan 2, 2023

Thanks for the recommendation!

In regards to the LazyLock, are you recommending creating a static LazyLock at the top of that function and referencing it in that function? I attempted it and could not get it to become a &'static str despite my best efforts.

@bjorn3
Copy link
Member

bjorn3 commented Jan 2, 2023

Did you try &*MY_STATIC?

@Noratrieb Noratrieb added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 3, 2023
@sigaloid sigaloid closed this by deleting the head repository Jan 6, 2023
@sigaloid
Copy link
Contributor Author

sigaloid commented Jan 6, 2023

That worked @bjorn3 - thanks!

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 6, 2023
Add default and latest stable edition to --edition in rustc (attempt 2)

Fixes rust-lang#106041

No longer leaks string like my first attempt PR, rust-lang#106094 - uses LazyLock to construct a `&'static str`

It will now output the default edition and latest stable edition in the help message for the `--edition` flag.

Going to request the same reviewer as the first attempt for continuity - r? `@Nilstrieb`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

--edition should tell what the default edition is
6 participants