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

Tait must be constrained if in sig #113169

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 29, 2023

r? @compiler-errors

kind of reverts #62423, but that PR only removed cycles in cases that we want to forbid now anyway.

see https://rust-lang.zulipchat.com/#narrow/stream/315482-t-compiler.2Fetc.2Fopaque-types/topic/lcnr.20oli.20meeting/near/370712246 for related discussion and motivating example

a TAIT showing up in a signature doesn't just mean it can register a hidden type, but that it must.

This is the design the types team decided upon for the following reasons

  • avoids a hypothetical situation where getting smarter in trait solving could cause new cycle errors or new inference errors to show up, where today something is treated as opaque.
  • avoids having the situation where a (minor?) change to a function body causes an error, because its TAIT usage suddenly isn't "opaque enough" anymore.
  • avoids having to explain why in some cases you need to put a Tait into a tiny module together with its defining usages. Now you basically gotta always do this, the moment a Tait is in a signature that isn't in the defining scope.
  • avoids false-cycle errors
  • anything diverging from this pattern needs to either
    • move the TAIT declaration and defining function into their own helper sub module
    • use the attributes we'll provide in the future to explicitly opt in or out of being in the defining scope

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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 Jun 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 29, 2023

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

@@ -1,5 +1,4 @@
#![feature(type_alias_impl_trait)]
// check-pass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only test for which there was no workaround after this change.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 5, 2023

☔ The latest upstream changes (presumably #113370) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk closed this Aug 3, 2023
@apiraino apiraino removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2023
@oli-obk oli-obk reopened this Oct 24, 2023
@oli-obk oli-obk force-pushed the tait_must_be_constrained_if_in_sig branch 2 times, most recently from b6612f4 to 62d2ddd Compare October 24, 2023 13:12
@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the tait_must_be_constrained_if_in_sig branch from 62d2ddd to 64f3f5b Compare October 24, 2023 13:36
Comment on lines -1 to -2
// check-pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR "reopens" #61863

the test doesn't really add too much new test coverage compared with other tests we already have.

@bors
Copy link
Contributor

bors commented Oct 25, 2023

☔ The latest upstream changes (presumably #117076) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 25, 2023
@oli-obk oli-obk force-pushed the tait_must_be_constrained_if_in_sig branch from 64f3f5b to fd06ad3 Compare October 25, 2023 13:04
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 25, 2023

☔ The latest upstream changes (presumably #117172) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk force-pushed the tait_must_be_constrained_if_in_sig branch from f0a4804 to 1d5ea10 Compare October 26, 2023 06:56
@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 30, 2023

@rustbot ready

@rustbot rustbot 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 Oct 30, 2023
@compiler-errors
Copy link
Member

I need to take another look at this, but blocking until it's been approved from the T-lang side.

@rustbot blocked

@rustbot rustbot 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 Nov 6, 2023
@bors
Copy link
Contributor

bors commented Nov 17, 2023

☔ The latest upstream changes (presumably #117278) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk added the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Jan 3, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 3, 2024

Nominating for lang team discussion. The explanation is entirely in the main post of this PR.

The question we want T-lang to answer is:

Is it ok from your side that we disallow

type Tait = impl Sized;
fn foo(x: Tait) -> Tait { x } // not allowed, because not defining in defining scope
fn bar() -> Tait { }

but allow the same code if the foo function is removed (or moved to a parent or sibling module)

@traviscross
Copy link
Contributor

This is also nominated as #117861. Also see #117865.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

Let's unnominate this in favor of #117861 which gets at the same question.

@rustbot rustbot removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Jan 17, 2024
@lcnr lcnr mentioned this pull request Jan 26, 2024
57 tasks
@Dylan-DPC
Copy link
Member

@oli-obk what's the status of this? it was blocked as it was nominated but then unnominated for a different pr, so i'm assuming that it is either superseded or blocked by it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. 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.

None yet

8 participants