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

feat: rustc_pass_by_value lint attribute #92646

Merged
merged 6 commits into from
Jan 16, 2022

Conversation

mdibaiee
Copy link
Contributor

@mdibaiee mdibaiee commented Jan 7, 2022

Useful for thin wrapper attributes that are best passed as value instead
of reference.

Fixes #76935

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 7, 2022
@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2022
@mdibaiee
Copy link
Contributor Author

mdibaiee commented Jan 7, 2022

My PR is not fully functional yet: For some reason when I try to compile code with #[pass_by_value], I get the error below, even though I have registered it as a builtin:

(Trying to enable this on rustc_middle::ty:Ty)

error: cannot find attribute `pass_by_value` in this scope
   --> compiler/rustc_middle/src/ty/mod.rs:469:3
    |
469 | #[pass_by_value]
    |   ^^^^^^^^^^^^^

I will try to take another look a bit later, but any help is appreciated. Thank you!

P.S. this PR is subject to change as I'm trying to understand the scope better, see my question here: #76935 (comment)

@rust-log-analyzer

This comment has been minimized.

@mdibaiee mdibaiee force-pushed the 76935/pass-by-value branch from f48caf7 to 96bba75 Compare January 7, 2022 15:40
@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 7, 2022
@mdibaiee mdibaiee force-pushed the 76935/pass-by-value branch 2 times, most recently from 7b3c4ca to 73367ad Compare January 7, 2022 15:44
@mdibaiee
Copy link
Contributor Author

mdibaiee commented Jan 7, 2022

@rustbot label -T-rustdoc

@rustbot rustbot removed the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 7, 2022
@mdibaiee mdibaiee force-pushed the 76935/pass-by-value branch from 73367ad to ddd8237 Compare January 7, 2022 15:49
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

impl looks good to me

considering that PASS_BY_VALUE is an internal lint i would prefer #[rustc_pass_by_value] as the attribute for now.

Before merging this I would like to see some tests, both testing the Ty, TyCtxt interactions, there are some existing ui-fulldeps tests for the old TY_PASS_BY_REFERENCE lint. And probably some ordinary ui tests using custom types

compiler/rustc_lint/src/pass_by_value.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/pass_by_value.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@mdibaiee mdibaiee changed the title feat: pass_by_value lint attribute feat: rustc_pass_by_value lint attribute Jan 8, 2022
@mdibaiee mdibaiee force-pushed the 76935/pass-by-value branch from 2ec2bd2 to 589e66c Compare January 8, 2022 22:40
@mdibaiee
Copy link
Contributor Author

mdibaiee commented Jan 8, 2022

I tried to write tests for custom types, but for some reason I didn't get any errors from those custom types in my test file. I am not sure if I understand it correctly, but is that because the lint is internal, and the tests might not be considered rustc internal, and so the lint doesn't apply to them?

I did this in src/test/ui-fulldeps/internal-lints/rustc_pass_by_value.rs:

#[rustc_pass_by_value]
trait CustomTrait {
    fn test<T: CustomTrait>(
        value: T,
        reference: &T, //~ ERROR passing `CustomTrait` by reference
    );
}

#[rustc_pass_by_value]
trait CustomTraitAlias: CustomTrait {
    fn test<T: CustomTraitAlias>(
        value: T,
        reference: &T, //~ ERROR passing `CustomTraitAlias` by reference
    );
}

#[rustc_pass_by_value]
enum CustomEnum {}

impl CustomEnum {
    fn test(
        value: CustomEnum,
        reference: &CustomEnum, //~ ERROR passing `CustomEnum` by reference
    ) {
    }
}

#[rustc_pass_by_value]
struct CustomStruct {}

#[rustc_pass_by_value]
type CustomAlias<'a> = &'a CustomStruct;

impl CustomStruct {
    fn test(
        value: CustomStruct,
        reference: &CustomStruct, //~ ERROR passing `CustomStruct` by reference
    ) {
    }

    fn test_alias(
        value: CustomAlias,
        reference: &CustomAlias, //~ ERROR passing `CustomAlias` by reference
    ) {
    }
}



————

