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

Warning period for detecting nested impl trait #58608

Merged

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 20, 2019

Here is some proposed code for making a warning period for the new checking of nested impl trait.

It undoes some of the corrective effects of PR #57730, by using boolean flags to track parts of the analysis that were previously skipped prior to PRs #57730 and #57981 landing.

Cc #57979

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2019
@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Feb 20, 2019
@pnkfelix
Copy link
Member Author

nominating for discussion at T-compiler meeting, in terms of evaluating the cost/benefit of doing a warning cycle in this case.

@Centril
Copy link
Contributor

Centril commented Feb 21, 2019

I think the benefit of a warning cycle is low (don't think this is too common) but I think so is the cost since we don't want to use this syntax for anything right now. That said, I'm wary of the number of C-future-compatibility lints that we have right now and that some of them have existed for so long.

To make sure these don't spiral out of control and to bring them down to a more manageable number, I'd like us to set stricter timelines and deadlines for turning these warnings into deny-lints and then to hard errors. The timelines can be set on a case-by-case basis when we make a future-compat lint. E.g. if we merge this before 1.34 goes into beta, in this instance I think it should become #[deny(..)] by 1.35 (+? 1 release) and a hard error by 1.36 (+? 1 release).

@pnkfelix
Copy link
Member Author

@Centril note that the PR as written does not make use of the lint infrastructure.

I suppose I should interpret your comment as pushing for the PR to be changed to be a proper lint, yes?

@Centril
Copy link
Contributor

Centril commented Feb 21, 2019

@pnkfelix I assumed this was just a quick hack and that you'd convert it to the infrastructure we have for C-future-compatibility warnings (and with a dedicated issue). You should interpret my comment as "we should schedule times for when we progress C-future-compatibility to deny to errors" starting with this one.

@pnkfelix
Copy link
Member Author

Okay. I'll wait until we discuss this at T-compiler meeting; if we collectively decide that we will do a warning-cycle, then I'll implement a proper lint. If we decide that we won't, then I'll just close this PR then.

@Zoxc
Copy link
Contributor

Zoxc commented Feb 21, 2019

This is changes existing errors into warnings too though, instead of just the edge case which was allowed.

@nikomatsakis
Copy link
Contributor

My feeling:

  • Landing a lint is probably the right thing to do
  • But if it is just a single (or a few) affected crates, and those crates have moved on, I'm not sure it's worth it
  • I agree that we should do a better job reviewing future compatibility warnings and generally establishing fixed timelines for them etc, I think that's a good topic to review as part of the Meta WG

@nikomatsakis
Copy link
Contributor

Oh, if we were going to do it, I would prefer to do it as a "real future compat lint"

@pnkfelix
Copy link
Member Author

@Zoxc wrote:

This is changes existing errors into warnings too though, instead of just the edge case which was allowed.

Hmm, my intention was to only catch the cases where we went injected fresh errors. I may have messed that up (as in, I may have misread the original code prior to PR #57730 and thought it was missing more than it was in its recursive traversal).

I'll try to double-check the situation there. I certainly don't want to downgrade more errors to warnings than necessary.

@@ -36,6 +36,11 @@ struct AstValidator<'a> {
// Used to ban `impl Trait` in path projections like `<impl Iterator>::Item`
// or `Foo::Bar<impl Trait>`
is_impl_trait_banned: bool,

// rust-lang/rust#57979: the ban of nested `impl Trait` was incomplete
// until sometime after PR #57730 landed. We track whether we should
Copy link
Contributor

Choose a reason for hiding this comment

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

I think should say bugged until PR #57730 landed.

@Zoxc
Copy link
Contributor

Zoxc commented Feb 21, 2019

I'd suggest bringing back NestedImplTraitVisitor from before #57730 and making a list of nodes which should have errors instead of warnings from that. Then pass that list on to AstValidator.

@pnkfelix
Copy link
Member Author

I'd suggest bringing back NestedImplTraitVisitor from before #57730 and making a list of nodes which should have errors instead of warnings from that. Then pass that list on to AstValidator.

yes okay I think I see where I went wrong now. I was doing this too late at night.

@petrochenkov
Copy link
Contributor

r? @Zoxc

@rust-highfive rust-highfive assigned Zoxc and unassigned petrochenkov Feb 22, 2019
@pnkfelix pnkfelix force-pushed the warning-period-for-detecting-nested-impl-trait branch from c60c3fd to aa32240 Compare February 22, 2019 14:49
@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 22, 2019

I wrote a long comment on one of the commits, but this is the important bit:

...
this version is an "improvement" on the one I posted earlier, so I am going to share it for the following reasons:

  1. Maybe someone will point out where I have gone wrong, either in my analysis above or in the code itself

  2. This version is clearly more complex than the (bad) one I had proposed earlier. And it may get even more complex before its "right." This all influences the debate on whether we attempt to land a warning cycle here at all.


Oh, also: it still doesn't use the lint infrastructure. I'll work on that next. But I'm nervous about investing much more effort in this; I was already on the fence about whether code for a warning cycle was worthwhile here, and that was before I put in the more complicated version encoded in the current PR.

@pnkfelix
Copy link
Member Author

also: It is of course a red flag that I have added two different pieces of boolean state for tracking warning cycle state, but only one diagnostic has changed in our test output.

My current hypothesis is that this is a reflection of weakness in our test suite, rather than a redundancy in the approach. But it would be good to actually come up with a concrete test ilustrating the point.

@pnkfelix
Copy link
Member Author

My current hypothesis is that this is a reflection of weakness in our test suite, rather than a redundancy in the approach. But it would be good to actually come up with a concrete test illustrating the point.

Here is a concrete example illustrating the case I am trying to catch with the second boolean flag:

pub trait Bar { }

pub trait Quux<T> { type Assoc; }
pub fn demo(_: impl Quux<(), Assoc=<() as Quux<impl Bar>>::Assoc>) { }

impl<T> Quux<T> for () { type Assoc = u32; }

fn main() { }

@pnkfelix pnkfelix force-pushed the warning-period-for-detecting-nested-impl-trait branch 3 times, most recently from e4a7bd0 to 9653193 Compare February 26, 2019 15:37
@pnkfelix
Copy link
Member Author

Okay, I think I'm done working on this.

However I believe I have also missed the cut-off for beta-next (see #58747).

  • (Clearly I should have devoted more time to this yesterday. Or dropped it entirely...)

Anyway, assuming that we are not planning to recut beta, there isn't much point in landing this unless we also plan to evenutally beta-backport it.

So, nominating for discussion (again) at T-compiler meeting.

@pnkfelix pnkfelix force-pushed the warning-period-for-detecting-nested-impl-trait branch from 4549f0c to d6cee67 Compare March 8, 2019 15:37
@pnkfelix pnkfelix changed the title [WIP] Warning period for detecting nested impl trait Warning period for detecting nested impl trait Mar 8, 2019
@Zoxc
Copy link
Contributor

Zoxc commented Mar 10, 2019

This looks good to me. One thing to note is that the warning_period_57979_nested_impl_trait field is sticky, which means that things that are errors on stable could become warning here, before being turned back into errors again in some future release.

@Centril
Copy link
Contributor

Centril commented Mar 10, 2019

(We should not turn errors on stable into warnings...)

@pnkfelix
Copy link
Member Author

pnkfelix commented Mar 11, 2019

I did try (in an earlier draft of this branch) to avoid making the booleans sticky; in particular, I had tried to reset them back to false at what I thought was the point in the control-flow that corresponded to where the old code would have actually re-initiated the recursive checking.

However, that attempt at a reset back to false didn't seem to be in the correct spot, so I decided that I must have misunderstood the control flow.

(also, my attempts to examples illustrating the stickiness of the current code were unsuccessful, which is another reason why I stopped investigating the matter.)

I'll look again and try to make the fields not-sticky.

@Zoxc
Copy link
Contributor

Zoxc commented Mar 11, 2019

@pnkfelix I think you'll need to bring back NestedImplTraitVisitor in order to do that. warning_period_57979_impl_trait_in_proj should be sticky, so that's fine.

@pnkfelix
Copy link
Member Author

pnkfelix commented Mar 11, 2019

I just want to note, we have a set of potentially conflicting factors here:

(We should not turn errors on stable into warnings...)

Similarly, we should not inject hard-errors into stable without a warning cycle (which is what motivated this PR in the first place).

And we should not overly complicate our own code base (which is why I've been trying to avoid solutions like bringing back NestedImplTraitVisitor itself whole hog). Note that I'm very much on the fence about how much these boolean flags as written are complicating the code base as it stands.


I don't know how to weigh the relative risk of the two options I see here; that is, the risk of:

  1. injecting hard errors into stable versus
  2. turning stable-errors into warnings.

Neither of the two options is great.

But also, neither option is ... that bad (?), given that we don't expect the given coding pattern to be very common in the first place. Plus I think the outcome that result from a downgrade from stable-error-to-warning are not unsoundness, but rather either fragile-dependence on type-inference details, or ICE'ing?


I haven't come to any concrete conclusions yet. I'm going to spend a little bit of time investigating (again) the option of making warning_period_57979_nested_impl_trait non-sticky. If I again fail to come up with something satisfactory, then I'll have to decide whether to move ahead with this PR or to close it.

Update: well, after further review I at least now believe I understand why my prior attempts to make the flag non-sticky failed (I had misunderstood how subtle the bug actually was, in terms of what the original use of walk_ty instead of visit_ty had actually missed in the overall logic).

@pnkfelix pnkfelix changed the title Warning period for detecting nested impl trait [WIP] Warning period for detecting nested impl trait Mar 11, 2019
Instead of a sticky-boolean flag that would downgrade errors to
warnings during further recursion into the type (which is overly broad
because we were not missing errors at arbitrarily deep levels), this
instead tracks state closer to what the original bug actually was.

In particular, the actual original bug was that we were failing to
record the existence of an outer `impl Trait` solely when it occurred
as an *immediate child* during the walk of the child types in
`visit_generic_args`.

Therefore, the correct way to precisely model when that bug would
manifest itself (and thus downgrade the error-to-warning accordingly)
is to track when those outer `impl Trait` cases were previously
unrecorded.

That's what this code does, by storing a flag with the recorded outer
`impl Trait` indicating at which point in the compiler's control flow
it had been stored.

I will note that this commit passes the current test suite. A
follow-up commit will also include tests illustrating the cases that
this commit gets right (and were handled incorrectly by the previous
sticky boolean).
@pnkfelix pnkfelix changed the title [WIP] Warning period for detecting nested impl trait Warning period for detecting nested impl trait Mar 11, 2019
@pnkfelix
Copy link
Member Author

@Zoxc you mind taking a look at this revised (non-sticky) approach for the nested impl Trait check?

@Zoxc
Copy link
Contributor

Zoxc commented Mar 12, 2019

This looks good to me now, though there's still the incorrect comment I noted earlier.

@pnkfelix
Copy link
Member Author

This looks good to me now, though there's still the incorrect comment I noted earlier.

Discussed in zulip; I will address.

@pnkfelix
Copy link
Member Author

@bors r=zoxc

@bors
Copy link
Contributor

bors commented Mar 12, 2019

📌 Commit 0a03ca7 has been approved by zoxc

@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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2019
@bors
Copy link
Contributor

bors commented Mar 12, 2019

⌛ Testing commit 0a03ca7 with merge 8f4c226...

bors added a commit that referenced this pull request Mar 12, 2019
…mpl-trait, r=zoxc

Warning period for detecting nested impl trait

Here is some proposed code for making a warning period for the new checking of nested impl trait.

It undoes some of the corrective effects of PR #57730, by using boolean flags to track parts of the analysis that were previously skipped prior to PRs #57730 and #57981 landing.

Cc #57979
@bors
Copy link
Contributor

bors commented Mar 12, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: zoxc
Pushing 8f4c226 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 12, 2019
@bors bors merged commit 0a03ca7 into rust-lang:master Mar 12, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #58608!

Tested on commit 8f4c226.
Direct link to PR: #58608

🎉 rls on windows: test-fail → test-pass (cc @nrc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Mar 12, 2019
Tested on commit rust-lang/rust@8f4c226.
Direct link to PR: <rust-lang/rust#58608>

🎉 rls on windows: test-fail → test-pass (cc @nrc @Xanewok, @rust-lang/infra).
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 16, 2019
bors added a commit that referenced this pull request Mar 16, 2019
[beta] Rollup backports

Cherry-picked:

* Include bounds from promoted constants in NLL #57202
* Warning period for detecting nested impl trait #58608
* Don't promote function calls to nonpromotable things #58784
* Make migrate mode work at item level granularity #58788
* Expand where negative supertrait specific error is shown #58861
* Expand where negative supertrait specific error is shown #58861

Rolled up:

* [BETA] Update cargo #59217

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants