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

Suggest use .. to fill in the rest of the fields of Struct #103012

Merged
merged 3 commits into from
Nov 6, 2022

Conversation

chenyukang
Copy link
Member

Fixes #102806

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 13, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2022

rustc_error_messages was changed

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

@rust-highfive
Copy link
Collaborator

r? @eholk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2022
self.bump();
match self.parse_expr() {
Ok(_p) => {
self.sess.emit_err(MissingDotDot { token_span: span, sugg_span: span });
Copy link
Member

Choose a reason for hiding this comment

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

Looking better!

Can you actually set the base expression like above? Something like base = ast::StructRest::Base(e). Basically emulate the code above. That should suppress the "missing fields x and y in initializer of V3" error below.

Also, can handle the case of ... } like above?

Copy link
Member Author

Choose a reason for hiding this comment

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

oo, I left the "missing fields x and y in initializer of V3" intentionally, I thought it may helpful for let developer know which fields are missing.

And for the case of ...}, we need to suggest something like ..{expr}? So I left it as before also.

Copy link
Member

@compiler-errors compiler-errors Oct 14, 2022

Choose a reason for hiding this comment

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

I thought it may helpful for let developer know which fields are missing.

This is extra noise if the developer wrote ... instead of .., which is what your PR is trying to fix, right? 😆

And for the case of ...}, we need to suggest something like ..{expr}?

Not in the case of a pattern, which is the same place that .. } is valid.


Basically, we should treat ... typo as closely to .. as possible, especially if we're "recovering" parsing here.

Copy link
Member

Choose a reason for hiding this comment

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

It may actually be easier to merge this check with the .. logic above, since we're basically just duplicating that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will make a update.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the case of let V3 { z: val, ...} = v, we already have a check to report:

error: expected field pattern, found `...`
  --> /home/cat/code/rust/src/test/ui/parser/issue-102806.rs:23:22
   |
23 |     let V3 { z: val, ... } = v;
   |                      ^^^ help: to omit remaining fields, use one fewer `.`: `..`

Will keep same as before.

Copy link
Member Author

Choose a reason for hiding this comment

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

It may actually be easier to merge this check with the .. logic above, since we're basically just duplicating that.

Tried to merge the two checks, but I found split them seems more easy to read.

Copy link
Member

Choose a reason for hiding this comment

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

That's ok, I don't think it's necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Finally, merge it with the .. logic above 😁

@compiler-errors
Copy link
Member

r? @compiler-errors I can take over this review, and seems like @chenyukang almost has this ready 😸

@compiler-errors compiler-errors 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 Oct 22, 2022
@Noratrieb
Copy link
Member

Hi, I've seen you changed some diagnostic structs in your PR. After #103345, the way we refer to fluent messages changed. They are now in a flat namespace with the same identifier as in the fluent file. For example, parser::cool_thing is now parser_cool_thing and parser::suggestion just suggestion.
You should rebase to the latest master and change your fluent message references as described above. Thanks!

@bors

This comment was marked as resolved.

@chenyukang
Copy link
Member Author

You should rebase to the latest master and change your fluent message references as described above. Thanks!

Rebased update.
r? @compiler-errors

@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2022

Could not assign reviewer from: compiler-errors.
User(s) compiler-errors are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

@Rageking8
Copy link
Contributor

Rageking8 commented Nov 4, 2022

You should rebase to the latest master and change your fluent message references as described above. Thanks!

Rebased update.
r? @compiler-errors

Should be @rustbot ready to signify the PR is ready for another review and not r?.

Since I saw you doing r? in other PRs where you meant to say you are ready.

@chenyukang
Copy link
Member Author

chenyukang commented Nov 4, 2022

Should be @rustbot ready to signify the PR is ready for another review and not r?.

Thanks for tip 😁.

@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 Nov 4, 2022
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

r=me,compiler-errors after one comment

compiler/rustc_parse/src/parser/expr.rs Outdated Show resolved Hide resolved
@chenyukang chenyukang force-pushed the fix-102806 branch 2 times, most recently from 29a4924 to 4b77e73 Compare November 4, 2022 17:11
@bors
Copy link
Contributor

bors commented Nov 5, 2022

@chenyukang: 🔑 Insufficient privileges: Not in reviewers

@compiler-errors
Copy link
Member

compiler-errors commented Nov 6, 2022

@bors r=davidtwco,compiler-errors

I hope you didn't mind me renaming the function typo, but I didn't want to delay r+'ing still since I know you have been waiting 😄

@bors
Copy link
Contributor

bors commented Nov 6, 2022

📌 Commit 28d82dd has been approved by davidtwco,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 Nov 6, 2022
@chenyukang
Copy link
Member Author

@bors r=davidtwco,compiler-errors

I hope you didn't mind me renaming the function typo, but I didn't want to delay r+'ing still since I know you have been waiting 😄

Thanks very much.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 6, 2022
…,compiler-errors

Suggest use .. to fill in the rest of the fields of Struct

Fixes rust-lang#102806
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 6, 2022
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#103012 (Suggest use .. to fill in the rest of the fields of Struct)
 - rust-lang#103851 (Fix json flag in bootstrap doc)
 - rust-lang#103990 (rustdoc: clean up `.logo-container` layout CSS)
 - rust-lang#104002 (fix a comment in UnsafeCell::new)
 - rust-lang#104014 (Migrate test-arrow to CSS variables)
 - rust-lang#104016 (Add internal descriptions to a few queries)
 - rust-lang#104035 (Add 'closure match' test to weird-exprs.rs.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Nov 6, 2022

⌛ Testing commit 28d82dd with merge 88935e0...

@bors bors merged commit 58f5d57 into rust-lang:master Nov 6, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 6, 2022
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-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.

Detect attempt to use '...' for struct update
9 participants