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

Check RPIT and AFIT hidden types are well-formed considering regions #114740

Closed

Conversation

compiler-errors
Copy link
Member

Fixes #114728

r? @aliemjay @lcnr or @oli-obk (who's on vacation but he shall see it when he gets back 🌴)

@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 Aug 11, 2023
@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 11, 2023
@bors
Copy link
Contributor

bors commented Aug 11, 2023

⌛ Trying commit fa1e4ba with merge d809384186eaa72b56b7b956dcf0837de4a00262...

Comment on lines 468 to 470
hir::OpaqueTyOrigin::TyAlias { .. }
if tcx.def_kind(tcx.parent(def_id.to_def_id())) == DefKind::OpaqueTy => {}
// Can have different predicates to their defining use
Copy link
Member Author

Choose a reason for hiding this comment

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

We could, perhaps, define the assumed wf types of a TAIT to be any types that show up in the bounds of a parent TAIT.

Don't think it's worth unless someone can come up with an unsoundness test here, though.

@bors
Copy link
Contributor

bors commented Aug 11, 2023

☀️ Try build successful - checks-actions
Build commit: d809384186eaa72b56b7b956dcf0837de4a00262 (d809384186eaa72b56b7b956dcf0837de4a00262)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d809384186eaa72b56b7b956dcf0837de4a00262): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-5.1% [-6.0%, -4.6%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -5.1% [-6.0%, -4.6%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.3% [1.3%, 1.3%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.3% [1.3%, 1.3%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.671s -> 631.783s (0.02%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 12, 2023
@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-114740 created and queued.
🤖 Automatically detected try build d809384186eaa72b56b7b956dcf0837de4a00262
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-114740 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@lcnr
Copy link
Contributor

lcnr commented Aug 14, 2023

r? @lcnr
r=me after crater + fcp

@rust-lang/types we previously did not regionck RPITs and async function return types because we assumed that they would already be checked by their parent function.

This is not the case in case we end up "subtyping" into an RPIT, see #114728. This PR causes us to now always check regions for RPITs as well.

@rfcbot fcp merge

@rfcbot

This comment was marked as outdated.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 14, 2023
@rfcbot rfcbot added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Aug 14, 2023
@lcnr lcnr added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 14, 2023
@lcnr
Copy link
Contributor

lcnr commented Aug 14, 2023

sorry

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Aug 14, 2023

@lcnr proposal cancelled.

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 14, 2023
@rfcbot rfcbot removed the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Aug 14, 2023
@lcnr
Copy link
Contributor

lcnr commented Aug 14, 2023

#114740 (comment)

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Aug 14, 2023

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 14, 2023
@aliemjay
Copy link
Member

aliemjay commented Aug 15, 2023

Here is a couple of what I think are unnecessary breakages of this implementation:

struct Static<T: 'static>(T);
fn test<' a>(s: Static<&'static u8>) -> impl Sized + 'a { s } 
  • Another more contreversial one:
trait Id<X> { type Ty; }
impl<X> Id<X> for () { type Ty = X; }
fn test() -> impl Id<&'static T, Ty = impl Sized> {}

Both are valid patterns that predate #94081. I believe we can keep supporting them by doing the WF check where subtyping is involved, namely in nll_relate and higher-ranked equality (which involves mutual subtyping).

@craterbot
Copy link
Collaborator

🎉 Experiment pr-114740 is completed!
📊 298 regressed and 2 fixed (348705 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 16, 2023
@compiler-errors
Copy link
Member Author

@craterbot
Copy link
Collaborator

👌 Experiment pr-114740-1 created and queued.
🤖 Automatically detected try build d809384186eaa72b56b7b956dcf0837de4a00262
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 16, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-114740-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-114740-1 is completed!
📊 255 regressed and 0 fixed (298 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 18, 2023
@lcnr
Copy link
Contributor

lcnr commented Aug 18, 2023

@aliemjay is trying an alternative approach in #114933, still in a crater run. It's definitely worth it to wait until that crater run is finished before making progress here.

@lcnr lcnr added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2023
@compiler-errors
Copy link
Member Author

I'm gonna close this in favor of the other

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPIT hidden types can be ill-formed
8 participants