not found errors (from test file): [
    Error {
        line_num: 71,
        kind: Some(
            Error,
        ),
        msg: "passing `CustomTrait` by reference",
    },
    Error {
        line_num: 79,
        kind: Some(
            Error,
        ),
        msg: "passing `CustomTraitAlias` by reference",
    },
    Error {
        line_num: 89,
        kind: Some(
            Error,
        ),
        msg: "passing `CustomEnum` by reference",
    },
    Error {
        line_num: 103,
        kind: Some(
            Error,
        ),
        msg: "passing `CustomStruct` by reference",
    },
    Error {
        line_num: 109,
        kind: Some(
            Error,
        ),
        msg: "passing `CustomAlias` by reference",
    },
]

Is there any way to enable those internal lints in this test file? I'll take another look tomorrow and might find some solution but would appreciate any help in the meantime.

@mdibaiee mdibaiee force-pushed the 76935/pass-by-value branch from 589e66c to f402026 Compare January 8, 2022 23:10
@rust-log-analyzer

This comment has been minimized.

@mdibaiee mdibaiee force-pushed the 76935/pass-by-value branch from f402026 to 172550f Compare January 9, 2022 08:23
@rust-log-analyzer

This comment has been minimized.

@mdibaiee mdibaiee force-pushed the 76935/pass-by-value branch from 172550f to db5a4e1 Compare January 9, 2022 09:22
@rust-log-analyzer

This comment has been minimized.

@mdibaiee mdibaiee force-pushed the 76935/pass-by-value branch from db5a4e1 to ee6bbca Compare January 9, 2022 10:01
@rust-log-analyzer

This comment has been minimized.

@mdibaiee mdibaiee force-pushed the 76935/pass-by-value branch from ee6bbca to 0f3d29e Compare January 9, 2022 10:42
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

some final nits, then r=me

compiler/rustc_feature/src/builtin_attrs.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/pass_by_value.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/pass_by_value.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/pass_by_value.rs Outdated Show resolved Hide resolved
}
GenericArg::Const(c) => {
let snippet =
cx.tcx.sess.source_map().span_to_snippet(c.span).unwrap_or_default();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cx.tcx.sess.source_map().span_to_snippet(c.span).unwrap_or_default();
cx.tcx.sess.source_map().span_to_snippet(c.span).unwrap_or_else(|_| String::from("_"));

🤔 don't want empty strings in case the span is broken. Don't know how to even get broken strings here, so i don't think this matters too much :p

@lcnr
Copy link
Contributor

lcnr commented Jan 11, 2022

r=me after this last nit, thanks for your patience

@bors delegate+

@bors
Copy link
Contributor

bors commented Jan 11, 2022

✌️ @mdibaiee can now approve this pull request

@mdibaiee
Copy link
Contributor Author

@lcnr thanks a million for your reviews! I will mark this with r=lcnr when CI passes 🚀

@mdibaiee
Copy link
Contributor Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Jan 11, 2022

📌 Commit 2728af7 has been approved by lcnr

@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 Jan 11, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
feat: rustc_pass_by_value lint attribute

Useful for thin wrapper attributes that are best passed as value instead
of reference.

Fixes rust-lang#76935
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
feat: rustc_pass_by_value lint attribute

Useful for thin wrapper attributes that are best passed as value instead
of reference.

Fixes rust-lang#76935
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
feat: rustc_pass_by_value lint attribute

Useful for thin wrapper attributes that are best passed as value instead
of reference.

Fixes rust-lang#76935
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
feat: rustc_pass_by_value lint attribute

Useful for thin wrapper attributes that are best passed as value instead
of reference.

Fixes rust-lang#76935
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#92487 (Fix unclosed boxes in pretty printing of TraitAlias)
 - rust-lang#92581 (ARMv6K Horizon - Enable default libraries)
 - rust-lang#92619 (Add diagnostic items for macros)
 - rust-lang#92635 (rustdoc: Yet more intra-doc links cleanup)
 - rust-lang#92646 (feat: rustc_pass_by_value lint attribute)
 - rust-lang#92706 (Clarify explicitly that BTree{Map,Set} are ordered.)
 - rust-lang#92710 (Include Projections when elaborating TypeOutlives)
 - rust-lang#92746 (Parse `Ty?` as `Option<Ty>` and provide structured suggestion)
 - rust-lang#92792 (rustdoc: fix intra-link for generic trait impls)
 - rust-lang#92814 (remove unused FIXME)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c5041f8 into rust-lang:master Jan 16, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 16, 2022
@mdibaiee mdibaiee deleted the 76935/pass-by-value branch January 16, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.

Attribute for types that should always be passed by value
7 participants