Skip to content

/rust

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

Make all coercion valid casts and add trivial cast lints #23630

Merged
merged 6 commits into from Mar 25, 2015

Conversation

Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@nrc @eddyb @nikomatsakis @bors @alexcrichton @lilyball @bluss @petrochenkov
@nrc
Copy link
Member

nrc commented Mar 23, 2015

See notes on the first commit

Closes #18601

r? @nikomatsakis

cc @eddyb

@rust-highfive rust-highfive assigned nikomatsakis Mar 23, 2015

src/libcollections/lib.rs Outdated
@@ -23,6 +23,8 @@
html_root_url = "http://doc.rust-lang.org/nightly/",
html_playground_url = "http://play.rust-lang.org/")]

#![allow(trivial_cast)]
#![allow(trivial_numeric_cast)]

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Mar 23, 2015

Contributor

The lint names don't follow the conventions. trivial_cast -> trivial_casts, trivial_numeric_cast -> trivial_numeric_casts?
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Mar 23, 2015

This seems to not interfere too much with my own DST branch (where I did a similar change to coercions to have both a and b shallow resolutions in the same place).
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Mar 23, 2015

I'm wondering if we want the lints to default to warn, in the absence of type ascription? I guess it is very rare to need to convert to a trait object, and one can typically use an intermediate variable, and that seems like the only case where one might require a cast today (other than the trivial numeric case, which is already separated). I agree that the lint names seem wrong.
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 23, 2015

Yeah, should be &x in the examples
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 23, 2015

Type ascription doesn't let you do any more, it is just more convenient, given there's an implementation that just needs rebasing and an accepted RFC, I would expect it to land very soon in any case.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Mar 23, 2015

r+

@nrc nrc force-pushed the nrc:coerce-tidy branch from f441fbd to c2fde5a Mar 23, 2015

@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 23, 2015

@bors r=nikomatsakis c2fde5a
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 23, 2015

🙀 c2fde5a is not a valid commit SHA. Please try again with c4f3029.
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 23, 2015

@bors r=nikomatsakis c4f3029
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 23, 2015

@bors: try

bors added a commit that referenced this pull request Mar 23, 2015

@bors
Auto merge of #23630 - nrc:coerce-tidy, r=<try> 
See notes on the first commit

Closes #18601

r? @nikomatsakis 

cc @eddyb
d8f41c5
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 23, 2015

⌛️ Trying commit c4f3029 with merge d8f41c5...
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 23, 2015

💔 Test failed - try-bsd
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 24, 2015

@bors: try

bors added a commit that referenced this pull request Mar 24, 2015

@bors
Auto merge of #23630 - nrc:coerce-tidy, r=<try> 
See notes on the first commit

Closes #18601

r? @nikomatsakis 

cc @eddyb
31738c0
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

⌛️ Trying commit 48287fe with merge 31738c0...
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

💔 Test failed - try-mac

@nrc nrc force-pushed the nrc:coerce-tidy branch from 48287fe to fc7e9b3 Mar 24, 2015

@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 24, 2015

@bors: try
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

⌛️ Trying commit fc7e9b3 with merge 8381980...

bors added a commit that referenced this pull request Mar 24, 2015

@bors
Auto merge of #23630 - nrc:coerce-tidy, r=<try> 
See notes on the first commit

Closes #18601

r? @nikomatsakis 

cc @eddyb
8381980
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

💔 Test failed - try-win-64

@nrc nrc force-pushed the nrc:coerce-tidy branch from fc7e9b3 to 7315e0a Mar 24, 2015

@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 24, 2015

@bors: try
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

⌛️ Trying commit 7315e0a with merge 2782e72...

bors added a commit that referenced this pull request Mar 24, 2015

@bors
Auto merge of #23630 - nrc:coerce-tidy, r=<try> 
See notes on the first commit

Closes #18601

r? @nikomatsakis 

cc @eddyb
2782e72
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

💔 Test failed - try-win-64

@nrc nrc force-pushed the nrc:coerce-tidy branch from 7315e0a to a7b6af9 Mar 24, 2015

@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 24, 2015

@bors: try

bors added a commit that referenced this pull request Mar 24, 2015

@bors
Auto merge of #23630 - nrc:coerce-tidy, r=<try> 
See notes on the first commit

