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

Detect when 'static obligation might come from an impl #73783

Merged
merged 8 commits into from
Jul 23, 2020

Conversation

estebank
Copy link
Contributor

Partly address #71341.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(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 Jun 27, 2020
@estebank
Copy link
Contributor Author

r? @Aaron1011

@rust-highfive rust-highfive assigned Aaron1011 and unassigned eddyb Jun 27, 2020
"cannot infer an appropriate lifetime"
} else {
err.span_label(sup_origin.span(), "...is captured here...");
if return_sp < sup_origin.span() {
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice for DiagnosticBuilder to have a concept of 'ordered diagnostics', as there are lots of other places where this kind of logic would be useful. However, it doesn't have to block this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a proposal around this at some point. I wanted to change the rendering if the diagnostics would not display in source order.

@bors

This comment has been minimized.

@estebank estebank force-pushed the impl-dyn-trait-static-lifetime branch 3 times, most recently from 05475b4 to bdb39ab Compare June 28, 2020 23:17
..
}) => Some(*span),
})) if of_trait.trait_def_id() == Some(trait_did) => Some(self_ty),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realized this is wrong, this is meant to check the self_ty instead. It happens to work in the test because it is the only impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using an alternative guessing method, but eventually we would want to do the check between self_ty and the original method receiver ty, but I always find it tricky to correctly compare hir::Tys and ty::Tys.

@Aaron1011 Aaron1011 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2020

// Get the `Ident` of the method being called and the corresponding `impl` (to point at
// `Bar` in `impl Foo for dyn Bar {}` and the definition of the method being called).
let (ident, self_ty) = match tcx.hir().get_if_local(instance.def_id()) {
Copy link
Member

Choose a reason for hiding this comment

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

We almost have enough information to do this for foreign crates - we can get the method ident (through AssocItem), and the overall impl span, but not the span of the self type. I think it's fine to just open an issue about extending this error message, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For foreign crates we might want to just say "the impl in another crate causes the 'static requirement, deal with it or bother their author 🤷‍♂️" 😬

@estebank estebank force-pushed the impl-dyn-trait-static-lifetime branch from 48f9664 to 5136013 Compare July 1, 2020 07:25
@estebank
Copy link
Contributor Author

estebank commented Jul 1, 2020

@Aaron1011 are there any items left to tackle?

@estebank estebank 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 1, 2020
@estebank
Copy link
Contributor Author

estebank commented Jul 1, 2020

(Changing the labels as I presume they were changed due to #73783 (comment). The new heuristic isn't perfect, but should work well enough.)

let parent_id = tcx.hir().get_parent_item(*hir_id);
match tcx.hir().find(parent_id) {
Some(Node::Item(Item { kind: ItemKind::Trait(..), .. })) => {
// The method being called is defined in the `trait`, but the `'static`
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this is handling a case like:

trait OtherTrait {}
trait Foo {
	fn bar(&self) {}
}

impl Foo for dyn OtherTrait {}

where we're using the default implementation from the trait?

I think we can remove the need for this by extending Instance::resolve to provide information about the parent impl item (even when the method implementation) comes from the trait itself
). Could you open an issue to track changing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just reused this method for the Trait::method(binding) case, but I'll look into modifying Instance::resolve regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a quick test modifying Instance::resolve and I caused a bunch of unrelated ICEs. I'll do that in a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with merging this as-is, unless there are other changes that you'd like to make.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm satisfied with the cases I'm handling. I'll revisit this in the future for further improvement but I think can be merged. (Of courses take a last review pass to make sure I didn't introduce anything by accident.)

@Aaron1011 Aaron1011 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2020
@estebank
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 21, 2020

📌 Commit 7b05fb5 has been approved by nikomatsakis

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 21, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 22, 2020
…time, r=nikomatsakis

Detect when `'static` obligation might come from an `impl`

Partly address rust-lang#71341.
@Manishearth
Copy link
Member

@bors r-

#74638 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 22, 2020
@estebank estebank force-pushed the impl-dyn-trait-static-lifetime branch from 7b05fb5 to 889a4d9 Compare July 22, 2020 20:12
@estebank
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 22, 2020

📌 Commit 889a4d9 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 22, 2020

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 22, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 23, 2020
…arth

Rollup of 9 pull requests

Successful merges:

 - rust-lang#73783 (Detect when `'static` obligation might come from an `impl`)
 - rust-lang#73868 (Advertise correct stable version for const control flow)
 - rust-lang#74460 (rustdoc: Always warn when linking from public to private items)
 - rust-lang#74538 (Guard against non-monomorphized type_id intrinsic call)
 - rust-lang#74541 (Add the aarch64-apple-darwin target )
 - rust-lang#74600 (Enable perf try builder)
 - rust-lang#74618 (Do not ICE on assoc type with bad placeholder)
 - rust-lang#74631 (rustc_target: Add a target spec option for disabling `--eh-frame-hdr`)
 - rust-lang#74643 (build: Remove unnecessary `cargo:rerun-if-env-changed` annotations)

Failed merges:

r? @ghost
@bors bors merged commit fe9babb into rust-lang:master Jul 23, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants