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

Perform WF-check on types with no type parameters #69741

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Mar 5, 2020

  • Start performing a WF-check on types without type parameters to verify that they will be able to be constructed.

  • Do not perform a Sized check, as type T = dyn Trait; should be allowed.

  • Do not WF-check types with type parameters because we currently lint against type T<X: Trait> = K<X>; because we do not propagate X: Trait, which means that we currently allow type T<X> = <X as Trait>::Foo;, which would be rejected if we WF-checked it because it would need to be written as type T<X: Trait> = <X as Trait>::Foo; to be correct.

    Instead, we simply don't check it at definition and only do so at use like we do currently. In the future, the alternatives would be to either automatically backpropagate the X: Trait obligation when WF-checking the type or (my preferred solution) properly propagate the explicit X: Trait obligation to the uses, which would likely be a breaking change.

Fixes #60980 by using a more appropriate span for the error.

@rust-highfive
Copy link
Collaborator

rust-highfive commented Mar 5, 2020

r? @varkor

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

@rust-highfive rust-highfive added the S-waiting-on-review label Mar 5, 2020
@estebank
Copy link
Contributor Author

estebank commented Mar 5, 2020

CC @eddyb @Centril @nikomatsakis

error[E0277]: the trait bound `NoClone: std::clone::Clone` is not satisfied
--> $DIR/struct-path-alias-bounds.rs:9:13
--> $DIR/struct-path-alias-bounds.rs:6:10
|
LL | struct S<T: Clone> { a: T }
| ------------------ required by `S`
...
LL | let s = A { a: NoClone };
| ^ the trait `std::clone::Clone` is not implemented for `NoClone`
LL | type A = S<NoClone>;
| ^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `NoClone`
Copy link
Contributor Author

@estebank estebank Mar 5, 2020

Choose a reason for hiding this comment

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

This is the category of errors I wanted to improve.

@Centril Centril added I-nominated T-lang labels Mar 6, 2020
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 9, 2020

I would indeed like to make progress on this but I'm a bit nervous about making this change in isolation, without a broader plan in place. We should do a crater run to get an idea of the impact, this may require future compatibility warnings.

@estebank
Copy link
Contributor Author

estebank commented Mar 9, 2020

@bors try @craterbot check

@bors
Copy link
Contributor

bors commented Mar 9, 2020

Trying commit 6d65bc6 with merge 89c05fd...

bors added a commit that referenced this issue Mar 9, 2020
Perform WF-check on `type`s with no type parameters

- Start performing a WF-check on `type`s to verify that they will be able
to be constructed.
- Do *not* perform a `Sized` check, as
`type T = dyn Trait;` should be allowed.
- Do *not* WF-check `type`s with type parameters because we currently lint *against* `type T<X: Trait> = K<X>;` because we do not propagate `X: Trait`, which means that we currently allow `type T<X> = <X as Trait>::Foo;`, which would be rejected if we WF-checked it because it would need to be written as `type T<X: Trait> = <X as Trait>::Foo;` to be correct.

   Instead, we simply don't check it at definition and only do so at use like we do currently. In the future, the alternatives would be to either automatically backpropagate the `X: Trait` obligation when WF-checking the `type` or (my preferred solution) properly propagate the explicit `X: Trait` obligation to the uses, which would likely be a breaking change.

Fixes #60980 by using a more appropriate span for the error.
@bors
Copy link
Contributor

bors commented Mar 10, 2020

☀️ Try build successful - checks-azure
Build commit: 89c05fd (89c05fd3cc6404a13e1bd71450c6132a2755fa4b)

@estebank
Copy link
Contributor Author

estebank commented Mar 10, 2020

@craterbot check

@craterbot
Copy link
Collaborator

craterbot commented Mar 10, 2020

👌 Experiment pr-69741 created and queued.
🤖 Automatically detected try build 89c05fd
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater and removed S-waiting-on-review labels Mar 10, 2020
@craterbot
Copy link
Collaborator

craterbot commented Mar 10, 2020

🚧 Experiment pr-69741 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@eddyb
Copy link
Member

eddyb commented Mar 12, 2020

Someone (maybe me later if someone reminds me in PM) should find the late 2018 PRs/issues relevant to this, since it's quite possible we did this exact crater run then.

@estebank
Copy link
Contributor Author

estebank commented Mar 12, 2020

@eddyb I'll dig into it, but in the meantime I thought it was a good idea to test again because even if the semantic consequences of this change haven't changed since then, the ecosystem's reliance on the current behavior might have.

@craterbot
Copy link
Collaborator

craterbot commented Mar 13, 2020

🎉 Experiment pr-69741 is completed!
📊 41 regressed and 1 fixed (95178 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review and removed S-waiting-on-crater labels Mar 13, 2020
@estebank
Copy link
Contributor Author

estebank commented Mar 14, 2020

bitvec and domain seem to be the crates most affected by this change.

@estebank
Copy link
Contributor Author

estebank commented Mar 23, 2020

I reached out to the bitvec author and this was a deprecation placeholder that users are not supposed to be using and will be removed in the next version. It was also likely used by accident, which lends credence to my gut feeling that it's imperative to start fixing this sooner rather that later.

@varkor
Copy link
Member

varkor commented Mar 26, 2020

Are we waiting on the lang team to discuss this again, or is this ready for review?

@Centril
Copy link
Contributor

Centril commented Mar 26, 2020

Probably the former :)

