Skip to content

Improve Error Messaging for Unconstructed Structs and Enum Variants in Generic Contexts - #92569

Merged
bors merged 2 commits into
rust-lang:masterfrom
George-lewis:87181
Apr 27, 2022
Merged

Improve Error Messaging for Unconstructed Structs and Enum Variants in Generic Contexts#92569
bors merged 2 commits into
rust-lang:masterfrom
George-lewis:87181

Conversation

@George-lewis

@George-lewisGeorge-lewis commented Jan 5, 2022

Copy link
Copy Markdown
Contributor

Improves error messaging for empty-tuple structs and enum variants in certain generic contexts. See new ui tests for examples.

Closes#87181

@rustbotrustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 5, 2022
@rust-highfive

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2022
@George-lewis

Copy link
Copy Markdown
ContributorAuthor

r? @estebank

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Can we assume this won't fail here? Is it ok for it to fail?

Comment threadcompiler/rustc_typeck/src/astconv/mod.rs Outdated
Comment threadcompiler/rustc_typeck/src/check/method/suggest.rs Outdated
@camelid

camelid commented Jan 5, 2022

Copy link
Copy Markdown
Member

Please also add a UI test (src/test/ui/...).

Comment threadcompiler/rustc_typeck/src/check/method/suggest.rs Outdated
@camelidcamelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-diagnostics Area: Messages for errors, warnings, and lints and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2022
@camelidcamelid changed the title Improve Error Messaging for Unconstructed Empty-Tuple MembersImprove Error Messaging for Unconstructed Empty-Tuple FieldsJan 5, 2022
Comment on lines 1 to 2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you need to run ./x.py test --bless src/test/ui/typeck/issue-87181-empty-tuple-fields.rs.

@George-lewisGeorge-lewisJan 5, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think I might be missing something? I ran your command and it generated the stderr file, but the test still fails, and it's showing me some JSON.

Edit: I figured it out, I needed a //~^ ERROR in there

@George-lewis

Copy link
Copy Markdown
ContributorAuthor

@camelid Did I pick an acceptable filename for the ui test? I couldn't find much guidance in the contribution guide

@camelid

Copy link
Copy Markdown
Member

The name seems fine to me.

@rust-log-analyzer

This comment has been minimized.

@George-lewis

George-lewis commented Jan 5, 2022

Copy link
Copy Markdown
ContributorAuthor

Proposal: Go beyond the scope of the ticket to improve error handling to support tuple-structs with elements, and also add the check to member access rather than just to method calls

For instance we can cover this case as so:

structBar<T>{bar:T}structFoo(u8);implFoo{fnfoo(){}}fnmain(){let thing = Bar{bar:Foo};
thing.bar.0;}
error[E0609]: no field `0` on type `fn(u8) -> Foo{Foo}`
--> test.rs:12:15
|
12 | thing.bar.0;
| --------- ^
| |
| help: call the constructor: `(thing.bar)(_)`
|
= help: placeholder
error: aborting due to previous error
For more information about this error, try `rustc --explain E0609`.

Also, if the original code has unnecessary brackets, like (thing.0).0, the current code will suggest a superfluous pair of brackets. Should we worry about that or just let the unnecessary-bracket lint pick it up for us?

@camelidcamelid 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 Jan 5, 2022
@George-lewis

Copy link
Copy Markdown
ContributorAuthor

I have expanded the scope a little in line with my proposal. Now, the same checks are applied to field accesses, and non-empty tuple constructors are handled. There are 2 new tests to cover these cases.

@George-lewisGeorge-lewis changed the title Improve Error Messaging for Unconstructed Empty-Tuple FieldsImprove Error Messaging for Unconstructed Structs and Enum Variants in Generic ContextsJan 5, 2022
Comment threadcompiler/rustc_typeck/src/check/mod.rs Outdated
@JohnCSimonJohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2022
Comment threadcompiler/rustc_typeck/src/check/mod.rs Outdated
Comment threadcompiler/rustc_typeck/src/check/mod.rs Outdated
@rust-log-analyzer

This comment has been minimized.

@bors

bors commented Apr 26, 2022

Copy link
Copy Markdown
Collaborator

💔 Test failed - checks-actions

@borsbors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 26, 2022
@George-lewis

Copy link
Copy Markdown
ContributorAuthor

curl: (6) Could not resolve host: ci-mirrors.rust-lang.org

It seems like there might have been a network error?

@estebank

Copy link
Copy Markdown
Contributor

@bors retry

@borsbors 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 Apr 26, 2022
@bors

bors commented Apr 26, 2022

Copy link
Copy Markdown
Collaborator

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout 87181(switch to your branch)
  2. git fetch upstream master(retrieve the latest master)
  3. git rebase upstream/master -p(rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self 87181 --force-with-lease(update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging compiler/rustc_typeck/src/check/method/suggest.rs
CONFLICT (content): Merge conflict in compiler/rustc_typeck/src/check/method/suggest.rs
Auto-merging compiler/rustc_typeck/src/check/expr.rs
Automatic merge failed; fix conflicts and then commit the result.

@borsbors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 26, 2022
@estebank

Copy link
Copy Markdown
Contributor

It seems like you'll have to rebase. You might want to squash all your commits into one before doing that to make it easier to resolve the merge conflicts.

@George-lewis

Copy link
Copy Markdown
ContributorAuthor

It does seem that way. I'm looking at it now and it looks like some new diagnostics were implemented, so I'll have to figure out what it does so I can integrate it with my diagnostics 🤔

@bors

bors commented Apr 26, 2022

Copy link
Copy Markdown
Collaborator

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

@George-lewis

Copy link
Copy Markdown
ContributorAuthor

Oh God

@George-lewis

George-lewis commented Apr 26, 2022

Copy link
Copy Markdown
ContributorAuthor

I think I messed up the rebase. At this point I just want to make a new branch

Edit: I'm in the process of making a new branch now. I guess I'll open a new PR when it's ready

@George-lewis

Copy link
Copy Markdown
ContributorAuthor

Unfortunately I had to force push, but I think it should be ok now

@estebank

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Apr 26, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit a6b570b has been approved by estebank

@borsbors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 26, 2022
@Dylan-DPCDylan-DPC mentioned this pull request Apr 27, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2022
Rollup of 6 pull requests
Successful merges:
- rust-lang#92569 (Improve Error Messaging for Unconstructed Structs and Enum Variants in Generic Contexts)
- rust-lang#96370 (Cleanup `report_method_error` a bit)
- rust-lang#96383 (Fix erased region escaping into wfcheck due to rust-lang#95395)
- rust-lang#96385 (Recover most `impl Trait` and `dyn Trait` lifetime bound suggestions under NLL)
- rust-lang#96410 (rustdoc: do not write `{{root}}` in `pub use ::foo` docs)
- rust-lang#96430 (Fix handling of `!` in rustdoc search)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors

bors commented Apr 27, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit a6b570b with merge 99b70ee...

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-diagnosticsArea: Messages for errors, warnings, and lintsS-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant 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.

Zero member tuple struct should warn on incorrect construction

11 participants

@George-lewis@rust-highfive@camelid@rust-log-analyzer@estebank@bors@JohnCSimon@apiraino@TomBinford@davidtwco@rustbot