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

Warn about more ignored bounds in type aliases #48020

Merged
merged 3 commits into from
Feb 7, 2018

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 5, 2018

It seems that all bounds in type aliases are entirely ignored, not just type bounds. This extends the warning appropriately.

I assume this should be made a hard error with the next epoch? I can't see any reason to accept these programs. (And suddenly enforcing these type bounds would be a breaking change.)

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 5, 2018
@@ -0,0 +1,18 @@
warning[E0122]: bounds are ignored in type aliases
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe "generic bounds" instead just "bounds"?
Just "bounds" looks like something is missing to me. Not sure.

@petrochenkov
Copy link
Contributor

r=me after maybe fixing the nit
@bors delegate+

@bors
Copy link
Contributor

bors commented Feb 6, 2018

✌️ @RalfJung can now approve this pull request

@petrochenkov petrochenkov 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 Feb 6, 2018
@RalfJung
Copy link
Member Author

RalfJung commented Feb 6, 2018

Oh fancy, I had no idea bors supports delegation like this. oO

I didn't want to write "type and lifetime bounds", hence my choice of words. I like "generic bounds", thanks for the suggestion!

@RalfJung
Copy link
Member Author

RalfJung commented Feb 6, 2018

@bors r=@petrochenkov

Let's see if this works...

@bors
Copy link
Contributor

bors commented Feb 6, 2018

📌 Commit ac183f8 has been approved by @petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 6, 2018
@RalfJung
Copy link
Member Author

RalfJung commented Feb 6, 2018

Oh, seems like you don't usually put at @ after r=... is that a problem?

@petrochenkov
Copy link
Contributor

@RalfJung
IIUC, the right side of @bors r= is for humans only and can contain anything if the comment author has r+ rights.

@bors
Copy link
Contributor

bors commented Feb 6, 2018

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Feb 6, 2018

📌 Commit ac183f8 has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented Feb 6, 2018

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Feb 6, 2018

📌 Commit ac183f8 has been approved by ```

@RalfJung
Copy link
Member Author

RalfJung commented Feb 6, 2018

lol indeed. Now poor bors is confused. Also, does it escape things properly? What about r=<!-- foobar -->? ;)

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Feb 6, 2018

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Feb 6, 2018

📌 Commit ac183f8 has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented Feb 6, 2018

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Feb 6, 2018

📌 Commit ac183f8 has been approved by petrochenkov

@bluss
Copy link
Member

bluss commented Feb 6, 2018

Just to be on the safe side, if you propose making it an error, what will happen to code like this, will it continue to be accepted?:

use std::ops::Index;
type Output<I, T> = <I as Index<T>>::Output;

@RalfJung
Copy link
Member Author

RalfJung commented Feb 6, 2018

I think so -- this checks bounds in the binders of type aliases, not touching the code it expands to. I assume you bring this up because it actually works? All I'm advocating for is the compiler tell us when we write something that it doesn't support.

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 7, 2018
…chenkov

Warn about more ignored bounds in type aliases

It seems that all bounds in type aliases are entirely ignored, not just type bounds. This extends the warning appropriately.

I assume this should be made a hard error with the next epoch? I can't see any reason to accept these programs. (And suddenly enforcing these type bounds would be a breaking change.)
@bluss
Copy link
Member

bluss commented Feb 7, 2018

Yes it works, but my intuition would be that the bound is required in the type alias definition there, and the compiler is doing the opposite, enforcing the form without bound.

@RalfJung
Copy link
Member Author

RalfJung commented Feb 7, 2018

The current behavior of type aliases is essentially a syntactic expansion. It's more like a macro. Type bounds are checked later. (That's my interpretation anyway, please correct me if that's wrong.)

I do agree that this is somewhat unexpected. However, changing it is backwards-incompatible. So I figured we could at least try to reduce the confusion by telling people when we ignore what they write, e.g. if they do

type SendableVec<T: Send> = Vec<T>

which actually doesn't mean the Vec is sendable.

I suppose what you're saying is that instead of forcing (well, linting) the bounds to be empty, we could lint them to exactly match whatever is required to make sense of the type in the right? I agree that'd be somewhat nicer and more compatible with an eventual future where these bounds are actually checked (at def'n time) and enforced. Unfortunately, I have no idea how to go about implementing that; it seems pretty complicated. Do you think this is a realistic thing to do, and we should rather abort/revert this MR to not have people that want to adhere to the lint change their code twice?

Notice however that for type bounds, we already warn. This just adds the same warning to lifetime bounds.

bors added a commit that referenced this pull request Feb 7, 2018
Rollup of 10 pull requests

- Successful merges: #47613, #47631, #47810, #47883, #47922, #47944, #48014, #48018, #48020, #48028
- Failed merges:
@bors bors merged commit ac183f8 into rust-lang:master Feb 7, 2018
@bluss
Copy link
Member

bluss commented Feb 8, 2018

In the long run I think the bound requirement is correct, but this has already turned quite the other way (with regular bounds), so it doesn't matter to stop just this PR.

@RalfJung RalfJung deleted the type-alias-bounds branch February 18, 2018 18:41
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.

6 participants