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

Support an explicit annotation for marker traits #53693

Open
wants to merge 5 commits into
base: master
from

Conversation

@scottmcm
Member

scottmcm commented Aug 25, 2018

From the tracking issue for rust-lang/rfcs#1268:

It seems obvious that we should make a #[marker] annotation. ~ #29864 (comment)

This PR allows you to put #[marker] on a trait, at which point:

  • The trait must not have any items All of the trait's items must have defaults
  • Any impl of the trait must be empty (not override any items)
  • But impls of the trait are allowed to overlap

r? @nikomatsakis

@rust-lang rust-lang deleted a comment from rust-highfive Aug 25, 2018

@rust-lang rust-lang deleted a comment from rust-highfive Aug 25, 2018

@scottmcm scottmcm changed the title from [WIP] Support an explicit annotation for marker traits to Support an explicit annotation for marker traits Aug 26, 2018

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Aug 26, 2018

Member

I think this is functional and sound, but it's missing the "this trait would be impossible to implement" error when defining a trait without default implementations for its items.

I'd appreciate advice on where that should go, as well as any other feedback on how I've been going about this.

Member

scottmcm commented Aug 26, 2018

I think this is functional and sound, but it's missing the "this trait would be impossible to implement" error when defining a trait without default implementations for its items.

I'd appreciate advice on where that should go, as well as any other feedback on how I've been going about this.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 27, 2018

Contributor

@scottmcm this is confusing to me. I thought a marker trait had no items -- not "items with defaults".

Contributor

nikomatsakis commented Aug 27, 2018

@scottmcm this is confusing to me. I thought a marker trait had no items -- not "items with defaults".

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Aug 27, 2018

Member

@nikomatsakis See #29864 (comment) for some context. But yes, it's an extension beyond the RFC, so I'm fine not implementing that for now.

I do still need advice on where to put the "has no items" valuation check, though. (Once I know where I can figure out how to get the item list.)

Member

scottmcm commented Aug 27, 2018

@nikomatsakis See #29864 (comment) for some context. But yes, it's an extension beyond the RFC, so I'm fine not implementing that for now.

I do still need advice on where to put the "has no items" valuation check, though. (Once I know where I can figure out how to get the item list.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 27, 2018

Contributor

@scottmcm I see. That seems like an ok extension, actually, but in that case I don't know what this is referring to...

I think this is functional and sound, but it's missing the "this trait would be impossible to implement" error when defining a trait without default implementations for its items.

...oh, I see, I thought you meant some fancy check designed to prove that the trait is imposible to implement. But actually you just mean that you want to report an error if you label a trait as #[marker] and it has items that lack defaults?

In that case, I think we should do that .. hmm .. either in WF or in coherence. Coherence seems like -- today -- it is focused on checking properties of impls so maybe wfcheck would be a better place.

Basically extending this function:

fn check_trait<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, item: &hir::Item) {
let trait_def_id = tcx.hir.local_def_id(item.id);
for_item(tcx, item).with_fcx(|fcx, _| {
check_where_clauses(tcx, fcx, item.span, trait_def_id, None);
vec![]
});
}

We should probably do a quick lang-team FCP here too.

Contributor

nikomatsakis commented Aug 27, 2018

@scottmcm I see. That seems like an ok extension, actually, but in that case I don't know what this is referring to...

I think this is functional and sound, but it's missing the "this trait would be impossible to implement" error when defining a trait without default implementations for its items.

...oh, I see, I thought you meant some fancy check designed to prove that the trait is imposible to implement. But actually you just mean that you want to report an error if you label a trait as #[marker] and it has items that lack defaults?

In that case, I think we should do that .. hmm .. either in WF or in coherence. Coherence seems like -- today -- it is focused on checking properties of impls so maybe wfcheck would be a better place.

Basically extending this function:

fn check_trait<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, item: &hir::Item) {
let trait_def_id = tcx.hir.local_def_id(item.id);
for_item(tcx, item).with_fcx(|fcx, _| {
check_where_clauses(tcx, fcx, item.span, trait_def_id, None);
vec![]
});
}

We should probably do a quick lang-team FCP here too.

@nikomatsakis nikomatsakis added the T-lang label Aug 27, 2018

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 27, 2018

Contributor

@rfcbot fcp merge

This PR modifies the special treatment of marker traits in two ways. First, you have to declare the traits explicitly, by writing #[marker], but secondly, it permits items in those traits -- so long as those items have defaults. However, once a trait is defined as #[marker], impls of the trait are then disallowed from containing any items, so you know that the defaults from the trait will be used.

Seems reasonable to me.

Contributor

nikomatsakis commented Aug 27, 2018

@rfcbot fcp merge

This PR modifies the special treatment of marker traits in two ways. First, you have to declare the traits explicitly, by writing #[marker], but secondly, it permits items in those traits -- so long as those items have defaults. However, once a trait is defined as #[marker], impls of the trait are then disallowed from containing any items, so you know that the defaults from the trait will be used.

Seems reasonable to me.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Aug 27, 2018

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Aug 27, 2018

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Aug 27, 2018

Contributor

@rfcbot concern vague

I'm 👍 on having a #[marker] annotation for marker traits with no items, and requiring that to get the overlapping allowance as a statement of your indefinite, permanent intent that this trait is a marker trait.

I'm not so convinced that redefining marker trait to include traits with default items and then adding additional checks that those are never overriden is a good idea. It comes at a nontrivial cost of complexity for users in terms of understanding the meaning and mechanics of both default methods and marker traits. The motivation doesn't seem that strong to me.

Contributor

withoutboats commented Aug 27, 2018

@rfcbot concern vague

I'm 👍 on having a #[marker] annotation for marker traits with no items, and requiring that to get the overlapping allowance as a statement of your indefinite, permanent intent that this trait is a marker trait.

I'm not so convinced that redefining marker trait to include traits with default items and then adding additional checks that those are never overriden is a good idea. It comes at a nontrivial cost of complexity for users in terms of understanding the meaning and mechanics of both default methods and marker traits. The motivation doesn't seem that strong to me.

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Aug 27, 2018

Contributor

I very much agree that the incoherency should be opt-in rather than be implicit.

However; this was not discussed in the RFC (unless I somehow missed that but I did look at every comment...) and this could use some more elaborate and detailed justification.
For example, the changes wrt. provided definitions could use some more explaining per @withoutboats's concern. This is also, relatively speaking to the size of the original RFC, quite a large change.
Therefore I'd like to move this back to the drawing board, so:

@rfcbot concern needs-rfc

In addition; I am not so sure about the concrete syntax here.
For example, it seems to me that #[overlappable] (as suggested by @Vurich in #29864 (comment)) or #[incoherent] is more informative than #[marker] as to what effect it has. I think this holds particularly if associated items with defaults are permitted but can't be overridden as it isn't a marker trait anymore.

The choice of using an attribute rather than some keyword as a prefix to the trait (e.g. overlappable trait Foo { .. }) hasn't been sufficiently justified either in my opinion. The justification that has been given (#29864 (comment)) is merely that there is precedent to use an attribute. But precedent alone is not sufficient. In would like to see a discussion of this in an RFC. As such:

@rfcbot concern concrete-syntax

Contributor

Centril commented Aug 27, 2018

I very much agree that the incoherency should be opt-in rather than be implicit.

However; this was not discussed in the RFC (unless I somehow missed that but I did look at every comment...) and this could use some more elaborate and detailed justification.
For example, the changes wrt. provided definitions could use some more explaining per @withoutboats's concern. This is also, relatively speaking to the size of the original RFC, quite a large change.
Therefore I'd like to move this back to the drawing board, so:

@rfcbot concern needs-rfc

In addition; I am not so sure about the concrete syntax here.
For example, it seems to me that #[overlappable] (as suggested by @Vurich in #29864 (comment)) or #[incoherent] is more informative than #[marker] as to what effect it has. I think this holds particularly if associated items with defaults are permitted but can't be overridden as it isn't a marker trait anymore.

The choice of using an attribute rather than some keyword as a prefix to the trait (e.g. overlappable trait Foo { .. }) hasn't been sufficiently justified either in my opinion. The justification that has been given (#29864 (comment)) is merely that there is precedent to use an attribute. But precedent alone is not sufficient. In would like to see a discussion of this in an RFC. As such:

@rfcbot concern concrete-syntax

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Aug 27, 2018

Contributor

To elaborate a bit: I'm fine with landing this PR and using #[marker] as a temporary syntax; but not with resolving the final design at this stage.

Contributor

