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.

Sign up for GitHub

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

Jump to bottom

the "add missing members" assists: implemented substitution of default values of const params #15179

Merged
merged 6 commits into from
Aug 15, 2023
Merged

the "add missing members" assists: implemented substitution of default values of const params #15179

merged 6 commits into from
Aug 15, 2023

Conversation

ponyii
Copy link
Contributor

@ponyii ponyii commented Jun 30, 2023

To achieve this, I've made hir::ConstParamData store the default values

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2023
ponyii
ponyii commented Jun 30, 2023
View reviewed changes
crates/hir/src/lib.rs Outdated Show resolved Hide resolved
@ponyii ponyii force-pushed the fix/default-values-of-const-params-are-ignored branch from 58fdf05 to df7e701 Compare July 1, 2023 17:49
@bors
Copy link
Contributor

bors commented Jul 6, 2023

☔ The latest upstream changes (presumably #15224) made this pull request unmergeable. Please resolve the merge conflicts.

@ponyii ponyii force-pushed the fix/default-values-of-const-params-are-ignored branch from 2665241 to 695c2d2 Compare July 10, 2023 19:01
ponyii
ponyii commented Jul 10, 2023
View reviewed changes
crates/syntax/rust.ungram
@@ -290,7 +290,7 @@ TypeParam =

ConstParam =
Attr* 'const' Name ':' Type
('=' default_val:Expr)?
('=' default_val:ConstArg)?
Copy link
Contributor Author

@ponyii ponyii Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

by the way, while the parser is aware of the restrictions for ConstArg, rust.ungram is not. not sure if it's worth fixing

@ponyii ponyii force-pushed the fix/default-values-of-const-params-are-ignored branch 3 times, most recently from 47b3db1 to 5ce26e3 Compare July 12, 2023 15:26
@ponyii ponyii marked this pull request as ready for review July 12, 2023 15:26
ponyii
ponyii commented Jul 12, 2023
View reviewed changes
crates/hir-ty/src/lib.rs Outdated
@@ -719,3 +721,16 @@ where
value.visit_with(&mut collector, DebruijnIndex::INNERMOST);
collector.placeholders.into_iter().collect()
}

pub fn known_const_to_string(konst: &Const, db: &dyn HirDatabase) -> Option<String> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure where it would be better to place this function, nor how to name it. but it seems that it should be separated from the old display function, as it treats unknown consts in a special way and "unfolds" complex expressions.

@lowr
Copy link
Contributor

lowr commented Jul 27, 2023

@HKalbasi Can you review the const lowering part? I'll review the rest.

@ponyii
Copy link
Contributor Author

ponyii commented Aug 7, 2023

@lowr @HKalbasi hello! I guess we could try to review and merge the PR this week

@HKalbasi
Copy link
Member

HKalbasi commented Aug 7, 2023

Sorry I missed the first mention. The const parts looks good to me.

lowr
lowr reviewed Aug 8, 2023
View reviewed changes
Copy link
Contributor

@lowr lowr left a comment

Choose a reason for hiding this comment

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

I'm deeply sorry it took so long to review. The change generally looks awesome; I'm leaving some small suggestions.

crates/ide-db/src/path_transform.rs Outdated Show resolved Hide resolved
crates/hir-ty/src/lower.rs Outdated Show resolved Hide resolved
crates/hir-ty/src/lower.rs Outdated Show resolved Hide resolved
crates/hir-ty/src/lower.rs Outdated Show resolved Hide resolved
@lowr
Copy link
Contributor

lowr commented Aug 8, 2023

One thing we could do (but does not block this PR; just dumping my thoughts before I forget. I'm not even sure if it's possible right now) is consteval const default expressions and substitute them rather than copy-pasting the default values from the definition. For example, given the following definitions

mod m {
    const fn foo() { 42 } // notice this is private to the module
    pub trait Trait<const N: usize = { foo() }> { }
}

we cannot substitute the default value for N outside the module without visibility errors. If we could consteval the values of const default expressions, we can avoid the errors and it'd be super cool.

@ponyii ponyii force-pushed the fix/default-values-of-const-params-are-ignored branch from 8929e40 to 68e8379 Compare August 8, 2023 18:16
@ponyii
Copy link
Contributor Author

ponyii commented Aug 8, 2023 •
edited
Loading

@lowr well, I'm not sure if it's a good idea to consteval expressions with private, hm, components. it sounds to me like a cheaty way to make private function (or smth else) public. as a developer, I'd rather change visibility of the expression components - or become scared that the private values are used in an unexpected place

@ponyii ponyii requested a review from lowr August 8, 2023 18:22
@HKalbasi
Copy link
Member

HKalbasi commented Aug 8, 2023

I think keeping these unevaluated is better, since unevaluated consts are usually unevaluated for a reason. For example, we shouldn't replace 1 << 8 with 64 or SOME_NAMED_CONSTANT * 2 with its value as they may lose their meaning. That said, we can have another assist for replacing constants with their evaluated value in const generics and users can compose these assists to get that behavior.

@ponyii
Copy link
Contributor Author

ponyii commented Aug 14, 2023

@HKalbasi @lowr could somebody please merge the PR?

@HKalbasi
Copy link
Member

Sorry for this taking so long.
@bors r+

@bors
Copy link
Contributor

bors commented Aug 15, 2023

📌 Commit 68e8379 has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 15, 2023

⌛ Testing commit 68e8379 with merge b771de3...

@bors
Copy link
Contributor

bors commented Aug 15, 2023

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

@bors bors merged commit b771de3 into rust-lang:master Aug 15, 2023
@ponyii ponyii mentioned this pull request Aug 15, 2023
Merged
bors added a commit that referenced this pull request Aug 15, 2023
@bors
Auto merge of #15463 - ponyii:fix/start-hovering-default-values-of-ge…
3461bfe 
…neric-const, r=HKalbasi

fix: start hovering default values of generic constants

It's just a kind of a postscriptum for [my last PR](#15179) adding default values of const generics to `hir::ConstParamData`. Here I patch other pieces of code which used to ignore const default values and which I managed to find (you're welcome to show me more)
bors added a commit that referenced this pull request Aug 15, 2023
@bors
Auto merge of #15463 - ponyii:fix/start-hovering-default-values-of-ge…
b147709 
…neric-const, r=HKalbasi

fix: start hovering default values of generic constants

It's just a kind of a postscriptum for [my last PR](#15179) adding default values of const generics to `hir::ConstParamData`. Here I patch other pieces of code which used to ignore const default values and which I managed to find (you're welcome to show me more)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HKalbasi HKalbasi HKalbasi left review comments

@lowr lowr Awaiting requested review from lowr

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ponyii @bors @lowr @HKalbasi @rustbot