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

Document recovered field in VariantData. #119121

Conversation

aDotInTheVoid
Copy link
Member

In hir, their was a comment, but it didn't show up in rustdoc. In the AST, it was entirely undocumented, and the meaning could only be figured out by looking at usages. Hopefully this helps the next person looking at this.

@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2023

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

@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 Dec 19, 2023
@@ -2788,6 +2788,8 @@ pub enum VariantData {
/// Struct variant.
///
/// E.g., `Bar { .. }` as in `enum Foo { Bar { .. } }`.
///
/// The `bool` is whether it was recovered by the parser.
Struct(ThinVec<FieldDef>, bool),
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
Struct(ThinVec<FieldDef>, bool),
Struct { fields: ThinVec<FieldDef>, recovered: bool },

Personally speaking, I'd prefer the variant fields to be named. Not sure how big the fallout of this change would be though.

Copy link
Member Author

Choose a reason for hiding this comment

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

gh-aDotInTheVoid@dev-desktop-eu-1:~/rust$ rg "VariantData::Struct" | wc -l
52

Thats doable, if people are OK with the churn. Not sure what exactly the policy is, but I'd be happy to do that if it'd be accepted.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, go ahead and do that. Please just make sure it's in a separate commit.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also wondering if that field should be Option<ErrorGuaranteed> instead of bool. Can be a separate PR, but maybe leave a FIXME?

// FIXME: investigate making this a `Option<ErrorGuaranteed>`

Copy link
Member Author

Choose a reason for hiding this comment

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

Please just make sure it's in a separate commit.

Seperate to what? I think the best thing to do would be do open a new PR, as it wouldn't have any of the changes from this diff.

Copy link
Member

Choose a reason for hiding this comment

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

Separate commit as in separate commit in this PR, lol. I don't see any need to split it out from this PR, but up to you.

@aDotInTheVoid
Copy link
Member Author

Superseded by #119145. I made a new PR because I started developing it from master (as it didn't need any of the changes here), and couldn't figure out how to make git/github have that commit in this PR

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 20, 2023
…truct, r=compiler-errors

Give `VariantData::Struct`  named fields, to clairfy `recovered`.

Implements rust-lang#119121 (comment). Supersedes rust-lang#119121

This way, it's clear what the bool fields means, instead of having to find where it's generated. Changes both ast and hir.

r? `@compiler-errors`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2023
Rollup merge of rust-lang#119145 - aDotInTheVoid:variantdata-struct-struct, r=compiler-errors

Give `VariantData::Struct`  named fields, to clairfy `recovered`.

Implements rust-lang#119121 (comment). Supersedes rust-lang#119121

This way, it's clear what the bool fields means, instead of having to find where it's generated. Changes both ast and hir.

r? `@compiler-errors`
@WaffleLapkin WaffleLapkin removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants