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

feat: inline const as literal #14925

Merged
merged 4 commits into from Jun 6, 2023

Conversation

viktorlott
Copy link
Contributor

@viktorlott viktorlott commented May 29, 2023

Assist: inline_const_as_literal

Evaluate and inline const variable as literal.

const STRING: &str = "Hello, World!";

fn something() -> &'static str {
    STR$0ING
}

->

const STRING: &str = "Hello, World!";

fn something() -> &'static str {
    "Hello, World!"
}

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2023
@viktorlott viktorlott force-pushed the inline-const-expr-as-static-str branch 6 times, most recently from 489d5e0 to 1e803cc Compare May 29, 2023 20:53
@viktorlott viktorlott marked this pull request as ready for review May 29, 2023 21:10
@HKalbasi
Copy link
Member

Interesting assist. Why it's name is limited to string literals? I think the current code works with integers and other kinds of literals without any changes, and with some trivial changes it could support arrays and tuples (supporting structs and enums are a little harder I think)

@viktorlott
Copy link
Contributor Author

viktorlott commented May 30, 2023

Interesting assist. Why it's name is limited to string literals? I think the current code works with integers and other kinds of literals without any changes, and with some trivial changes it could support arrays and tuples (supporting structs and enums are a little harder I think)

That was actually my initial intention, but I decided to scope it to &str because I had some trouble applying effective tests for all the cases (e.g testing built-in/proc macros and evaluating blocks with resource locks). But yeah, we could rescope it such that other primitive types also work.

Should I include lazy const expressions also, or just avoid ADTs all together?

@HKalbasi
Copy link
Member

That was actually my initial intention, but I decided to scope it to &str because I had some trouble applying effective tests for all the cases (e.g testing built-in/proc macros and evaluating blocks with resource locks). But yeah, we could rescope it such that other primitive types also work.

It's fine if this PR only supports &str and other cases are deferred to future (I think it already supports some more things, but anyway), just using general words for name and message of the assist is good enough for start.

Should I include lazy const expressions also, or just avoid ADTs all together?

By those, you mean expressions that contains closures? I guess those are theoretically impossible to support. For adt the problem is private fields, unresolved names, names that need to be imported and similar things, so I think it should avoid ADTs.

@viktorlott viktorlott force-pushed the inline-const-expr-as-static-str branch 2 times, most recently from 579db63 to ae3be8e Compare May 31, 2023 10:34
@viktorlott viktorlott changed the title feat: inline const expr as static str feat: inline const as literal May 31, 2023
@viktorlott viktorlott force-pushed the inline-const-expr-as-static-str branch 3 times, most recently from 21d83a5 to d68d11e Compare May 31, 2023 16:33
crates/ide-assists/src/handlers/inline_const_as_literal.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/inline_const_as_literal.rs Outdated Show resolved Hide resolved
Comment on lines +683 to +713
fn inline_const_as_literal_expr_as_str_lit_not_applicable() {
check_assist_not_applicable(
inline_const_as_literal,
r#"
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add some negative tests for cases that their type is not supported (adt, closure, ...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely,
Are the newly added tests enough, or should I add more variance to them?

@HKalbasi
Copy link
Member

HKalbasi commented Jun 2, 2023

#14947 fixes some of the render_eval problems in this PR tests.

@viktorlott
Copy link
Contributor Author

#14947 fixes some of the render_eval problems in this PR tests.

Great! I'll take a look at your comments after work

@viktorlott viktorlott force-pushed the inline-const-expr-as-static-str branch from d68d11e to 217d930 Compare June 5, 2023 22:43
@viktorlott viktorlott force-pushed the inline-const-expr-as-static-str branch from 217d930 to eef716d Compare June 6, 2023 10:14
@HKalbasi
Copy link
Member

HKalbasi commented Jun 6, 2023

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Jun 6, 2023

📌 Commit eef716d has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 6, 2023

⌛ Testing commit eef716d with merge 058e2d2...

@bors
Copy link
Collaborator

bors commented Jun 6, 2023

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 058e2d2 to master...

@bors bors merged commit 058e2d2 into rust-lang:master Jun 6, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants