-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Cleanup and refactor FnCtxt::report_no_match_method_error #148652
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
Cleanup and refactor FnCtxt::report_no_match_method_error #148652
Conversation
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
|
r? @lcnr |
9a8910e to
177bb33
Compare
This comment has been minimized.
This comment has been minimized.
|
For changes which move a bunch of code around it's really helpful to split it into individual commits, e.g. if you have "move things into a |
| ) { | ||
| let mut find_candidate_for_method = false; | ||
|
|
||
| if should_label_not_found && !custom_span_label { |
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.
do this in the caller and only have a single bool argument?
| let mut restrict_type_params = false; | ||
| let mut suggested_derive = false; | ||
| let mut unsatisfied_bounds = false; |
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.
these 3 bools are very ☠️ would be nice to look into how to make them unnecessary or somehow unify their intended impact
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.
mhhhh Some of theses errors are mutually exclusive : restrict_type_params and suggested_derive. So an enum perhaps... what do you think ?
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.
This enum might be returned by the function suggest_unsatisfied_ty_or_trait
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.
so I think there's "what's the current behavior" and "what should it be". As in, should these errors be exclusive or would it be fine to simplify the control flow here.
I think if you want to keep this as a purely structural change, having 3 bools isn#t the worst thing in the world.
Idk how I feel about an enum, it might be slightly cleaner, but my issue is mainly that we have this mangled controlflow in the first place :>
177bb33 to
2057779
Compare
This comment has been minimized.
This comment has been minimized.
|
I have splitted the changes into severals commits, it will be easier to review. I have included most of the requested changes already. |
|
The conversations I kept opened are still unclear to me, but at least we can discuss on a sound basis (and reviewable) |
…od_error Currently this method is quiet long and complex, this commit refactors it and improves its readability by adding sub-fn
…thod_error Currently this method is quiet long and complex, this commit refactors it and improves its readability by adding sub-fn
…hod_error Currently this method is quiet long and complex, this commit refactors it and improves its readability by adding sub-fn
…d_error Currently this method is quiet long and complex, this commit refactors it and improves its readability by adding sub-fn
…o_match_method_error Currently this method is quiet long and complex, this commit refactors it and improves its readability by adding sub-fn
…t_no_match_method_error Currently this method is quiet long and complex, this commit refactors it and improves its readability by adding sub-fn
…eport_no_match_method_error Currently this method is quiet long and complex, this commit refactors it and improves its readability by adding sub-fn
Currently this method is quiet long and complex, this commit improves its readability, refactor and cleanup few things
2057779 to
4c952eb
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r+ rollup |
…or-refactoring, r=lcnr Cleanup and refactor FnCtxt::report_no_match_method_error As discussed on zulip with `@lcnr,` this is a proposal for refactorizing this method. See [#t-compiler/help > (PR rust-lang#144674) Merge multiple suggestions into a single @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/.28PR.20.23144674.29.20Merge.20multiple.20suggestions.20into.20a.20single/near/539991695)
…or-refactoring, r=lcnr Cleanup and refactor FnCtxt::report_no_match_method_error As discussed on zulip with ``@lcnr,`` this is a proposal for refactorizing this method. See [#t-compiler/help > (PR rust-lang#144674) Merge multiple suggestions into a single @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/.28PR.20.23144674.29.20Merge.20multiple.20suggestions.20into.20a.20single/near/539991695)
Rollup of 8 pull requests Successful merges: - #147736 (Stabilize `asm_cfg`) - #148652 (Cleanup and refactor FnCtxt::report_no_match_method_error) - #149167 (skip checking supertraits in object candidate for `NormalizesTo` goal) - #149210 (fix: Do not ICE on normalization failure of an extern static item) - #149268 (add implementation-internal namespace for globalasm) - #149274 (Fix invalid link generation for type alias methods) - #149302 (Fix comment wording in simplify_comparison_integral.rs) - #149305 (Simplify OnceCell Clone impl) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #148652 - rperier:report_no_match_method_error-refactoring, r=lcnr Cleanup and refactor FnCtxt::report_no_match_method_error As discussed on zulip with ```@lcnr,``` this is a proposal for refactorizing this method. See [#t-compiler/help > (PR #144674) Merge multiple suggestions into a single @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/.28PR.20.23144674.29.20Merge.20multiple.20suggestions.20into.20a.20single/near/539991695)
Rollup of 8 pull requests Successful merges: - rust-lang/rust#147736 (Stabilize `asm_cfg`) - rust-lang/rust#148652 (Cleanup and refactor FnCtxt::report_no_match_method_error) - rust-lang/rust#149167 (skip checking supertraits in object candidate for `NormalizesTo` goal) - rust-lang/rust#149210 (fix: Do not ICE on normalization failure of an extern static item) - rust-lang/rust#149268 (add implementation-internal namespace for globalasm) - rust-lang/rust#149274 (Fix invalid link generation for type alias methods) - rust-lang/rust#149302 (Fix comment wording in simplify_comparison_integral.rs) - rust-lang/rust#149305 (Simplify OnceCell Clone impl) r? `@ghost` `@rustbot` modify labels: rollup
As discussed on zulip with @lcnr, this is a proposal for refactorizing this method.
See #t-compiler/help > (PR #144674) Merge multiple suggestions into a single @ 💬