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

simplify const params diagnostic on stable #95820

Merged
merged 1 commit into from
Apr 12, 2022
Merged

Conversation

OliverMD
Copy link
Contributor

@OliverMD OliverMD commented Apr 8, 2022

Resolves #95150

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

r? @wesleywiser

(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 Apr 8, 2022
@OliverMD
Copy link
Contributor Author

OliverMD commented Apr 8, 2022

@lcnr You added the E-Mentor tag on #95150, is this sort of what you had in mind? Would you like to review this?

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.

Thanks 👍

While your approach works, I think that this code already was a bit more complex than necessary and should get cleaned up a bit while we're already here.

@@ -863,7 +863,9 @@ fn check_param_wf(tcx: TyCtxt<'_>, param: &hir::GenericParam<'_>) {
}
};

if traits::search_for_structural_match_violation(param.span, tcx, ty).is_some() {
if tcx.features().adt_const_params // structural matching only allowed with adt_const_params feature
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have a check for the adt_const_params feature a few lines above. I would prefer to only have 1 branch for that. So pretty much

if tcx.features().adt_const_params {
   // complex check¹
} else {
   // simple check, completely ignoring structural equality
}

¹ It might be interesting to check whether function pointers are considered a structural match violation. If they are, you can completely avoid checking the ty in that branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah gotcha, thanks! Unfortunately it looks like function pointers (and raw pointers) aren't structural match violations, but just to check, my (simplistic) reasoning is based on removing the ty check in the complex check case and then observing that the following test case successfully compiled, which I believe it shouldn't:

// revisions: full min
#![cfg_attr(full, feature(adt_const_params))]
#![cfg_attr(full, allow(incomplete_features))]

struct Test();

fn pass() {
    println!("Hello, world!");
}

impl Test {
    pub fn call_me(&self) {
        self.test::<pass>();
    }

    fn test<const FN: fn()>(&self) {
        //~^ ERROR: using function pointers as const generic parameters is forbidden
        FN();
    }
}

fn main() {
    let x = Test();
    x.call_me()
}

@lcnr
Copy link
Contributor

lcnr commented Apr 11, 2022

r? @lcnr

@rust-highfive rust-highfive assigned lcnr and unassigned wesleywiser Apr 11, 2022
@lcnr
Copy link
Contributor

lcnr commented Apr 12, 2022

thanks

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 12, 2022

📌 Commit 3b4589a 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 Apr 12, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 12, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#95671 (feat: Allow usage of sudo [while not accessing root] in x.py)
 - rust-lang#95716 (sess: warn w/out fluent bundle w/ user sysroot)
 - rust-lang#95820 (simplify const params diagnostic on stable)
 - rust-lang#95900 (Fix documentation for wasm32-unknown-unknown)
 - rust-lang#95947 (`impl const Default for Box<[T]>` and `Box<str>`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 93e6020 into rust-lang:master Apr 12, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 12, 2022
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.

Diagnostic for const generic enum on stable suggests requirements for nightly-only feature (PartialEq, Eq)
6 participants