Skip to content

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Aug 10, 2023

Fixes #114636

This PR eliminates the dummy field, initially introduced in #113999, thereby enabling unrestricted use of ident.unwrap(). A side effect of this action is that we can only report the error of the first macro invocation field within the struct node.

An alternative solution might be giving a virtual name to the macro, but it appears more complex.(#114636 (comment)). Furthermore, if you think #114636 (comment) is a better solution, feel free to close this PR.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Aug 10, 2023

cc @Centri3

@rustbot
Copy link
Collaborator

rustbot commented Aug 10, 2023

r? @compiler-errors

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

This is a somewhat sad regression but it's fine imo

@compiler-errors
Copy link
Member

compiler-errors commented Aug 10, 2023

r=me when ci is green

@bors delegate+ rollup

@bors
Copy link
Collaborator

bors commented Aug 10, 2023

✌️ @bvanjoi, you can now approve this pull request!

If @compiler-errors told you to "r=me" after making some further change, please make that change, then do @bors r=@compiler-errors

@Centri3
Copy link
Member

Centri3 commented Aug 10, 2023

An alternative solution might be giving a virtual name to the macro, but it appears more complex.

It really isn't. You can easily give it its name by using the already parsed name variable, though I think having multiple fields with the same name is probably breaking an invariant of named structs. Maybe not? I'm not sure but I think this is something disallowed really early on, as afaik macro expansion doesn't even happen if there are multiple fields with the same name.

Instead, I think, if you want to go through the trouble, make parse_name_and_ty return an Option<FieldDef> and push it onto the field list only if it's Some. Otherwise, just continue and act as though it never existed.

We'll also need to edit parse_single_struct_field and parse_field_def. After that, since it unwraps the Err there, we can just not push it if it's None.

This shouldn't even be that big of a performance regression since all it is is usually a pointer under the hood, and a single null check after the call.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Aug 11, 2023

make parse_name_and_ty return an Option and push it onto the field list only if it's Some

This represents an improved solution.

@rustbot ready

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.

Sorry for the back and forth @bvanjoi, but I thought about this some more and I'm not convinced that all of the changes are worth it. Let's just go with the simple approach and bail eagerly.

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Aug 21, 2023

Sorry for the back and forth

Not to worry. As a beginner, every piece of advice is invaluable.

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

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Aug 30, 2023

📌 Commit 3ed435f has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#113565 (Make SIGSEGV handler emit nicer backtraces)
 - rust-lang#114704 (parser: not insert dummy field in struct)
 - rust-lang#115272 (miri/diagnostics: don't forget to print_backtrace when ICEing on unexpected errors)
 - rust-lang#115313 (Make `get_return_block()` return `Some` only for HIR nodes in body)
 - rust-lang#115347 (suggest removing `impl` in generic trait bound position)
 - rust-lang#115355 (new solver: handle edge case of a recursion limit of 0)
 - rust-lang#115363 (Don't suggest adding parentheses to call an inaccessible method.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6391165 into rust-lang:master Aug 30, 2023
@rustbot rustbot added this to the 1.74.0 milestone Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

ICE: None when deriving Debug of struct containing unreachable!()
5 participants