Skip to content

Suggest creating unary tuples when types don't match a trait #132583

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

mejrs
Copy link
Contributor

@mejrs mejrs commented Nov 4, 2024

When you want to have a variadic function, a common workaround to implement this is to create a trait and then implement that trait for various tuples. For example in pyo3 there exists

/// Calls the object with only positional arguments.
pub fn call1(&self, args: impl IntoPy<Py<PyTuple>>) -> PyResult<&PyAny> {
   ...
}

with various impls like

impl<A: IntoPy<PyObject> IntoPy<Py<PyAny>> for (A,)
impl<A: IntoPy<PyObject, B: IntoPy<PyObject> IntoPy<Py<PyAny>> for (A, B)
... etc

This means that if you want to call the method with a single item you have to create a unary tuple, like (x,), rather than just x.

This PR implements a suggestion to do that, if applicable.

@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 4, 2024
Comment on lines +17 to +18
LL | check(((),));
| + ++
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this suggestion okay? I'm not quite sure what #[do_not_recommend] is supposed to (not) show. Its rfc only talks about not mentioning confusing traits.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine.

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

Comment on lines 4462 to 4472
self.infcx.enter_forall(trait_ref, |trait_ref| {
if self
.type_implements_trait(trait_ref.def_id, trait_ref.args, root_obligation.param_env)
.must_apply_modulo_regions()
{
err.multipart_suggestion_verbose(
format!("use a unary tuple instead"),
vec![(span.shrink_to_lo(), "(".into()), (span.shrink_to_hi(), ",)".into())],
Applicability::MaybeIncorrect,
);
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

this is kinda unnecessary. instead of using enter_forall and calling type_implements_trait, just use predicate_must_hold_modulo_regions.

obligation: &PredicateObligation<'tcx>,
span: Span,
) {
if !matches!(obligation.cause.code(), ObligationCauseCode::FunctionArg { .. }) {
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 turn this into an if let ObligationCauseCode::FunctionArg { arg_hir_id, .. } = ....

Then use the span from arg_hir_id via tcx.hir.span(arg_hir_id). It may seem roundabout, but there's always a chance that the obligation span has been messed with, and getting the span from the actual arg expression is strictly more right...

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2024
@mejrs
Copy link
Contributor Author

mejrs commented Nov 4, 2024

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2024
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Nov 4, 2024

📌 Commit 5a48fe2 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#132355 (Fix compiler panic with a large number of threads)
 - rust-lang#132486 (No need to instantiate binder in `confirm_async_closure_candidate`)
 - rust-lang#132544 (Use backticks instead of single quotes for library feature names in diagnostics)
 - rust-lang#132559 (find the generic container rather than simply looking up for the assoc with const arg)
 - rust-lang#132579 (add rustc std workspace crate sources)
 - rust-lang#132583 (Suggest creating unary tuples when types don't match a trait)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a4f323c into rust-lang:master Nov 4, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2024
Rollup merge of rust-lang#132583 - mejrs:tuples, r=compiler-errors

Suggest creating unary tuples when types don't match a trait

When you want to have a variadic function, a common workaround to implement this is to create a trait and then implement that trait for various tuples. For example in `pyo3` there exists
```rust
/// Calls the object with only positional arguments.
pub fn call1(&self, args: impl IntoPy<Py<PyTuple>>) -> PyResult<&PyAny> {
   ...
}
```

with various impls like
```rust
impl<A: IntoPy<PyObject> IntoPy<Py<PyAny>> for (A,)
impl<A: IntoPy<PyObject, B: IntoPy<PyObject> IntoPy<Py<PyAny>> for (A, B)
... etc
```

This means that if you want to call the method with a single item you have to create a unary tuple, like `(x,)`, rather than just `x`.

This PR implements a suggestion to do that, if applicable.
@rustbot rustbot added this to the 1.84.0 milestone Nov 4, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants