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 tuple struct syntax #61782

Merged
merged 5 commits into from
Jun 20, 2019

Conversation

Electron-libre
Copy link
Contributor

refs #57242

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Jun 12, 2019
@petrochenkov
Copy link
Contributor

LGTM.
r? @estebank on whether /* fields */ is good enough or should be replaced with an actual fields list.

@Electron-libre
Copy link
Contributor Author

IMHO providing the constructor signature would be a better hint.
Eg:
use function constructor MyFoo(u32, String)

Since only type and order matter for "tuple" syntax.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@estebank

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@estebank
Copy link
Contributor

src/librustc_typeck/check/expr.rs:1223: line longer than 100 chars

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Ideally we would try to use a structured suggestion that gives out the correct code (which is why @petrochenkov was asking for my input), but I think this is a good first pass at a solution which can be improved upon down the line.

Left some nitpicks inline that shouldn't be too hard to address.

src/librustc_typeck/check/expr.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/expr.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/expr.rs Outdated Show resolved Hide resolved
src/test/ui/numeric/numeric-fields.stderr Outdated Show resolved Hide resolved
@estebank
Copy link
Contributor

IMHO providing the constructor signature would be a better hint.

In order to do so you would need to modify this code to not emit one error per field, but rather per invocation, and only once you figure out you have the correct amount of fields (and the correct types for them!) you can emit one error with the structured suggestion. I don't want perfect be enemy of the good enough that will make things materially better :)

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

Copy link
Contributor

@estebank estebank 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 once the nitpicks have been addressed.

src/librustc_typeck/check/expr.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/expr.rs Outdated Show resolved Hide resolved
@Electron-libre
Copy link
Contributor Author

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jun 19, 2019

@Electron-libre: 🔑 Insufficient privileges: Not in reviewers

@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 19, 2019

📌 Commit b72b1ac has been approved by estebank

@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 Jun 19, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 20, 2019
…_syntax, r=estebank

suggest tuple struct syntax

refs rust-lang#57242
bors added a commit that referenced this pull request Jun 20, 2019
Rollup of 4 pull requests

Successful merges:

 - #60454 (Add custom nth_back to Skip)
 - #60772 (Implement nth_back for slice::{Iter, IterMut})
 - #61782 (suggest tuple struct syntax)
 - #61968 (rustc: disallow cloning HIR nodes.)

Failed merges:

r? @ghost
@bors bors merged commit b72b1ac into rust-lang:master Jun 20, 2019
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants