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

E0122: clarify wording #43176

Merged
merged 2 commits into from
Aug 11, 2017
Merged

E0122: clarify wording #43176

merged 2 commits into from
Aug 11, 2017

Conversation

RalfJung
Copy link
Member

I assume the reason these constraints are not hard errors is backwards compatibility. If yes, I think the error explanation (at least the long form) should be clearer about that, which is what this PR does.

If not, the explanation should give some other suitable explanation.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

An attempt was made to add a generic constraint to a type alias. While Rust will
allow this with a warning, it will not currently enforce the constraint.
An attempt was made to add a generic constraint to a type alias. This constraint is
entirely ignored. For backwards compatibility, Rust still allows this with a warning.
Copy link
Member

Choose a reason for hiding this comment

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

[00:12:30] error: description for error code E0122 contains a line longer than 80 characters.
[00:12:30] if you're inserting a long URL use the footnote style to bypass this check.
[00:12:30]     --> /checkout/src/librustc_typeck/diagnostics.rs:13:1
[00:12:30]      |
[00:12:30] 13   | / register_long_diagnostics! {
[00:12:30] 14   | |
[00:12:30] 15   | | E0023: r##"
[00:12:30] 16   | | A pattern used to match against an enum variant must provide a sub-pattern for
[00:12:30] ...    |
[00:12:30] 4680 | |
[00:12:30] 4681 | | }
[00:12:30]      | |_^
[00:12:30]      |
[00:12:30]      = note: this error originates in a macro outside of the current crate
[00:12:30] 
[00:12:30] error: aborting due to previous error(s)

Copy link
Member Author

Choose a reason for hiding this comment

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

oops did not know about this -- will fix.

Consider the example below:
An attempt was made to add a generic constraint to a type alias. This constraint
is entirely ignored. For backwards compatibility, Rust still allows this with a
warning. Consider the example below:
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect. The constraint is required to use associated type syntax (without writing <T as Trait>::Assoc - which you can't for Fn traits on stable).

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you elaborate? Are you saying there is a difference between the constraint being "not enforced" and "entirely ignored"?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the constraint is only ignored when referencing the alias, not within it. Actually I think <T as Trait>::Assoc still requires a T: Trait bound. And the only reason that works is we effectively recover a T: Trait bound from the presence of a <T as Trait>::Assoc type, even if we don't know whether the type came from somewhere which already required T: Trait.

Copy link
Member Author

Choose a reason for hiding this comment

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

the constraint is only ignored when referencing the alias, not within it.

Are you sure?

This is accepted:

struct Test<T: Copy> { t : T}

type MyTest<T> = Test<T>;

And so it this:

trait Test {
    type Type;
}

type MyTest<T> = <T as Test>::Type;

To me it looks like constraints are not at all checked or enforced inside aliases.

Copy link
Member

Choose a reason for hiding this comment

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

@nikomatsakis Do we not do any WF-checking 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.

@eddyb at present, I think we do not! I expect we are going to make various changes here -- possibly in a "new epoch", but hopefully we can pull it off without that.

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 13, 2017
@carols10cents
Copy link
Member

What's the status on this PR, @RalfJung @eddyb? Are we waiting for an answer for this comment from @nikomatsakis?

@nikomatsakis
Copy link
Contributor

@eddyb @RalfJung -- funny, I've been meaning to bring this issue up as a back-compat concern. I'm hoping to make type aliases more "first class" -- i.e., roll them into lazy normalization, which I think I see a way towards implementing now -- and of course we're going to have to wrestle with the question of what to do about existing broken cases.

@RalfJung
Copy link
Member Author

The label "waiting on author" seems to be wrong... that would be waiting on me, and then I'm not sure what I am expected to do.^^

@nikomatsakis is the new wording of the message ("this constraint is entirely ignored") correct or not?

@carols10cents
Copy link
Member

ok, sounds like this should be waiting on review then, since @RalfJung is waiting on input from @nikomatsakis !

@carols10cents carols10cents 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 Jul 24, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jul 25, 2017

note: @nikomatsakis is on vacation and is behind on notifications.

@aidanhs aidanhs 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 Aug 3, 2017
@aidanhs
Copy link
Member

aidanhs commented Aug 3, 2017

Ping @nikomatsakis - not sure what your current status is with wading through notifications, just want to keep this on your radar for review!

@arielb1
Copy link
Contributor

arielb1 commented Aug 6, 2017

note: @nikomatsakis is still on vacation and is behind on notifications.

@alexcrichton
Copy link
Member

@eddyb you're the listed reviewer for this, but should that be moved over to @nikomatsakis? It looks like all the last comments are indicating that's the blocker!

@eddyb
Copy link
Member

eddyb commented Aug 10, 2017

Oh well. @bors r+

@bors
Copy link
Contributor

bors commented Aug 10, 2017

📌 Commit 5067ef2 has been approved by eddyb

@aidanhs
Copy link
Member

aidanhs commented Aug 10, 2017

@bors rollup

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 11, 2017
E0122: clarify wording

I *assume* the reason these constraints are not hard errors is backwards compatibility. If yes, I think the error explanation (at least the long form) should be clearer about that, which is what this PR does.

If not, the explanation should give some other suitable explanation.
bors added a commit that referenced this pull request Aug 11, 2017
MaloJaffre added a commit to MaloJaffre/rust that referenced this pull request Aug 11, 2017
E0122: clarify wording

I *assume* the reason these constraints are not hard errors is backwards compatibility. If yes, I think the error explanation (at least the long form) should be clearer about that, which is what this PR does.

If not, the explanation should give some other suitable explanation.
@bors bors merged commit 5067ef2 into rust-lang:master Aug 11, 2017
@RalfJung RalfJung deleted the explain 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-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants