Skip to content

Create more error codes - #42519

Merged
bors merged 7 commits into
rust-lang:masterfrom
GuillaumeGomez:create-more-error-codes
Jun 27, 2017
Merged

Create more error codes#42519
bors merged 7 commits into
rust-lang:masterfrom
GuillaumeGomez:create-more-error-codes

Conversation

@GuillaumeGomez

Copy link
Copy Markdown
Member

Fixes#31174.
Part of #42229.

cc @Susurrus

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @pnkfelix

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

@aidanhs

Copy link
Copy Markdown
Contributor
[00:46:17] error[E0600]: cannot apply unary operator `!` to type `char`
[00:46:17] --> <anon>:3:1
[00:46:17] |
[00:46:17] 3 | assert!(c, 'V');
[00:46:17] | ^^^^^^^^^^^^^^^^
[00:46:17] |
[00:46:17] = note: this error originates in a macro outside of the current crate

Presumably meant to be assert_eq?

@aidanhsaidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 8, 2017
@GuillaumeGomez
GuillaumeGomezforce-pushed the create-more-error-codes branch from 9bedfa8 to 341f827CompareJune 8, 2017 13:17
@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

Absolutely!

@GuillaumeGomez

GuillaumeGomez commented Jun 9, 2017

Copy link
Copy Markdown
MemberAuthor

Fixed btw.

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should read: "Only primitive types can be cast into each other"

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above.

I'm thinking the first line should read more like "Casting between non-primitive types" or something instead of just saying "invalid cast" and then having to explain every time that a valid cast is just between primitives.

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"DST" instead of "They" at the start to be more clear.

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Get rid of "So in order"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any way to remove "non-scalar" from this? Your expanded error descriptions you added above use the term "primitive types", can that be used everywhere for all these errors instead of "non-scalar"? I never see "non-scalar" in any docs anywhere.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hum, do you have a better formulation?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"non-primitive cast"? Primitive is a word people will/should be exposed to when using rust. I don't think "non-scalar" appears much in the docs (though I haven't searched, but I've never seen it).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fine by me!

@GuillaumeGomez
GuillaumeGomezforce-pushed the create-more-error-codes branch from 341f827 to c1791baCompareJune 9, 2017 20:21
@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

Updated.

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This reads awkwardly. I suggest "was attempted on a type other than u8".

This error does bring up the question of why are only u8s castable to chars? I thought a char in rust was 4 bytes?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think it comes straight from the C language. In C, char are 8 bits and supposed to represent ascii characters.

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should be "can be casted"

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe instead be more clear what simple means: "Thin pointers are purely a reference to a memory address"

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This brings up the question of why? What extra information is required to be stored that makes a fat pointer a necessity. Adding a sentence or clarifying why just a pure memory address would be useful here (I'm not even actually sure of why fat pointers are necessary).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

So, the explanation for you too (so nice of me haha 😉 ). Let's take a slice: you need in addition of the memory address, the size of the slice.

That's actually a good explanation, I'll add it. :)

@GuillaumeGomez
GuillaumeGomezforce-pushed the create-more-error-codes branch from c1791ba to 69cdf76CompareJune 9, 2017 20:40
@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

Updated.

@GuillaumeGomez
GuillaumeGomezforce-pushed the create-more-error-codes branch 3 times, most recently from e18afb3 to c70c37aCompareJune 10, 2017 19:23
Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"can be casted"

@GuillaumeGomez
GuillaumeGomezforce-pushed the create-more-error-codes branch from c70c37a to b0016caCompareJune 11, 2017 10:31
@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

Updated.

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are a few instances of "casted" used here when I think the past tense of "cast" is actually just "cast".

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Damn irregular verbs! :p

@GuillaumeGomez
GuillaumeGomezforce-pushed the create-more-error-codes branch from b0016ca to b9804adCompareJune 11, 2017 16:50
@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

Fixed the verbing error.

@bors

bors commented Jun 12, 2017

Copy link
Copy Markdown
Collaborator

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

@GuillaumeGomez
GuillaumeGomezforce-pushed the create-more-error-codes branch from b9804ad to 851fbbcCompareJune 12, 2017 16:58
@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

And re-updated. cc @Susurrus

@Susurrus

Copy link
Copy Markdown
Contributor

LGTM

@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

@bors: r+

@bors

bors commented Jun 12, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 851fbbc has been approved by GuillaumeGomez

@frewsxcv

Copy link
Copy Markdown
Contributor

@bors r-

who is the reviewer?

@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

r? @frewsxcv

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if "dynamically sized types" needs to be capitalized here, but not a big deal

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree, it shouldn't be capitalized.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It was to make easier the understanding of DST.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Ok removed.

Comment threadsrc/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As long as this is the catch-all error message for invalid casting, I'm slightly leaning towards listing all the different casting rules. Maybe we could just link to the book? Other error messages seem to link to the book/reference

@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

So, I added an url to the rust reference to every cast rules in all long error explanation. Do you think I need to update something else?

@QuietMisdreavus

Copy link
Copy Markdown
Contributor

I still think it would be better to rephrase the explain text for E0606 to state something like "incompatible cast" instead of talking about "non-primitive types". Especially now that it's linking to the list of "valid casts", we can just call it "invalid" or "incompatible" or something like that.

@bors

bors commented Jun 16, 2017

Copy link
Copy Markdown
Collaborator

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

@arielb1arielb1 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 Jun 20, 2017
@GuillaumeGomez
GuillaumeGomezforce-pushed the create-more-error-codes branch from 7c12bbb to 9137153CompareJune 24, 2017 19:28
@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

I slightly updated the long error explanation. Does it seem good like this?

@QuietMisdreavus

Copy link
Copy Markdown
Contributor

It looks like the links to the reference got taken out in the last update? I'd rather leave those in.

@GuillaumeGomezGuillaumeGomez mentioned this pull request Jun 24, 2017
@GuillaumeGomez

Copy link
Copy Markdown
MemberAuthor

Updated.

@QuietMisdreavus

Copy link
Copy Markdown
Contributor

Travis looks stuck on one builder, but the ALLOW_PR build passed, so that's good enough for me.

@bors r+

Thanks so much!

@bors

bors commented Jun 26, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit bcf0d60 has been approved by QuietMisdreavus

@frewsxcv

Copy link
Copy Markdown
Contributor

@bors rollup

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 27, 2017
@frewsxcvfrewsxcv mentioned this pull request Jun 27, 2017
bors added a commit that referenced this pull request Jun 27, 2017
Rollup of 5 pull requests
- Successful merges: #42519, #42871, #42874, #42905, #42917
- Failed merges:
@bors
bors merged commit bcf0d60 into rust-lang:masterJun 27, 2017
@GuillaumeGomez
GuillaumeGomez deleted the create-more-error-codes branch June 27, 2017 07:42
bors added a commit that referenced this pull request Jun 29, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@GuillaumeGomez@rust-highfive@aidanhs@bors@Susurrus@frewsxcv@QuietMisdreavus@pnkfelix@arielb1