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

Preserve order of generic args #11140

Merged
merged 2 commits into from
Mar 4, 2022
Merged

Conversation

HKalbasi
Copy link
Member

rust-lang/rust#90207 removed order restriction of generic args, i.e. const generics can now become before of type generics. We need to preserve this order to analyze correctly, and this PR does that.

It also simplifies implementation of const generics a bit IMO.

Implementing default generics the same problem of #7434, we need lower them to body and then evaluate them.

crates/hir/src/has_source.rs Outdated Show resolved Hide resolved
crates/hir/src/lib.rs Outdated Show resolved Hide resolved
}

impl ConstParam {
pub fn merge(self) -> TypeOrConstParam {
Copy link
Member

Choose a reason for hiding this comment

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

Feels like this should be just a plain From impl?

Copy link
Member

Choose a reason for hiding this comment

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

Though I guess the benefit here is in not having a generic output type involved, the name merge feels wrong though since we don't merge anything

Copy link
Member Author

Choose a reason for hiding this comment

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

merge is reverse of split on the TypeOrConstParam. Suggestion for better name? generalize?

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 have better names either, so keeping it as merge or generalize is fine

Copy link
Member

Choose a reason for hiding this comment

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

I would also expect this to just be a From impl, is that not possible?

crates/hir/src/semantics.rs Outdated Show resolved Hide resolved
crates/hir_def/src/attr.rs Outdated Show resolved Hide resolved
crates/hir_def/src/attr.rs Outdated Show resolved Hide resolved
crates/ide/src/navigation_target.rs Outdated Show resolved Hide resolved
crates/ide/src/navigation_target.rs Outdated Show resolved Hide resolved
crates/ide/src/syntax_highlighting/inject.rs Outdated Show resolved Hide resolved
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

This looks good in my eyes now, but since there is a lot of type stuff involved @flodiebold should take a look at this as well once they have time since I don't feel comfortable about that part of the codebase

@lnicola
Copy link
Member

lnicola commented Jan 17, 2022

@HKalbasi can you please rebase?

@HKalbasi
Copy link
Member Author

HKalbasi commented Mar 1, 2022

@flodiebold Can you please review this PR? It touches too many files so it conflicts with master frequently.

Copy link
Member

@flodiebold flodiebold left a comment

Choose a reason for hiding this comment

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

Seems ok to me, though I wonder whether if we still have TypeParamId and ConstParamId we should still be using them in some places that explicitly expect a type/const param.

crates/hir_def/src/lib.rs Outdated Show resolved Hide resolved
@@ -68,7 +68,7 @@ enum Scope {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum TypeNs {
SelfType(ImplId),
GenericParam(TypeParamId),
GenericParam(TypeOrConstParamId),
Copy link
Member

Choose a reason for hiding this comment

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

in type ns, this has to be a type, doesn't it? so why can't this stay a TypeParamId?

Copy link
Member Author

Choose a reason for hiding this comment

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

Test hover_const_param fails if I change it. Seems I have broken something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

crates/hir_def/src/resolver.rs Outdated Show resolved Hide resolved
crates/hir_ty/src/db.rs Outdated Show resolved Hide resolved
@HKalbasi HKalbasi force-pushed the const_generic branch 4 times, most recently from c3f3605 to c334e92 Compare March 4, 2022 08:09
@flodiebold
Copy link
Member

bors d+

@bors
Copy link
Contributor

bors bot commented Mar 4, 2022

✌️ HKalbasi can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@HKalbasi
Copy link
Member Author

HKalbasi commented Mar 4, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 4, 2022

@bors bors bot merged commit f8329ba into rust-lang:master Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants