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

Rename some RegionKind variants to be inline with TyKind/ConstKind #95

Closed
1 of 3 tasks
BoxyUwU opened this issue Jul 11, 2023 · 7 comments
Closed
1 of 3 tasks

Rename some RegionKind variants to be inline with TyKind/ConstKind #95

BoxyUwU opened this issue Jul 11, 2023 · 7 comments
Labels
final-comment-period The FCP has started, most (if not all) team members are in agreement major-change A major change proposal major-change-accepted An accepted major change proposal T-types Add this label so rfcbot knows to poll the types team to-announce Announce this issue on triage meeting

Comments

@BoxyUwU
Copy link
Member

BoxyUwU commented Jul 11, 2023 •
edited
Loading

Proposal

I would like to rename:
RegionKind::EarlyBound to RegionKind::EarlyParam
RegionKind::Free to RegionKind::LateParam
RegionKind::LateBound to RegionKind::Bound

I find this to be far more consistent with TyKind/ConstKind. It's taken me a while for it to properly stick what all the 3 variants are on RegionKind since they're so differently named than TyKind, and also have an entire extra variant for representing lifetime parameters. I think the meaning of RegionKind::Free would have been a lot clearer with RegionKind::LateParam.

I also think it's kind of confusing that RegionKind::EarlyBound gets used for early bound generic paramaters but we do not use RegionKind::LateBound for late bound generic parameters. I think its unnecessarily confusing to draw a parallel between ReLateBound/ReEarlyBound and the concept of late/early bound generic parameters so directly when it does not actually apply. I am comfortable with drawing the parallel between EarlyParam/LateParam since it does not actually say Early/Late bound and in this case the parallel is actually present.

