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

Warnings for issue #32330 #33137

Merged

Conversation

nikomatsakis
Copy link
Contributor

This is an extension of the previous PR that issues warnings in more situations than before. It does not handle all cases of #32330 but I believe it issues warnings for all cases I've seen in practice.

Before merging I'd like to address:

  • open a good issue explaining the problem and how to fix it (I have a draft writeup)
  • work on the error message, which I think is not as clear as it could/should be (suggestions welcome)

r? @aturon

@bors
Copy link
Contributor

bors commented Apr 24, 2016

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

@nikomatsakis nikomatsakis force-pushed the issue-32330-lbr-in-return-type-warning-2 branch from a254c26 to 4fe2845 Compare April 25, 2016 19:57
@@ -626,3 +635,39 @@ impl<'tcx> TypeVisitor<'tcx> for HasTypeFlagsVisitor {
false
}
}

/// Collects all the late-bound regions it finds into a hash set.
Copy link
Member

Choose a reason for hiding this comment

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

This comment might want to live on the public function instead.

@bors
Copy link
Contributor

bors commented May 3, 2016

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

@nikomatsakis nikomatsakis force-pushed the issue-32330-lbr-in-return-type-warning-2 branch from 4fe2845 to 8380fe9 Compare May 17, 2016 00:39
@nikomatsakis
Copy link
Contributor Author

OK, I've rebased this PR (which was...fun). I decided to remove (temporarily, at least) the corrections to subtyping, and focus just on adding warnings around #32330. @aturon had given r+ on the previous version.

@nikomatsakis
Copy link
Contributor Author

I remembered I want to update the issue in the lint to point at a tracking issue. I'll do a crater run tonight and fix it up in the morning.

@nikomatsakis nikomatsakis force-pushed the issue-32330-lbr-in-return-type-warning-2 branch 2 times, most recently from 0a98537 to cfa3471 Compare May 17, 2016 14:24
@nikomatsakis
Copy link
Contributor Author

Hmm. Did a crater run without the improvements to subtyping and was started by the results. My memory is that (with the lint set to deny) we got 4 root regressions before, but now I see 73:

https://gist.github.com/nikomatsakis/2b324ddc0d68c90ccbafb8f71e812344

@nikomatsakis
Copy link
Contributor Author

I am running a new crater run using the more correct subtyping rules to reconfirm (ti's possible that crates.io changed, or perhaps that those are timeouts actually).

@nikomatsakis
Copy link
Contributor Author

OK I looked a bit more closely. The results are not timeouts (though there was one). They are a mix of expected lint errors but also some unexpected type errors -- most of them are in fact in the nom crate, despite crater reporting them as a "root regression". I will investigate a bit.

We ought not to be affecting inference state when assembling candidates,
so invoke select inside of a probe.
@nikomatsakis nikomatsakis force-pushed the issue-32330-lbr-in-return-type-warning-2 branch 2 times, most recently from 8debd69 to 90ff7dd Compare May 17, 2016 20:32
@nikomatsakis
Copy link
Contributor Author

In an effort to just land something dang it, I've pared this branch way back. It is now just warning at the declaration site for types and where-clauses that are clearly invalid. Doing a make check to make sure I didn't mess something up, but this is a subset of the previous code.

@nikomatsakis
Copy link
Contributor Author

Result from the crater run with the changes to subtyping: https://gist.github.com/nikomatsakis/4edf5ef8f7bf65bbb2029c05b46f7818

10 root regressions.

More than before; so it's definitely important to start landing something.

@nikomatsakis
Copy link
Contributor Author

But a good number of those are these errors that I don't fully understand, so I'd rather not land said changes now...

This is a step towards fixing rust-lang#32330. The full fix would be a breaking
change, so we begin by issuing warnings for scenarios that will break.
@nikomatsakis nikomatsakis force-pushed the issue-32330-lbr-in-return-type-warning-2 branch from 90ff7dd to 639890d Compare May 18, 2016 00:26
@nikomatsakis
Copy link
Contributor Author

@bors r=aturon

@bors
Copy link
Contributor

bors commented May 18, 2016

📌 Commit 639890d has been approved by aturon

@bors
Copy link
Contributor

bors commented May 18, 2016

⌛ Testing commit 639890d with merge 75e23e1...

bors added a commit that referenced this pull request May 18, 2016
…rning-2, r=aturon

Warnings for issue #32330

This is an extension of the previous PR that issues warnings in more situations than before. It does not handle *all* cases of #32330 but I believe it issues warnings for all cases I've seen in practice.

Before merging I'd like to address:

- open a good issue explaining the problem and how to fix it (I have a [draft writeup][])
- work on the error message, which I think is not as clear as it could/should be (suggestions welcome)

r? @aturon

[draft writeup]: https://gist.github.com/nikomatsakis/631ec8b4af9a18b5d062d9d9b7d3d967
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants