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

Properly deal with GATs when looking for method chains to point at #121912

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

fmease
Copy link
Member

@fmease fmease commented Mar 2, 2024

Fixes #121898.

While it prevents an ICE and the structured suggestion is correct, the method chain diagnostic notes are weird / useless / incorrect judging by a quick look. I guess I should improve that in this PR. Sufficiently taken care of.

r? estebank or compiler-errors (#105332, #105674).

@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 Mar 2, 2024
| ^ `Base::Base` is `<T as Base>::Base<_>` here
...
LL | input.fmap(f1).fmap(f2)
| -------- `Base::Base` remains `_` here
Copy link
Member Author

Choose a reason for hiding this comment

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

useless

Copy link
Contributor

Choose a reason for hiding this comment

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

We can likely just check for _ and avoid the label then, as a quick change.

Copy link
Member Author

@fmease fmease Mar 2, 2024

Choose a reason for hiding this comment

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

I can either change the select_where_possible a few lines below my changes to select_all_or_error which would make this note go away or instead add an is_ty_var check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check if select_all_or_error will result in any currently "good" note going away too?

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'll make CI do the dirty work...

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 can either change the select_where_possible a few lines below my changes to select_all_or_error which would make this note go away or instead add an is_ty_var check.

Went with is_ty_var since select_all_or_error turned out to be too strict.

--> $DIR/method-chain-gats.rs:13:29
|
LL | fn fmap2<T, A, B, C>(input: T, f1: impl Fn(A) -> B, f2: impl Fn(B) -> C) -> T::Base<C>
| ^ `Base::Base` is `<T as Base>::Base<_>` here
Copy link
Member Author

@fmease fmease Mar 2, 2024

Choose a reason for hiding this comment

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

wrong location I suppose?

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 the location is "right", because the logic is looking for the root element of the method chain (even accounting for let bindings and fn arguments), so this is trying to say that <Base>::Base<_> = <T as Base>::Base<_>, which is somewhat useless information.

I do have to say that the test code makes my head spin, even without accounting for the error message 😬

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 the only thing left to do is to improve this label to better account for GATs, but that shouldn't happen in this PR.

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2024
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

The spurious labels seem pre-existing. r=me if you don't want to rework them in this PR.

@rust-log-analyzer

This comment has been minimized.

@fmease fmease added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 2, 2024
@estebank
Copy link
Contributor

estebank commented Mar 3, 2024

@bors r=compiler-errors,estebank

@bors
Copy link
Contributor

bors commented Mar 3, 2024

📌 Commit 6035e87 has been approved by compiler-errors,estebank

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 Mar 3, 2024
@bors
Copy link
Contributor

bors commented Mar 3, 2024

⌛ Testing commit 6035e87 with merge 31eb43b...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2024
…piler-errors,estebank

Properly deal with GATs when looking for method chains to point at

Fixes rust-lang#121898.

~~While it prevents an ICE and the structured suggestion is correct, the method chain diagnostic notes are weird / useless / incorrect judging by a quick look. I guess I should improve that in this PR.~~ Sufficiently taken care of.

r? estebank or compiler-errors (rust-lang#105332, rust-lang#105674).
@rust-log-analyzer
Copy link
Collaborator

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Mar 3, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 3, 2024
@fmease
Copy link
Member Author

fmease commented Mar 3, 2024

Network failure
@bors retry

@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 Mar 3, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2024
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#121130 (Suggest moving definition if non-found macro_rules! is defined later)
 - rust-lang#121912 (Properly deal with GATs when looking for method chains to point at)
 - rust-lang#121927 (Add a proper `with_no_queries` to printing)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cd9e5b5 into rust-lang:master Mar 4, 2024
11 of 12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2024
Rollup merge of rust-lang#121912 - fmease:diag-method-chains-gat, r=compiler-errors,estebank

Properly deal with GATs when looking for method chains to point at

Fixes rust-lang#121898.

~~While it prevents an ICE and the structured suggestion is correct, the method chain diagnostic notes are weird / useless / incorrect judging by a quick look. I guess I should improve that in this PR.~~ Sufficiently taken care of.

r? estebank or compiler-errors (rust-lang#105332, rust-lang#105674).
@fmease fmease deleted the diag-method-chains-gat branch March 4, 2024 12:31
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.

ICE: type parameter B/#1 (B/#1/1) out of range when instantiating
6 participants