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

Mark DefineOpaqueTypes::Yes use-sites uncovered by tests as FIXMEs #117369

Closed
wants to merge 36 commits into from

Conversation

lqd
Copy link
Member

@lqd lqd commented Oct 29, 2023

I noticed #116652 earlier, and this PR marks all the DefineOpaqueTypes::No use-sites that didn't make any test fail when flipped to DefineOpaqueTypes::Yes (each into their dedicated commit as requested in that issue).

r? @compiler-errors

lqd added 30 commits October 29, 2023 19:28
@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 Oct 29, 2023
@matthiaskrgr
Copy link
Member

duplicate of #117348 ? 😅

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.

Hm... I actually wonder if we may just want to track these usage sites as issues, rather than FIXMEs in the code.

Ideally, I would like to have interested individuals (or maybe just myself) study the way that the code is being used and create tests from that (or conclude that tests cannot be created and switch them to DefineOpaqueTypes::Yes).

Also, each usage site alone is a bit too granular. Except for candidate assembly and selection in the new trait solver, we probably want to group (e.g.) all of the method probe usages together, and all of the coercion usages together, etc.

@@ -165,6 +165,7 @@ fn visit_implementation_of_dispatch_from_dyn(tcx: TyCtxt<'_>, impl_did: LocalDef
use rustc_type_ir::TyKind::*;
match (source.kind(), target.kind()) {
(&Ref(r_a, _, mutbl_a), Ref(r_b, _, mutbl_b))
// FIXME(DefineOpaqueTypes): no test exercizes using `DefineOpaqueTypes::Yes` below.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// FIXME(DefineOpaqueTypes): no test exercizes using `DefineOpaqueTypes::Yes` below.
// FIXME(DefineOpaqueTypes): no test exercises using `DefineOpaqueTypes::Yes` below.

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL

@lqd
Copy link
Member Author

lqd commented Oct 29, 2023

duplicate of #117348 ? 😅

I saw that after starting and it's more complementary: it flipped uses to DefineOpaqueTypes::Yes which I didn't do (and I don't believe was expected, if I understood the steps in #116652 correctly), and didn't mark the places that were not covered by tests which I did do.

Some of the new DefineOpaqueTypes::Yes in that PR will land, while all the FIXMEs for the others that probably won't land are in this PR.

@compiler-errors
Copy link
Member

And yeah, this work is kind-of underway in #117348.

I actually slightly regret the approach suggested #116652. Seems a bit too much like busy work -- creating 36 commits that all just add a single FIXME to a file seems... kinda pointless?

Ideally we'd be working incrementally towards our goal of actually getting rid of DefineOpaqueTypes::No, and while this is a step in the right direction, it also seems a bit like churn.

@lqd
Copy link
Member Author

lqd commented Oct 29, 2023

Ok then let's close this.

@lqd lqd closed this Oct 29, 2023
@lqd lqd deleted the define-opaque-types branch October 29, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants