-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 typo and improve documentation for E0632 #85520
Conversation
Some changes occurred in diagnostic error codes |
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
You may need to filter out the usage of the const generic parameter to specify a parameter's type: fn foo<const N: usize>(_: [i32; N], _: impl Copy) {}
struct Boo<const N: i32>{ ... }
fn bar<const N: i32>(_: Boo<i32>, _: impl Debug) {} |
You are right, of course. Thanks for pointing this out, it should be fixed now. |
☔ The latest upstream changes (presumably #86588) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in review here. This looks great. I have two thoughts:
- It'll be interesting if Implement a
explicit_generic_args_with_impl_trait
feature gate #86176 lands, since that feature gate essentially means this lint isn't needed anymore. But I think in the meantime, it's worth landing this. - I'd like a perf run after this is rebase. It is an extra ast pass, so I expect maybe a little bit of a regression; but I want to put a number on it before it lands.
Other than that, r=me after perf
Rebased on what? I did a rebase about half an hour ago on the current master branch. |
Oh whoops. Did see that it was already rebased. 🤦♀️ @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 3ff3c614cefb484ad6b209d4518be0b37cbb578d with merge d52bd326fc9e84dc83a60bfbaa9155ee6dc47cc3... |
☀️ Try build successful - checks-actions |
Queued d52bd326fc9e84dc83a60bfbaa9155ee6dc47cc3 with parent 456a032, future comparison URL. |
Finished benchmarking try commit (d52bd326fc9e84dc83a60bfbaa9155ee6dc47cc3): comparison url. Summary: ERROR categorizing benchmark run! Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
I'm not sure what to make of these results (the comparison URL link seems to be broken: "could not find start commit for bound ..."), but in any case, I have added an early return for functions that don't have const parameters, and if no |
I guess it's a bug. Let's run another one and hope it works this time. @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 06824f63ec4c3787719b7a7b7ad0e066540bc307 with merge f2285179271f79ab0fc43ae08cbd2d1d727c515f... |
☀️ Try build successful - checks-actions |
Queued f2285179271f79ab0fc43ae08cbd2d1d727c515f with parent 17ea490, future comparison URL. |
Finished benchmarking try commit (f2285179271f79ab0fc43ae08cbd2d1d727c515f): comparison url. Summary: This benchmark run did not return any significant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
@FabianWolff I guess the policy is a compiler MCP and lang FCP for new lints (see https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/New.20lint.20.28declaring.20an.20uncallable.20function.29.20.28.2385520.29/near/244202217). @petrochenkov noted that the cost/benefit of this is too high Can you make an MCP if you think this is worth it and see where that goes? |
Ah, I didn't know it is such a big deal. No, I don't think it is worth it here. I have removed all of my changes except for a fixed typo and the extended description for |
This comment has been minimized.
This comment has been minimized.
@bors r+ rollup |
📌 Commit 69c8573 has been approved by |
Rollup of 8 pull requests Successful merges: - rust-lang#85504 (the foundation owns rust trademarks) - rust-lang#85520 (Fix typo and improve documentation for E0632) - rust-lang#86680 (Improve error for missing -Z with debugging option) - rust-lang#86728 (Check node kind to avoid ICE in `check_expr_return()`) - rust-lang#86740 (copy rust-lld as ld in dist) - rust-lang#86746 (Fix rustdoc query type filter) - rust-lang#86750 (Test cross-crate usage of `feature(const_trait_impl)`) - rust-lang#86755 (alloc: `RawVec<T, A>::shrink` can be in `no_global_oom_handling`.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Edit: After #85520 (comment), this PR has been boiled down to just an extended description for
E0632
and a fixed typo.