I would also like to avoid drawing a similarity between generic parameters and bound vars right now since we do not actually represent params with bound vars right now (and if we did we'd have to rename RegionKind::LateBound to RegionKind::Bound anyway).

Renaming EarlyBound/Free is it a bit of a wasted effort if we end up removing them in favor of placeholders and boundvars but this seems really simple to do wheras it's probably more complex to replace our param representation with bound vars as was briefly discussed in the t-types meeting on early/late bound parameters.

I do not work with regions very much since I don't touch borrowck much. I'd feel much more comfortable if someone who has spent a fair amount of time working with regions would approve this or say this is a coherent naming scheme for the regions.

Mentors or Reviewers

I can review this or implement this or mentor this. I think really anybody on t-compiler-contributors or t-types is qualified to do any of those 3 things.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A types team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Types team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
@BoxyUwU BoxyUwU added major-change A major change proposal T-types Add this label so rfcbot knows to poll the types team labels Jul 11, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2023

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/types

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Jul 11, 2023
@compiler-errors
Copy link
Member

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Jul 11, 2023
@BoxyUwU BoxyUwU closed this as completed Jul 14, 2023
@BoxyUwU BoxyUwU reopened this Aug 21, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 21, 2023

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/types

@compiler-errors
Copy link
Member

yeet

@rustbot second

@compiler-errors
Copy link
Member

cc https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/naming.20bikeshedding/near/386440383

@WaffleLapkin
Copy link
Member

Should this be considered accepted? the "second" comment was 2 months ago

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Oct 25, 2023

Yeah it should lol

@BoxyUwU BoxyUwU added the major-change-accepted An accepted major change proposal label Oct 25, 2023
@BoxyUwU BoxyUwU closed this as completed Oct 25, 2023
@lcnr lcnr mentioned this issue Nov 13, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 13, 2023
@bors
Auto merge of rust-lang#117876 - lcnr:region-kind-rename, r=BoxyUwU
4bd2fd5 
`ReLateBound` -> `ReBound`

first step of rust-lang/types-team#95

already fairly large xx

there's some future work here I intentionally did not contribute as part of this PR, from my notes:
- `DescriptionCtx` to `DescriptionCtxt`
- what is `CheckRegions::Bound`?
- `collect_late_bound_regions` et al
- `erase_late_bound_regions` -> `instantiate_bound_regions_with_erased`?
- `EraseEarlyRegions` should be removed, feels duplicate

r? `@BoxyUwU`
@lcnr lcnr mentioned this issue Nov 14, 2023
Merged
6 tasks
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 14, 2023
@bors
Auto merge of rust-lang#117908 - lcnr:region-kind-rename, r=BoxyUwU
3da0593 
finish `RegionKind` renaming

second step of rust-lang/types-team#95

continues the work from rust-lang#117876. While working on this and I encountered a bunch of further cleanup which I'll either open a tracking issue for or will do in a separate PR:
- rewrite the `RegionKind` docs, they still talk about `ReEmpty` and are generally out of date
- rename `DescriptionCtx` to `DescriptionCtxt`
- what is `CheckRegions::Bound`?
- `collect_late_bound_regions` et al
- `erase_late_bound_regions` -> `instantiate_bound_regions_with_erased`?
- `EraseEarlyRegions` visitor should be removed, feels duplicate

r? `@BoxyUwU`
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 15, 2023
@bors
Auto merge of rust-lang#117908 - lcnr:region-kind-rename, r=BoxyUwU
316629e 
finish `RegionKind` renaming

second step of rust-lang/types-team#95

continues the work from rust-lang#117876. While working on this and I encountered a bunch of further cleanup which I'll either open a tracking issue for or will do in a separate PR:
- rewrite the `RegionKind` docs, they still talk about `ReEmpty` and are generally out of date
- rename `DescriptionCtx` to `DescriptionCtxt`
- what is `CheckRegions::Bound`?
- `collect_late_bound_regions` et al
- `erase_late_bound_regions` -> `instantiate_bound_regions_with_erased`?
- `EraseEarlyRegions` visitor should be removed, feels duplicate

r? `@BoxyUwU`
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 15, 2023
@bors
Auto merge of rust-lang#117908 - lcnr:region-kind-rename, r=BoxyUwU
1500db7 
finish `RegionKind` renaming

second step of rust-lang/types-team#95

continues the work from rust-lang#117876. While working on this and I encountered a bunch of further cleanup which I'll either open a tracking issue for or will do in a separate PR:
- rewrite the `RegionKind` docs, they still talk about `ReEmpty` and are generally out of date
- rename `DescriptionCtx` to `DescriptionCtxt`
- what is `CheckRegions::Bound`?
- `collect_late_bound_regions` et al
- `erase_late_bound_regions` -> `instantiate_bound_regions_with_erased`?
- `EraseEarlyRegions` visitor should be removed, feels duplicate

r? `@BoxyUwU`
bors added a commit to rust-lang/miri that referenced this issue Nov 16, 2023
@bors
Auto merge of #117908 - lcnr:region-kind-rename, r=BoxyUwU
cb1ea1b 
finish `RegionKind` renaming

second step of rust-lang/types-team#95

continues the work from #117876. While working on this and I encountered a bunch of further cleanup which I'll either open a tracking issue for or will do in a separate PR:
- rewrite the `RegionKind` docs, they still talk about `ReEmpty` and are generally out of date
- rename `DescriptionCtx` to `DescriptionCtxt`
- what is `CheckRegions::Bound`?
- `collect_late_bound_regions` et al
- `erase_late_bound_regions` -> `instantiate_bound_regions_with_erased`?
- `EraseEarlyRegions` visitor should be removed, feels duplicate

r? `@BoxyUwU`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
@bors
Auto merge of #117908 - lcnr:region-kind-rename, r=BoxyUwU
eedf0c4 
finish `RegionKind` renaming

second step of rust-lang/types-team#95

continues the work from #117876. While working on this and I encountered a bunch of further cleanup which I'll either open a tracking issue for or will do in a separate PR:
- rewrite the `RegionKind` docs, they still talk about `ReEmpty` and are generally out of date
- rename `DescriptionCtx` to `DescriptionCtxt`
- what is `CheckRegions::Bound`?
- `collect_late_bound_regions` et al
- `erase_late_bound_regions` -> `instantiate_bound_regions_with_erased`?
- `EraseEarlyRegions` visitor should be removed, feels duplicate

r? `@BoxyUwU`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
@bors
Auto merge of #117908 - lcnr:region-kind-rename, r=BoxyUwU
f7cf322 
finish `RegionKind` renaming

second step of rust-lang/types-team#95

continues the work from #117876. While working on this and I encountered a bunch of further cleanup which I'll either open a tracking issue for or will do in a separate PR:
- rewrite the `RegionKind` docs, they still talk about `ReEmpty` and are generally out of date
- rename `DescriptionCtx` to `DescriptionCtxt`
- what is `CheckRegions::Bound`?
- `collect_late_bound_regions` et al
- `erase_late_bound_regions` -> `instantiate_bound_regions_with_erased`?
- `EraseEarlyRegions` visitor should be removed, feels duplicate

r? `@BoxyUwU`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
final-comment-period The FCP has started, most (if not all) team members are in agreement major-change A major change proposal major-change-accepted An accepted major change proposal T-types Add this label so rfcbot knows to poll the types team to-announce Announce this issue on triage meeting
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@compiler-errors @BoxyUwU @WaffleLapkin @rustbot