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

Add wrong_any_coerce lint #3376

Closed
wants to merge 3 commits into from

Conversation

HMPerson1
Copy link
Contributor

This lints any coercion to dyn Any where the source type derefs to dyn Any (e.g. Box<dyn Any>, Rc<dyn Any>, etc.). We might also want to lint RefCell<dyn Any> or Mutex<dyn Any>, but I don't know of a good way to check for things like that without just hardcoding a list of "container-ish things".

Fixes #2988

src_ty: Ty<'tcx>,
tgt_ty: Ty<'tcx>,
) -> Option<LintData<'tcx>> {
// redo the typechecking for this coercion to see if it required unsizing something to `dyn Any`
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this? Would it be possible to use https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/struct.TyCtxt.html#method.struct_lockstep_tails to figure out whether the src_ty already is Any at the same level that tgt_ty is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

struct_lockstep_tails doesn't go through references and Box<T> ends up as PhantomData<T>, so we'd have to do some additional work to use that. That also wouldn't work if the type is generic (i.e. T: Deref<Target=dyn Any>).

Copy link
Contributor

Choose a reason for hiding this comment

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

ugh, makes sense :/

Do you think we can share this function with rustc by refactoring the original rustc code to factor out this function and make it public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like resolve_obligations_for_unsize_coercion(.., obligation_callback: impl FnMut(&PredicateObligation) -> ()) -> ..? Then typechecking could use the callback to check for unsizing to a tuple (which apparently is feature gated) and we could check for unsizing to dyn Any?

@phansch phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 6, 2018
@flip1995
Copy link
Member

Ping @HMPerson1. I'm going over old PRs, that were abandoned by us reviewers (sorry for that!) or by the authors. Are you still interested in completing this?

return true;
}
}
// TODO: check for `RefCell<dyn Any>`?
Copy link
Member

@flip1995 flip1995 Jul 2, 2019

Choose a reason for hiding this comment

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

Do you want to do this in this PR? Otherwise you can put this in the Known Problems section.

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 3, 2019
@bors
Copy link
Contributor

bors commented Jul 17, 2019

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

@flip1995 flip1995 added the A-lint Area: New lints label Nov 25, 2019
@phansch
Copy link
Member

phansch commented Apr 15, 2020

Hi @HMPerson1, since this PR has been inactive for a while, I'm going to go ahead and close it. Please don't hesitate to re-open it/ask for help. (As far as I can tell, the last status was that there were some remaining todo comments in the code)

@phansch phansch closed this Apr 15, 2020
@phansch phansch added the S-inactive-closed Status: Closed due to inactivity label Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints S-inactive-closed Status: Closed due to inactivity S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Any pitfalls
5 participants