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

Introduce `matches(T: Trait)` condition to rustc_on_unimplemented #44755

Closed
arielb1 opened this Issue Sep 21, 2017 · 7 comments

Comments

Projects
None yet
6 participants
@arielb1
Contributor

arielb1 commented Sep 21, 2017

We want to allow rustc_on_unimplemented to check arbitrary type conditions to make the error message depend on the input types. The sanest way to do that is to allow conditioning on traits. For example, for #42526 we want something like:

trait IsNoneError {}
impl IsNoneError for NoneError {}

#[rustc_on_unimplemented(
               on(all(from_method="from",
                      from_desugaring="?",
                      matches("IsNoneError", Self="T")),
                  message="can't use Try on Option in a function returning Result")
                  )]
trait From<T> {
    // ...
}

The syntax I prefer is matches("TRAIT_NAME", Self="SELF_NAME", "PARAM_1_NAME", ..) where TRAIT_NAME and SELF_NAME are mandatory, but probably any syntax can work (this is all feature gated under the rustc_on_unimplemented attribute, so we can experiment).

At least for now, we can limit the parameters to be type parameters from our generics - the on_unimplemented code already knows how to look these up. You can use these to implement anything you like with the proper trait bound.

Mentoring notes here.

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Sep 24, 2017

Mentoring notes

This is somewhat of a moderately big feature and I'm not sure myself of the best way to implement it.

The current code that implements rustc_on_unimplemented is rustc::traits::on_unimplemented and is called from rustc::traits::error_reporting.

rustc_on_unimplemented is currently used in the Try trait. After the new Try for Option impl lands, we'll like to extend it to the From trait like I've said on the issue description.

I think the best syntax for our feature is what I had in the description (matches("IsNoneError", Self="T"))) because all the parsing is already done for us, but any syntax could be made work.

The matches is present in the attribute - you can see the on_unimplemented code for how to work with attributes.

I think the most annoying thing here is how to convert the trait name to the DefId for the trait. That requires performing name resolution. The problem is that name resolution information is not stored after HIR construction. So (@jseyfried: correct me if I'm wrong) within a crate (XXX that is incorrect: macros can perform name resolution cross-crate, maybe we can behave like a macro? @jseyfried tell me what to do here).

Then, we need to create our Substs, which should be easy - you can see how to refer to type parameters in the original trait ref from the format function in on_unimplemented.rs.

Once we have the DefId and Substs, we can create a PolyTraitRef, and match it using evaluate_obligation on a new SelectionContext (see e.g. what predicate_can_apply in rustc::traits::error_reporting does, but without the folder).

Don't forget to add tests and to add the implementation for Try.

@cramertj

This comment has been minimized.

Member

cramertj commented Oct 3, 2017

I'm starting on this.

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Oct 3, 2017

@cramertj cool! feel free to ask me for help.

@cramertj

This comment has been minimized.

Member

cramertj commented Oct 25, 2017

@arielb1 I've pushed up my WIP branch for this here. Can you take a look and let me know if the approach seems reasonable? Additionally, what's the best/easiest way to check if the Self type implements the provided trait, given DefIds for the Self type and the trait? I need something along those lines in order to complete this step.

@arielb1

This comment has been minimized.

Contributor

arielb1 commented Oct 25, 2017

@cramertj

The typeck part of your PR is basically OK. I'll like one of the resolution guys (e.g. @petrochenkov) go over your pull request and check that it doesn't use the wrong functions.

To check that your type implements the trait, you should probably pass a closure to evaluate from on_unimplemented_note, where you should create a Self: Trait obligation and call evaluate_obligation on it as e.g. method matching does.

You can create the Self: Trait obligation e.g. using the following code:

TraitRef {
    def_id: your_def_id,
    substs: tcx.mk_substs_trait(your_self_ty, &[])
}
@cramertj

This comment has been minimized.

Member

cramertj commented Oct 25, 2017

@arielb1 Great! Thank you so much for the quick feedback.

@cramertj

This comment has been minimized.

Member

cramertj commented Dec 1, 2017

My apologies-- I've been really busy with other things and had forgotten about this. If someone else wants to pick it up, they're welcome to it. If not, I'll try to schedule some time to finish this up soon.

bors added a commit that referenced this issue Feb 7, 2018

Auto merge of #47613 - estebank:rustc_on_unimplemented, r=nikomatsakis
Add filtering options to `rustc_on_unimplemented`

- Add filtering options to `rustc_on_unimplemented` for local traits, filtering on `Self` and type arguments.
- Add a way to provide custom notes.
- Tweak binops text.
- Add filter to detect wether `Self` is local or belongs to another crate.
- Add filter to `Iterator` diagnostic for `&str`.

Partly addresses #44755 with a different syntax, as a first approach. Fixes #46216, fixes #37522, CC #34297, #46806.

bors added a commit that referenced this issue Feb 7, 2018

Auto merge of #47613 - estebank:rustc_on_unimplemented, r=nikomatsakis
Add filtering options to `rustc_on_unimplemented`

- Add filtering options to `rustc_on_unimplemented` for local traits, filtering on `Self` and type arguments.
- Add a way to provide custom notes.
- Tweak binops text.
- Add filter to detect wether `Self` is local or belongs to another crate.
- Add filter to `Iterator` diagnostic for `&str`.

Partly addresses #44755 with a different syntax, as a first approach. Fixes #46216, fixes #37522, CC #34297, #46806.

Manishearth added a commit to Manishearth/rust that referenced this issue Feb 7, 2018

Rollup merge of rust-lang#47613 - estebank:rustc_on_unimplemented, r=…
…nikomatsakis

Add filtering options to `rustc_on_unimplemented`

- Add filtering options to `rustc_on_unimplemented` for local traits, filtering on `Self` and type arguments.
- Add a way to provide custom notes.
- Tweak binops text.
- Add filter to detect wether `Self` is local or belongs to another crate.
- Add filter to `Iterator` diagnostic for `&str`.

Partly addresses rust-lang#44755 with a different syntax, as a first approach. Fixes rust-lang#46216, fixes rust-lang#37522, CC rust-lang#34297, rust-lang#46806.

@estebank estebank closed this Jun 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment