Correctly fill default type parameters #4445
Conversation
flodiebold
left a comment
flodiebold
left a comment
There was a problem hiding this comment.
Exciting! Yeah, I think I have a better way to get the AST, see comment.
| .filter_map(|(k, v)| match v { | ||
| Some(v) => Some((k, v)), | ||
| None => match k.default(source_scope.db)? { | ||
| TypeRef::Path(path) => Some(( |
There was a problem hiding this comment.
We shouldn't look into the type ref here -- it's naming a type, we want to put that type into the code, there should be no need to special-case. And this should of course also work for types that aren't single-ident paths (&Foo, foo::Bar, ...).
The other thing is that the default is written in the scope of the trait declaration, and might not resolve here at the impl site. So we need to qualify it.
I think we can solve both problems at the same time:
- instead of using
db.generic_params(), which gives us just the unresolvedTypeRef, we can usedb.generic_defaults(), which will give us the resolvedTy. - and then we can use the existing infrastructure to print the type using the
display_source_codefunction.
There was a problem hiding this comment.
I wonder if we should replace ast_transform with display_source_code altogether?
There was a problem hiding this comment.
I'm not sure we're quite there yet, considering formatting, handling of type aliases, etc...
| TypeRef::Path(path) => Some(( | ||
| k, | ||
| ast::make::type_arg(&format!("{}", path.mod_path().as_ident()?)) | ||
| .type_ref()?, |
There was a problem hiding this comment.
seems unnecessary to add a make::type_arg when we actually just want the type_ref?
crates/ra_assists/Cargo.toml
Outdated
| ra_db = { path = "../ra_db" } | ||
| ra_ide_db = { path = "../ra_ide_db" } | ||
| hir = { path = "../ra_hir", package = "ra_hir" } | ||
| hir_def = { path = "../ra_hir_def", package = "ra_hir_def" } |
There was a problem hiding this comment.
ide crates should only depend on hir directly, hir_xxx is sort of private stuff for compiler internals.
| .filter_map(|(k, v)| match v { | ||
| Some(v) => Some((k, v)), | ||
| None => match k.default(source_scope.db)? { | ||
| TypeRef::Path(path) => Some(( |
There was a problem hiding this comment.
I wonder if we should replace ast_transform with display_source_code altogether?
|
Tried fixing; the indexing into |
crates/ra_hir/src/code_model.rs
Outdated
| pub fn default(self, db: &dyn HirDatabase) -> Option<Ty> { | ||
| let params = db.generic_defaults(self.id.parent); | ||
| let local_idx: u32 = self.id.local_id.into_raw().into(); | ||
| params.get(local_idx as usize).map(|d| d.clone()) |
There was a problem hiding this comment.
hm yeah local_idx isn't necessarily the correct index here. There's a function that will give you the correct index in ra_hir_ty::utils, but it's currently private to that crate: generics(db, parent_def).param_idx(param_id). let's maybe just export that as a function from ra_hir_ty? i.e.
pub fn param_idx(db, param: TypeParamId) -> Option<usize> { generics(db, param.parent).param_idx(param) }There was a problem hiding this comment.
also, .map(|d| d.clone()) = .cloned(), I think
There was a problem hiding this comment.
oh, and this function should wrap the Ty in a code_model::Type 😬
|
bors r+ |
|
Build succeeded: |
3 participants
Fixes #3877
So, basically even if the parameters are omitted from the
implblock, check the parameters intraitif they have a default type, and if they do go fromhirtoast::TypeArg. I've added a helper for that but I am not sure that it's a proper way to go fromhirtoasthere.