Fix crash when labeling arguments for call_once and friends #129320
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) some time within the next two weeks. 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 (
|
rustbot
added
S-waiting-on-review
| @@ -57,7 +57,7 @@ pub fn check_legal_trait_for_method_call( | |||
| enum CallStep<'tcx> { | |||
| Builtin(Ty<'tcx>), | |||
| DeferredClosure(LocalDefId, ty::FnSig<'tcx>), | |||
| /// E.g., enum variant constructors. | |||
| /// Call overloading when callee implements one of the Fn* traits. | |||
There was a problem hiding this comment.
This was a drive-by change which I think is more accurate.
| .skip(if is_method { 1 } else { 0 }) | ||
| .collect(); | ||
|
|
||
| generic_params.into_iter().zip(params).collect() | ||
| assert!(generic_params.len() == params.len()); |
There was a problem hiding this comment.
This is a little paranoid but if we're calling zip with lists of different lengths, the result will likely be subtly wrong.
| let mut generics_with_unmatched_params = Vec::new(); | ||
| if let Some(params_with_generics) = self.get_hir_params_with_generics(def_id, is_method) | ||
| { | ||
| assert_eq!(params_with_generics.len(), matched_inputs.len()); |
There was a problem hiding this comment.
I couldn't tell from surrounding code if this should be an assert!, a debug_assert!, or a bug!. Feedback welcome!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
You can just delete the 128848 test since you already added it as a regular UI test I think |
|
@compiler-errors thanks, done! (Should I have known that test was there somehow?) |
|
@jder Looks good to me FWIW (though I'm not the expert here, merely the perpetrator :) ), thanks for fixing it. I wasn't thrilled about zipping potentially unequal sizes for sure, but I couldn't see any way they wouldn't be equal offhand, but the worry now is with these asserts if they weren't equal it would throw an ICE again. Though I suspect in these cases the reasoning is obscure enough (like this kind of a bug) that it's (at least borderline) acceptable in such a case. However over to those who know more than I. Actually the zipping unequal sizes might cause a panic too, would have to check. FYI, @compiler-errors I suspect knew to remove that test because Github actions threw that up. Always worth checking that passes. which it now has, rustc has a fantastic test suite that CI will run for you whenever you make changes. Also for anyone reviewing, the changes are much smaller if you ignore whitespace I found. |
|
Hey @cjgillot, this has been open a couple weeks, would you have time to take a look? Let me know if you'd like me to break it up to make it easier to review, or if I should ping someone else. Thanks! |
|
☔ The latest upstream changes (presumably #130269) made this pull request unmergeable. Please resolve the merge conflicts. |
|
LGTM -- can you rebase, and fix that one nit I left? Also, I'm thinking those |
…should point at the called method (eg Fn::call_once), not the underlying callee. Fixes 128848
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
|
@compiler-errors thanks for the review! |
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#129320 (Fix crash when labeling arguments for call_once and friends) - rust-lang#130266 (target: default to the medium code model on LoongArch targets) - rust-lang#130297 (Dataflow cleanups) - rust-lang#130299 (Add set_dcx to ParseSess) - rust-lang#130301 (some fixes for clashing_extern_declarations lint) - rust-lang#130305 (Clippy: consider msrv for const context for const_float_bits_conv) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#129320 (Fix crash when labeling arguments for call_once and friends) - rust-lang#130266 (target: default to the medium code model on LoongArch targets) - rust-lang#130297 (Dataflow cleanups) - rust-lang#130299 (Add set_dcx to ParseSess) - rust-lang#130301 (some fixes for clashing_extern_declarations lint) - rust-lang#130305 (Clippy: consider msrv for const context for const_float_bits_conv) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#129320 - jder:issue-128848, r=compiler-errors Fix crash when labeling arguments for call_once and friends When calling a method on Fn* traits explicitly, argument diagnostics should point at the called method (eg Fn::call_once), not the underlying callee. This PR makes 3 main changes: * It uses TupleArguments to detect if the user called a Fn* method directly (`my_fn.call_once(…)`) or implicitly (`my_fn(…)`). If it was explicit, argument diagnostics should point at the call_once method, not the underlying callable. * The previous state was causing confusion between the two arguments lists (which could be different lengths), causing an out-of-bounds slice indexing in rust-lang#128848. I added a length assert to capture the requirement in case this regresses or happens in another case. * Unfortunately, this assert tripped when the required arguments information was not available (`self.get_hir_params_with_generics` was returning an empty Vec), so I've updated that to return None when that information is not available. (cc `@strottos` if you have any comments, since you added this function in rust-lang#121595) Sorry this causes a bunch of indentation changes, recommend reviewing [ignoring whitespace](https://github.com/rust-lang/rust/pull/129320/files?w=1).) This is my first rustc PR, so please call out if you'd like this split into more commits (or PRs), style nits, etc. I will add a few comments/questions inline. Thank you! Fixes rust-lang#128848
Fix circular fn_sig queries to correct number of args for methods Fixes rust-lang#130400. This was a [debug assert](https://github.com/rust-lang/rust/blob/28e8f01c2a2f33fb4214925a704e3223b372cad5/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L2557) added to some argument error reporting code in rust-lang#129320 which verified that the number of params (from the HIR) matched the `matched_inputs` which ultimately come from ty::FnSig. In the reduced test case: ``` fn foo(&mut self) -> _ { foo() } ``` There is a circular dependency computing the ty::FnSig -- when trying to compute it, we try to figure out the return value, which again depends on this ty::FnSig. In rust-lang#105162, this was supported by short-circuiting the cycle by synthesizing a FnSig with error types for parameters. The [code in question](https://github.com/rust-lang/rust/pull/105162/files#diff-a65feec6bfffb19fbdc60a80becd1030c82a56c16b177182cd277478fdb04592R44) computes the number of parameters by taking the number of parameters from the hir::FnDecl and adding 1 if there is an implicit self parameter. I might be missing a subtlety here, but AFAICT the adjustment for implicit self args is unnecessary and results in one too many args. For example, for this non-errorful code: ``` trait Foo { fn bar(&self) {} } ``` The resulting hir::FnDecl and ty::FnSig both have the same number of inputs -- 1. So, this PR removes that adjustment and adds a test for the debug ICE. r? `@compiler-errors`
Rollup merge of rust-lang#130496 - jder:issue-130400, r=compiler-errors Fix circular fn_sig queries to correct number of args for methods Fixes rust-lang#130400. This was a [debug assert](https://github.com/rust-lang/rust/blob/28e8f01c2a2f33fb4214925a704e3223b372cad5/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L2557) added to some argument error reporting code in rust-lang#129320 which verified that the number of params (from the HIR) matched the `matched_inputs` which ultimately come from ty::FnSig. In the reduced test case: ``` fn foo(&mut self) -> _ { foo() } ``` There is a circular dependency computing the ty::FnSig -- when trying to compute it, we try to figure out the return value, which again depends on this ty::FnSig. In rust-lang#105162, this was supported by short-circuiting the cycle by synthesizing a FnSig with error types for parameters. The [code in question](https://github.com/rust-lang/rust/pull/105162/files#diff-a65feec6bfffb19fbdc60a80becd1030c82a56c16b177182cd277478fdb04592R44) computes the number of parameters by taking the number of parameters from the hir::FnDecl and adding 1 if there is an implicit self parameter. I might be missing a subtlety here, but AFAICT the adjustment for implicit self args is unnecessary and results in one too many args. For example, for this non-errorful code: ``` trait Foo { fn bar(&self) {} } ``` The resulting hir::FnDecl and ty::FnSig both have the same number of inputs -- 1. So, this PR removes that adjustment and adds a test for the debug ICE. r? `@compiler-errors`
When calling a method on Fn* traits explicitly, argument diagnostics should point at the called method (eg Fn::call_once), not the underlying callee.
This PR makes 3 main changes:
my_fn.call_once(…)) or implicitly (my_fn(…)). If it was explicit, argument diagnostics should point at the call_once method, not the underlying callable.self.get_hir_params_with_genericswas returning an empty Vec), so I've updated that to return None when that information is not available. (cc @strottos if you have any comments, since you added this function in Better reporting on generic argument mismatchs #121595) Sorry this causes a bunch of indentation changes, recommend reviewing ignoring whitespace.)This is my first rustc PR, so please call out if you'd like this split into more commits (or PRs), style nits, etc. I will add a few comments/questions inline. Thank you!
Fixes #128848