Skip to content

Pass opaque types for the type_alias_bounds lint #108663

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

Closed
wants to merge 8 commits into from

Conversation

mu001999
Copy link
Contributor

@mu001999 mu001999 commented Mar 2, 2023

Fixes #108617

@rustbot
Copy link
Collaborator

rustbot commented Mar 2, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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 Mar 2, 2023
@mu001999
Copy link
Contributor Author

mu001999 commented Mar 3, 2023

r? @lcnr

@rustbot
Copy link
Collaborator

rustbot commented Mar 3, 2023

Could not assign reviewer from: lcnr.
User(s) lcnr are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

@lcnr
Copy link
Contributor

lcnr commented Mar 3, 2023

r? @oli-obk

do we convert the whole type alias to an opaque for type Foo<T: Debug> = (impl Debug, usize); 🤔

@rustbot rustbot assigned oli-obk and unassigned lcnr Mar 3, 2023
@mu001999
Copy link
Contributor Author

mu001999 commented Mar 3, 2023

For this lint, there is not enough information at check time to determine if bounds are needed when opaque types appear on the right side. So I think in this case, the warning should not be emitted for now.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 3, 2023

I think we should still lint if the non-opaque types contain generic params of the type alias. So for example:

type Foo<T: Debug> = (impl Foo<T>, T);

Also: please add a bunch of tests for various cases (other types using generic params, other types not using them, ...)

@mu001999
Copy link
Contributor Author

mu001999 commented Mar 3, 2023

@oli-obk Thanks! I pushed some new commits, only bounds whose ty-params or lifetimes-params are used in non-opaque types would be linted now. And I added some tests. :)

@rust-log-analyzer

This comment has been minimized.

@mu001999
Copy link
Contributor Author

mu001999 commented Mar 3, 2023

Sorry, some tests failed, I will check the code.

@mu001999
Copy link
Contributor Author

mu001999 commented Mar 4, 2023

@oli-obk I have tried to only lint bounds whose ty is a generic parameter used in the non-opaque types if there is an opaque type. You can see the new commits. But it still lead to some false positives. For the following code, the bound T: 'a cannot be removed and will trigger the warning:

#![feature(type_alias_impl_trait)]
#![allow(dead_code)]

use std::fmt::Debug;

type Foo<'a, T: 'a> where &'a T: Debug = (impl Debug + 'a, T);
fn foo<'a, U: Debug + 'a>(v: U) -> Foo<'a, U> where &'a U: Debug {
    (Vec::<&'a U>::new(), v)
}

So, I think the best way is to not lint the type when meeting opaque types. What do you think?

@lcnr
Copy link
Contributor

lcnr commented Mar 6, 2023

just want to throw this out there, but could we change only type aliases with opaque types to actually get treated as alias types and completely disable the lint for those?

I think our longterm goal is to check wf for trait aliases anyways, so this feels like a step in the right direction 🤔

@oli-obk
Copy link
Contributor

oli-obk commented Mar 6, 2023

could we change only type aliases with opaque types to actually get treated as alias types

you mean, adding TyKind::Alias but only using it for type aliases that have an opaque type in them?

longterm goal is to check wf for trait aliases anyways

I think you mean type aliases, but yes 😆

@mu001999
Copy link
Contributor Author

mu001999 commented Mar 7, 2023

Sounds good, maybe this PR should be closed :)

@oli-obk
Copy link
Contributor

oli-obk commented Mar 7, 2023

Thanks for opening the PR! We found a good design due to it, I'll assign the issue to myself

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. 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.

Spurious "bounds [...] not enforced in type aliases" when using TAIT
5 participants