@Centril Centril added S-waiting-on-team and removed S-waiting-on-review labels Mar 26, 2020
@crlf0710 crlf0710 removed the S-waiting-on-team label Apr 24, 2020
@RalfJung
Copy link
Member

RalfJung commented Mar 2, 2021

This PR seems to make a somewhat surprising distinction between type aliases with and without parameters? If I read the comments above correctly, the former require WF while the latter do not, and the plan is to keep it like that. Isn't that strangely inconsistent, or am I misunderstanding something?

I think we felt pretty comfortable with landing this PR if it simply issues warnings -- not necessarily future-compatibility warnings, just standard lints.

As demonstrated by #82700, doing this requires relying on const_err to report just a lint, not a hard error. So if the lang team was planning for these warnings to be permanent (and not just part of a transition plan to an eventual hard error), then this lang team decision is in direct contradiction with #71800, where the plan is to make const-eval failures hard errors, never lints.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 2, 2021

From my reading of the conversation, the lang team was not aware of the issue with constants at the time of the meeting. I think wrt broken-and-locally-unused constants, we already have consensus and I think wg-const-eval has enough leeway to extend that to constants in type aliases.

Otherwise we can change the param env of the wf check to Reveal::Selection which will report no const eval errors at all, so we have a way back out that doesn't even require #82700

@lcnr
Copy link
Contributor

lcnr commented Mar 2, 2021

This PR seems to make a somewhat surprising distinction between type aliases with and without parameters? If I read the comments above correctly, the former require WF while the latter do not, and the plan is to keep it like that. Isn't that strangely inconsistent, or am I misunderstanding something?

This distinction is already the case for type param defaults, so the following doesn't error even though the type default isn't wf.

struct Foo<T, U = (Vec<[u32]>, T)>(T, U);

Have we tried always erroring on eval errors of constants in type aliases? I don't think there is much, if any, code using incorrect constants there. So i personally would be interested in trying that instead of modifying const eval.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 2, 2021

Have we tried always erroring on eval errors of constants in type aliases?

This is what is done right now, and there were no regressions on crater that had const eval errors (I just checked again).

@crlf0710 crlf0710 added S-waiting-on-team and removed S-waiting-on-team labels Mar 20, 2021
//pub type Foo = [i32; 0 - 1];
//^ evaluation of constant value failed
Copy link
Contributor

@oli-obk oli-obk Mar 29, 2021

Choose a reason for hiding this comment

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

Please turn these into error tests (see https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/erroneous.20constants.20in.20type.20alias/near/229492342 for "lang team sign off on making this a hard error since crater is clean")

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 1, 2021

To be clear, I thought the @rust-lang/lang consensus was that we would be ok with issuing warnings, but not errors.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 1, 2021

To be clear, I thought the @rust-lang/lang consensus was that we would be ok with issuing warnings, but not errors.

Yes, but constants were not discussed. So I brought it up on zulip, and my message got one textual approval and two thumbs-up reactions.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 1, 2021

Perhaps I am missing something.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 1, 2021

I created a workaround that does not hard error in #82700 but it made no one in @rust-lang/wg-const-eval happy (including myself). The main difference between hard erroring on constants and hard erroring on other wf-bounds checks failing is that the latter has crater regressions while the former does not. Other than that, the reasoning about making it hard errors or not is the same.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 2, 2021

Hmm. My problem with hard errors on other wf-bounds checks is not that they have crater regressions, but that it's inconsistent to do so only in narrow cases. I'm not sure what would make constants different from anything else: I guess it's just that we get ICEs sometimes with constants? This seems related to the questions we were discussing in our recent meeting.

@crlf0710 crlf0710 added S-waiting-on-team and removed S-waiting-on-team labels Apr 23, 2021
@bors
Copy link
Contributor

bors commented Apr 29, 2021

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

@oli-obk
Copy link
Contributor

oli-obk commented May 3, 2021

I'm not sure what would make constants different from anything else: I guess it's just that we get ICEs sometimes with constants?

No, the problem is that error reporting on constants happens inside the query that evaluates the constant. We would need to walk up the DefId parents of a constant to see whether it is part of a type alias and then change the behaviour of the error reporting. This is possible, but it complicates our ctfe error reporting machinery further.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 3, 2021

I see, that's interesting. I think my expectation is that we would simply not evaluate the default value in any way until it is used. I guess this is not what we do?

@oli-obk
Copy link
Contributor

oli-obk commented May 3, 2021

Wf-check evaluated all constants, because "an array type whose length computation errors is not well formed". At least that was how I understood wfcheck in other places. We could generally stop const evaluating during wfcheck, even in other situations and leave it to the usages, but I'm not sure what the ramnifications of that are

@crlf0710 crlf0710 added S-waiting-on-team and removed S-waiting-on-team labels May 22, 2021
@crlf0710 crlf0710 added S-waiting-on-team and removed S-waiting-on-team labels Jun 11, 2021
@crlf0710 crlf0710 added S-waiting-on-team and removed S-waiting-on-team labels Jul 4, 2021
@crlf0710 crlf0710 added S-waiting-on-team and removed S-waiting-on-team labels Jul 24, 2021
@crlf0710 crlf0710 added S-waiting-on-team and removed S-waiting-on-team labels Sep 10, 2021
@Dylan-DPC
Copy link
Member

Dylan-DPC commented May 31, 2022

@oli-obk @nikomatsakis is there any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team T-lang
Projects
None yet
Development

Successfully merging this pull request may close these issues.