Centril commented Aug 27, 2018

To elaborate a bit: I'm fine with landing this PR and using #[marker] as a temporary syntax; but not with resolving the final design at this stage.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Aug 27, 2018

Member

I agree with @Centril and @withoutboats - this seems like a big change and marker traits with default methods adds considerably to the complexity of the feature, as well as making marker not really an appropriate name, IMO.

Member

nrc commented Aug 27, 2018

I agree with @Centril and @withoutboats - this seems like a big change and marker traits with default methods adds considerably to the complexity of the feature, as well as making marker not really an appropriate name, IMO.

@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Aug 28, 2018

Member

Ok, I'll just do the "opt into the RFC behaviour using an attribute" part of the change, making having anything in a marker trait illegal and postponing any extensions for later.

I think #[marker] is correct because of the core::marker module, so this is already a term of art. This PR isn't stabilizing anything, so keep bikeshedding and we can change it later.

Member

scottmcm commented Aug 28, 2018

Ok, I'll just do the "opt into the RFC behaviour using an attribute" part of the change, making having anything in a marker trait illegal and postponing any extensions for later.

I think #[marker] is correct because of the core::marker module, so this is already a term of art. This PR isn't stabilizing anything, so keep bikeshedding and we can change it later.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Aug 28, 2018

Contributor

@rfcbot resolve vague

Contributor

withoutboats commented Aug 28, 2018

@rfcbot resolve vague

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Aug 28, 2018

Contributor

@rfcbot resolve needs-rfc
@rfcbot resolve concrete-syntax

Let's get this PR through with #[marker] as temporary syntax :)
But before stabilization I'd like to see an RFC.
I'd also prefer a different naming; I'll elaborate on why in the tracking issue.

Contributor

Centril commented Aug 28, 2018

@rfcbot resolve needs-rfc
@rfcbot resolve concrete-syntax

Let's get this PR through with #[marker] as temporary syntax :)
But before stabilization I'd like to see an RFC.
I'd also prefer a different naming; I'll elaborate on why in the tracking issue.

@eddyb

eddyb approved these changes Aug 28, 2018

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 28, 2018

Contributor

@scottmcm ping me when you've updated with the more minimal scope; I agree with the general consensus that we ought to go through RFC process here.

Contributor

nikomatsakis commented Aug 28, 2018

@scottmcm ping me when you've updated with the more minimal scope; I agree with the general consensus that we ought to go through RFC process here.

scottmcm added some commits Sep 3, 2018

Improve error checking and reporting
- Check for valid attributes more reliably
- Don't bloat the error index for boring errors
- Do use real error codes for the interesting ones
@scottmcm

This comment has been minimized.

Show comment
Hide comment
@scottmcm

scottmcm Sep 3, 2018

Member

Ok, @nikomatsakis, I've restricted this to the minimal scope:

#[marker]
trait MarkerConstWithDefault {
const N: usize = 43;
//~^ ERROR marker traits cannot have associated items
}
#[marker]
trait MarkerTypeWithDefault {
type Output = ();
//~^ ERROR marker traits cannot have associated items
}
#[marker]
trait MarkerFnWithDefault {
fn foo() {}
//~^ ERROR marker traits cannot have associated items
}

Member

scottmcm commented Sep 3, 2018

Ok, @nikomatsakis, I've restricted this to the minimal scope:

#[marker]
trait MarkerConstWithDefault {
const N: usize = 43;
//~^ ERROR marker traits cannot have associated items
}
#[marker]
trait MarkerTypeWithDefault {
type Output = ();
//~^ ERROR marker traits cannot have associated items
}
#[marker]
trait MarkerFnWithDefault {
fn foo() {}
//~^ ERROR marker traits cannot have associated items
}

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 17, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Sep 17, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 17, 2018

Contributor

@bors r=me with a writeup for relnotes.

@scottmcm can you add a brief paragraph to leave for release notes?

I do not anticipate much comment here, and we can always back this change out (it's not affecting anything stable).

Contributor

nikomatsakis commented Sep 17, 2018

@bors r=me with a writeup for relnotes.

@scottmcm can you add a brief paragraph to leave for release notes?

I do not anticipate much comment here, and we can always back this change out (it's not affecting anything stable).

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis
Contributor

nikomatsakis commented Sep 17, 2018

@bors r-

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