Skip to content

feat: Add two new diagnostics: one for mismatch in generic arguments count, and another for mismatch in their kind #19479

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ChayimFriedman2
Copy link
Contributor

Also known as E0747 and E0107.

And by the way, rewrite how we lower generic arguments and deduplicate it between paths and method calls. The new version is taken almost straight from rustc.

This commit also changes the binders of generic_defaults(), to only include the binders of the arguments up to (and not including) the current argument. This make it easier to handle it in the rewritten lowering of generic args. It's also how rustc does it.

This is a step towards #17590, the next step will be to add a quickfix to the count mismatch diagnostic that adds new parameters. Since this became somewhat big I separated the steps.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2025
@ChayimFriedman2 ChayimFriedman2 force-pushed the generic-mismatch branch 3 times, most recently from b8c3e02 to 107dbaa Compare April 14, 2025 08:56
… and another for mismatch in their kind

Also known as E0747 and E0107.

And by the way, rewrite how we lower generic arguments and deduplicate it between paths and method calls. The new version is taken almost straight from rustc.

This commit also changes the binders of `generic_defaults()`, to only include the binders of the arguments up to (and not including) the current argument. This make it easier to handle it in the rewritten lowering of generic args. It's also how rustc does it.
Comment on lines +3232 to +3261

/// This function find the AST fragment that corresponds to an `AssociatedTypeBinding` in the HIR.
pub fn hir_assoc_type_binding_to_ast(
segment_args: &ast::GenericArgList,
binding_idx: u32,
) -> Option<ast::AssocTypeArg> {
segment_args
.generic_args()
.filter_map(|arg| match arg {
ast::GenericArg::AssocTypeArg(it) => Some(it),
_ => None,
})
.filter(|binding| binding.param_list().is_none() && binding.name_ref().is_some())
.nth(binding_idx as usize)
}

/// This function find the AST generic argument from the one in the HIR. Does not support the `Self` argument.
pub fn hir_generic_arg_to_ast(
args: &ast::GenericArgList,
arg_idx: u32,
has_self_arg: bool,
) -> Option<ast::GenericArg> {
args.generic_args()
.filter(|arg| match arg {
ast::GenericArg::AssocTypeArg(_) => false,
ast::GenericArg::LifetimeArg(arg) => arg.lifetime().is_some(),
ast::GenericArg::ConstArg(_) | ast::GenericArg::TypeArg(_) => true,
})
.nth(arg_idx as usize - has_self_arg as usize)
}
Copy link
Member

Choose a reason for hiding this comment

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

Naming nit (also the location for them seems unrelated), these don't touch anything hir related

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hir is against the ast: it finds the AST for the HIR lowered.

Comment on lines +11 to +22
let owner_description = match d.def {
hir::GenericDef::Function(_) => "function",
hir::GenericDef::Adt(hir::Adt::Struct(_)) => "struct",
hir::GenericDef::Adt(hir::Adt::Enum(_)) => "enum",
hir::GenericDef::Adt(hir::Adt::Union(_)) => "union",
hir::GenericDef::Trait(_) => "trait",
hir::GenericDef::TraitAlias(_) => "trait alias",
hir::GenericDef::TypeAlias(_) => "type alias",
hir::GenericDef::Impl(_) => "impl",
hir::GenericDef::Const(_) => "constant",
hir::GenericDef::Static(_) => "static",
};
Copy link
Member

Choose a reason for hiding this comment

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

Let's extract that into a method? Seems generally useful for more diagnostics in the future

@Veykril
Copy link
Member

Veykril commented Apr 21, 2025

#19624 will probably collide with this once merged

@ChayimFriedman2
Copy link
Contributor Author

#19624 will probably collide with this once merged

I'm fine with waiting until it's merged, I don't want to make @jackh726's life any harder.

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.

3 participants