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

WIP: Normalize associated types in structs when performing unsizing coercion #87548

Closed
wants to merge 2 commits into from
Closed

WIP: Normalize associated types in structs when performing unsizing coercion #87548

wants to merge 2 commits into from

Conversation

WaffleLapkin
Copy link
Member

Closes #75899

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2021
@WaffleLapkin WaffleLapkin marked this pull request as draft July 28, 2021 13:10
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
tidy: Skipping binary file check, read-only filesystem
Checking which error codes lack tests...
* 625 error codes
* highest error code: E0783
tidy error: /checkout/compiler/rustc_typeck/src/check/coercion.rs:607: trailing whitespace
tidy error: /checkout/compiler/rustc_typeck/src/check/coercion.rs:610: trailing whitespace
tidy error: /checkout/compiler/rustc_typeck/src/check/coercion.rs:614: trailing whitespace
Found 0 error codes with no tests
Done!
* 342 features
some tidy checks failed
some tidy checks failed


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build" "16"


Build completed unsuccessfully in 0:00:13

@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
jackh726
jackh726 requested changes Oct 21, 2021
View reviewed changes
compiler/rustc_typeck/src/check/coercion.rs

debug!("coerce_unsized: normalized {:?} to {:?}; obls: {:?}", trait_pred, trait_pred2, obls);
let trait_pred = trait_pred2;
// coercion.obligations.extend(obls);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this commented out? We definitely shouldn't drop these obligations.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is very WIP and actually even abandoned 😅

At the time, I was experimenting with this and was hitting problems I couldn't understand, so I was commenting out different parts in an attempt to understand what and how (doesn't) work.

We've discussed this with Niko in a zulip thread and he said that it would probably be better to remove the "special trait solver" altogether:

nikomatsakis: as it happens, I would love to remove this part of the coercion code that you are fixing
nikomatsakis: I'm not sure if we can get away with it, but it's going to be really painful to make a part of chalk
nikomatsakis: the reason I dislike this code is that it is basically reproducing part of the normal "evaluate whether a trait matches" logic
nikomatsakis: but doing it in a custom and different way
nikomatsakis: anyway, I imagine that this is part of why you are seeing different behavior
nikomatsakis: I'm debating whether I can convince you into trying to remove this logic, instead of fixing it

I wanted to work on this (the issue I'm trying to fix really bothers me), but this never ended up happening.

I can try to revive this PR or try to make the refactor Niko proposed. But I'd need some guidance since I haven't worked on the compiler much before, so I'm not sure if I'll help or just spend someones time 😅

I'm assuming since you've assigned yourself, you know this part of the compiler well?

@jackh726 jackh726 assigned jackh726 and unassigned nikomatsakis Oct 21, 2021
@jackh726 jackh726 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 Oct 21, 2021
@jackh726
Copy link
Member

This is very WIP and actually even abandoned

I'm going to go ahead and close this as inactive...feel free to reopen if you want to continue working on it

@jackh726 jackh726 closed this Dec 18, 2021
@jackh726 jackh726 added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 18, 2021
@WaffleLapkin WaffleLapkin deleted the coerce_normalization branch December 15, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 requested changes

Assignees

@jackh726 jackh726

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unsizing coercion does not normalize associated types in structs.
6 participants
@WaffleLapkin @rust-log-analyzer @jackh726 @nikomatsakis @rust-highfive @inquisitivecrystal