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.

Sign up for GitHub

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

Jump to bottom

Don't suggest impl Trait in path position #113310

Merged
merged 1 commit into from
Jul 11, 2023
Merged

Don't suggest impl Trait in path position #113310

merged 1 commit into from
Jul 11, 2023

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Jul 4, 2023

Fixes #113264.

@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 4, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2023

Some changes occurred in need_type_info.rs

cc @lcnr

jieyouxu
jieyouxu commented Jul 4, 2023
View reviewed changes
compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs Outdated
Comment on lines 485 to 496
let ty_var_is_impl_trait = move |ty_vid| {
let mut infcx_inner = self.infcx.inner.borrow_mut();
let ty_vars = infcx_inner.type_variables();
let var_origin = ty_vars.var_origin(ty_vid);
if let TypeVariableOriginKind::TypeParameterDefinition(name, _) = var_origin.kind
&& name.as_str().starts_with("impl ")
{
true
} else {
false
}
};
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a less hacky way to do this check?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to change fn fmt_printer to not use the name ofTypeVariableOriginKind::TypeParameterDefinition if the DefId is from an argument position impl trait instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think reaching 11 indentation levels might be justification to refactor some of this to a function anyway 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to change fn fmt_printer to not use the name ofTypeVariableOriginKind::TypeParameterDefinition if the DefId is from an argument position impl trait instead.

How do you check if a DefId is from an arg position impl Trait?

Copy link
Contributor

Choose a reason for hiding this comment

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

tcx.generics_of(tcx.parent(def_id)), look at the https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/generics/struct.GenericParamDef.html for whether the type param is synthetic.

I am not 10% sure whether the parent is necessary 🤷 try it without, if it ices, add tcx.parent

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I think that works, but I'm not sure if it's 100% correct.

@lcnr lcnr mentioned this pull request Jul 4, 2023
Merged
@jieyouxu jieyouxu force-pushed the dont-suggest-impl-trait-in-paths branch from 78874c7 to aaf1a13 Compare July 5, 2023 07:50
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 5, 2023
@Dylan-DPC
Rollup merge of rust-lang#113319 - lcnr:type-param-def-def-id, r=comp…
b73344f 
…iler-errors

`TypeParameterDefinition` always require a `DefId`

the `None` case never actually reaches diagnostics so it feels better for diagnostics to be able to rely on the `DefId` being there, cc rust-lang#113310
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jul 5, 2023
@compiler-errors
Rollup merge of rust-lang#113319 - lcnr:type-param-def-def-id, r=comp…
5c7a7d9 
…iler-errors

`TypeParameterDefinition` always require a `DefId`

the `None` case never actually reaches diagnostics so it feels better for diagnostics to be able to rely on the `DefId` being there, cc rust-lang#113310
@jieyouxu jieyouxu force-pushed the dont-suggest-impl-trait-in-paths branch from aaf1a13 to fafc689 Compare July 7, 2023 14:44
lcnr
lcnr reviewed Jul 7, 2023
View reviewed changes
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.

one nit, after that r=me

@jieyouxu jieyouxu force-pushed the dont-suggest-impl-trait-in-paths branch from fafc689 to e6b9a55 Compare July 7, 2023 15:42
lcnr
lcnr reviewed Jul 7, 2023
View reviewed changes
compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs Outdated
Comment on lines 169 to 170
if let Some(idx) = generics.param_def_id_to_index(infcx.tcx, def_id)
&& let generic_param_def = generics.param_at(idx as usize, infcx.tcx)
Copy link
Contributor

@lcnr lcnr Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

please unwrap instead of using if let. Not getting Some here indicates a bug

Copy link
Member

Choose a reason for hiding this comment

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

very astute! #113610

@jieyouxu jieyouxu force-pushed the dont-suggest-impl-trait-in-paths branch from e6b9a55 to b5208b3 Compare July 7, 2023 16:04
@lcnr
Copy link
Contributor

lcnr commented Jul 11, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 11, 2023

📌 Commit b5208b3 has been approved by lcnr

It is now in the queue for this repository.

@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 Jul 11, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 11, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 11, 2023
@bors
Auto merge of rust-lang#113577 - matthiaskrgr:rollup-vaa83ip, r=matth…
e571544 
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#112717 (Implement a few more rvalue translation to smir)
 - rust-lang#113310 (Don't suggest `impl Trait` in path position)
 - rust-lang#113497 (Support explicit 32-bit MIPS ABI for the synthetic object)
 - rust-lang#113560 (Lint against misplaced where-clauses on associated types in traits)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c6df564 into rust-lang:master Jul 11, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 11, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee left review comments

@lcnr lcnr lcnr left review comments

@tgross35 tgross35 tgross35 left review comments

Assignees

@TaKO8Ki TaKO8Ki

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
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.

Compiler suggestion ping-pongs between E0283 and E0562
7 participants
@jieyouxu @rustbot @lcnr @bors @tgross35 @workingjubilee @TaKO8Ki