Closes #18601

r? @nikomatsakis 

cc @eddyb
ce76bff
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

⌛️ Trying commit a7b6af9 with merge ce76bff...
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

💔 Test failed - try-mac
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 24, 2015

@bors: try
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 24, 2015

@bors r=nikomatsakis a7b6af9
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 24, 2015

@bors retry
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

⌛️ Trying commit a7b6af9 with merge 3f8058d...

bors added a commit that referenced this pull request Mar 24, 2015

@bors
Auto merge of #23630 - nrc:coerce-tidy, r=<try> 
See notes on the first commit

Closes #18601

r? @nikomatsakis 

cc @eddyb
3f8058d
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

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

nrc added some commits Mar 20, 2015

@nrc
Add trivial cast lints. 
This permits all coercions to be performed in casts, but adds lints to warn in those cases.

Part of this patch moves cast checking to a later stage of type checking. We acquire obligations to check casts as part of type checking where we previously checked them. Once we have type checked a function or module, then we check any cast obligations which have been acquired. That means we have more type information available to check casts (this was crucial to making coercions work properly in place of some casts), but it means that casts cannot feed input into type inference.

[breaking change]

* Adds two new lints for trivial casts and trivial numeric casts, these are warn by default, but can cause errors if you build with warnings as errors. Previously, trivial numeric casts and casts to trait objects were allowed.
* The unused casts lint has gone.
* Interactions between casting and type inference have changed in subtle ways. Two ways this might manifest are:
- You may need to 'direct' casts more with extra type information, for example, in some cases where `foo as _ as T` succeeded, you may now need to specify the type for `_`
- Casts do not influence inference of integer types. E.g., the following used to type check:

```
let x = 42;
let y = &x as *const u32;
```

Because the cast would inform inference that `x` must have type `u32`. This no longer applies and the compiler will fallback to `i32` for `x` and thus there will be a type error in the cast. The solution is to add more type information:

```
let x: u32 = 42;
let y = &x as *const u32;
```
95602a7
@nrc
Minor refactoring in coercion.rs
9374c21
@nrc
Remove the UnusedCasts lint
088cd56
@nrc
Add tests
dc206a9
@nrc
Change lint names to plurals
e7122a5
@nrc
Bug fixes
Loading status checks…
7e3ee02

@nrc nrc force-pushed the nrc:coerce-tidy branch from a7b6af9 to 7e3ee02 Mar 24, 2015

@nrc

This comment has been minimized.

Sign in to view
Copy link
Member Author

nrc commented Mar 24, 2015

@bors: r=nikomatsakis 7e3ee02

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 24, 2015

@alexcrichton
rollup merge of rust-lang#23630: nrc/coerce-tidy 
See notes on the first commit

Closes rust-lang#18601

r? @nikomatsakis

cc @eddyb
a1d2e62

@bors bors merged commit 7e3ee02 into rust-lang:master Mar 25, 2015
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Ms2ger Ms2ger referenced this pull request Mar 25, 2015

Closed

Coercion to an unspecified but easily inferrable raw pointer (`as *const _`) has regressed #23699

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Mar 26, 2015

@nrc how would you feel about making the trivial_numeric_casts lint allow-by-default? Right now if you're using a libc type you don't actually know what the definition is (e.g. libc::time_t) so it's casted into an i64 so we know what it is. This conversion, though, is a trivial cast on some platforms (but not necessarily on all platforms).

@lilyball lilyball referenced this pull request Mar 26, 2015

Closed

trivial_numeric_casts warning incorrect when casting between type aliases #23739

@lilyball

This comment has been minimized.

Sign in to view
Copy link
Contributor

lilyball commented Mar 26, 2015

I just filed #23739 requesting that trivial_numeric_casts ignore casts between types that are type aliases. This was motivated by libc types.
@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor

bluss commented Mar 26, 2015

Ah, breaking change needs to be exactly [breaking-change] (with hyphen) to show up in the live list, so this didn't.

@bluss bluss referenced this pull request Mar 26, 2015

Closed

Trivial cast warning for cast that can't be removed #23742

@eddyb eddyb referenced this pull request Mar 26, 2015

Closed

Implement type ascription. #21836

This was referenced Mar 26, 2015

Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.