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

fix: Function argument type inference with associated type impl trait #16769

Merged
merged 1 commit into from Mar 6, 2024

Conversation

ShoyuVanilla
Copy link
Contributor

Fixes #15412

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 6, 2024
@@ -1055,10 +1055,10 @@ impl<'a> TyLoweringContext<'a> {

fn assoc_type_bindings_from_type_bound(
&'a self,
bound: &'a TypeBound,
bound: &'a Interned<TypeBound>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the argument type into Interned for faster equality comparision

.with_type_param_mode(ParamLoweringMode::Variable);
let ctx = if let GenericDefId::FunctionId(_) = def {
TyLoweringContext::new(db, &resolver, def.into())
.with_impl_trait_mode(ImplTraitLoweringMode::Variable)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

impl Traits that appear in the function arguments should be handled as generic parameters with trait implementation predicates. So, we handle impl Trait by the same way with the type parameters.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just require the modes to specified in the new constructor. Feels like an easy to cause bug by relying on a default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be better. Actually, I've been considering giving a shot for #16584 while writing this PR and that would require some changes to lower.rs. It would be good to apply your suggestions while making that changes

.with_type_param_mode(ParamLoweringMode::Variable);
let ctx = if let GenericDefId::FunctionId(_) = def {
TyLoweringContext::new(db, &resolver, def.into())
.with_impl_trait_mode(ImplTraitLoweringMode::Variable)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just require the modes to specified in the new constructor. Feels like an easy to cause bug by relying on a default.

@Veykril
Copy link
Member

Veykril commented Mar 6, 2024

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Mar 6, 2024

📌 Commit a8f5611 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Mar 6, 2024

⌛ Testing commit a8f5611 with merge 1a55ab3...

@bors
Copy link
Collaborator

bors commented Mar 6, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 1a55ab3 to master...

@bors bors merged commit 1a55ab3 into rust-lang:master Mar 6, 2024
11 checks passed
@ShoyuVanilla ShoyuVanilla deleted the issue-15412 branch March 6, 2024 13:04
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.

Unknown type in presence of nested RPIT
4 participants