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

Extend #[deriving(Default)] to support specified field defaults #14219

Closed
wants to merge 1 commit into from

Conversation

lilyball
Copy link
Contributor

When deriving the Default trait, individual struct fields can be marked
up with a #[default="expr"] attribute to indicate that this field
should be given the results of expr instead of Default::default().

Example:

#[deriving(Default)]
struct Foo {
    #[default="42"]
    x: int,
    #[default="Ok(())"]
    err: IoResult<()>
}

@pcwalton
Copy link
Contributor

Need to think about how this interacts with macro hygiene.

@pcwalton
Copy link
Contributor

I think it won't be a problem though as long as the macro expander keeps the identifiers at the same scope.

@lilyball
Copy link
Contributor Author

In general, I agree. In this specific case, the expansion is done immediately after the struct, which means it's in the same scope, which means it doesn't even matter if the macro expander handles identifier hygiene (but only in the context of #[deriving]).

@huonw
Copy link
Member

huonw commented May 15, 2014

I think this should be feature gated; because the use of strings seems unfortunate and the possible concerns about hygiene/resolution.

ast::LitStr(ref s, _) => Some(cx.parse_expr(s.get().to_strbuf())),
_ => {
cx.span_err(v.span,
"non-string literals are not allowed in #[default] attribute");
Copy link
Member

Choose a reason for hiding this comment

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

I think the convention is for backticks around "code".

@lilyball
Copy link
Contributor Author

@huonw Feature-gating suggests that there's some way it will become un-feature-gated at some point. But our attribute syntax doesn't allow for expressions as the value. If we change the attribute syntax that will presumably be a backwards-incompatible change anyway, so I don't see the danger in leaving this un-feature-gated.

@huonw
Copy link
Member

huonw commented May 15, 2014

If we change the attribute syntax that will presumably be a backwards-incompatible change anyway

I don't think it is. It seems (relatively) reasonable for attributes to just allow arbitrary token trees, or even just the = form allowing tts (or expressions) on the RHS. Any of those is backwards-compatible.

@lilyball
Copy link
Contributor Author

Perhaps. But unless there's any expectation that this will ever actually be done, feature-gating this functionality just serves to ensure nobody ever uses it.

@huonw
Copy link
Member

huonw commented May 15, 2014

feature-gating this functionality just serves to ensure nobody ever uses it.

That is definitely hyperbolic: people do use feature gated things.

@lilyball
Copy link
Contributor Author

People use feature-gated things when either they cannot do what they want without it, or it's sufficiently problematic to do what they want without it. But it's a lot less likely that someone will be willing to use a feature-gated string just to avoid writing an impl Default.

In any case, my point still stands that, unless we actually believe that we may change the attribute syntax to allow arbitrary token trees, then feature-gating this doesn't make sense. The only good reason to feature-gate it is so that making that change to attributes doesn't break anyone who's already using this feature. But unless we think we're actually going to make such a change, that reason doesn't apply. And if we feature-gate it now on the off chance that we decide to make that change later, then it will never get un-feature-gated (because it was based on a hypothetical, one that will remain possible indefinitely).

@alexcrichton
Copy link
Member

I would be cautious about adding a feature such as this. We don't currently have any form of customizing #[deriving], and I fear that adding this would open up the floor for many more changes to come in. It's tough to see how all these deriving modes would interact and it would be a little unfortunate for structs to normally be littered with attributes for all the various behaviors they may want.

The idea in general seems appealing, but it also doesn't seem that far off from just writing impl Default yourself. Allowing arbitrary expressions in strings also seems... interesting. I would personally rather keep deriving as simple as possible because that's easy to understand, and collect some more compelling use cases before adding a feature such as this.

When deriving the Default trait, individual struct fields can be marked
up with a `#[default="expr"]` attribute to indicate that this field
should be given the results of `expr` instead of `Default::default()`.

Example:

    #[deriving(Default)]
    struct Foo {
        #[default="42"]
        x: int,
        #[default="Ok(())"]
        err: IoResult<()>
    }
@lilyball
Copy link
Contributor Author

@alexcrichton I was under the impression that the ability to customize #[deriving] was generally regarded as something we'd like to have? Certainly every time I've seen someone mention a desire to have pluggable derivings the responses was always universally in favor of the idea.

As for customizing the ability of existing #[deriving], I can't think offhand of any other derivings where such customizability makes sense.

The idea in general seems appealing, but it also doesn't seem that far off from just writing impl Default yourself.

Sure, and the ability to write int x = 3 in C++11 structs isn't too far off from just saying x(3) in all your constructors. But it was done because it's a convenient feature that people like. Similarly, the ability to derive Default is nice, but it only works if every single field implements it. This PR is largely meant to solve the case where only one field doesn't implement it but the default is correct for everything else.

Allowing arbitrary expressions in strings also seems... interesting.

I agree it's unfortunate, but the only real alternatives are to either allow arbitrary token streams in attributes, or to add syntax x: int = 42 to the actual language itself, which I don't think we want to do.

collect some more compelling use cases before adding a feature such as this.

I think the most compelling reason to do this is the fact that almost nothing uses #[deriving(Default)], and that's because very little of our standard libraries actually bother to provide Default implementations. In fact, the only types I can see in our standard library that use #[deriving(Default)] are glob::MatchOptions and glob::Pattern.

The idea is that by lowering the amount of effort it takes to provide a Default implementation, it's more likely for people to actually implement it.

@lilyball
Copy link
Contributor Author

I've pushed a new version based on @huonw's feedback.

@lilyball
Copy link
Contributor Author

As an example of the point I just made, StrBuf does not currently implement Default. It obviously should. But as long as it doesn't, anyone with a struct with a StrBuf field cannot #[derive(Default)].

@alexcrichton
Copy link
Member

I can't think offhand of any other derivings where such customizability makes sense.

  • Show - why not show some fields and not others? Perhaps some fields should be transformed when being displayed (such as calling .display())
  • Eq/TotalEq - maybe the field foo doesn't lend itself to equality
  • Ord/TotalOrd - maybe field bar takes precedent over foo, and baz shouldn't be compared at all.
  • Clone - I don't want to call clone(), I'd rather call my_clone()
  • Hash - field foo may not want to be hashed
  • Encodable/Decodable - I'd like to omit field x, rename y to z, reorder b above a, etc.

The list does not end with Default. This is not the only deriving mode which will have a request to be configured.

@lilyball
Copy link
Contributor Author

All of those with the exception of Encodable/Decodable would require passing the field (or the entire struct) in to the expression. I don't see how to handle that, without doing something like #[show="|field| field.display()"] which I think is pretty gross. A single stand-alone expression as a string is unfortunate enough, but embedding a whole closure in there? That's just too far.

Encodable/Decodable I didn't think of. And actually, I do want to do precisely that sort of thing. Go supports the ability to e.g. change the name of the field when encoding to JSON. It would be really convenient if Rust could do the same thing. This was even suggested recently in IRC. Although since it's actually a generic Encodable/Decodable that's implemented rather than specifically JSON support, I'd be really cautious about trying to do that.

That said, the existence of #[default] does not mean we must go hog-wild with everything. With the exception of the unfortunate aspect that the expression must be contained in a string, #[default] is rather straightforward and turns the currently nearly useless #[deriving(Default)] into something that we can actually use. None of the other suggestions are straightforward or simple, and thus I don't expect anyone to naturally assume that the existence of #[default] means the others are obviously desired.

@liigo
Copy link
Contributor

liigo commented May 15, 2014

I like this PR!
2014年5月15日 下午2:36于 "Kevin Ballard" notifications@github.com写道:

All of those with the exception of Encodable/Decodable would require
passing the field (or the entire struct) in to the expression. I don't see
how to handle that, without doing something like #[show="|field|
field.display()"] which I think is pretty gross. A single stand-alone
expression as a string is unfortunate enough, but embedding a whole closure
in there? That's just too far.

Encodable/Decodable I didn't think of. And actually, I do want to do
precisely that sort of thing. Go supports the ability to e.g. change the
name of the field when encoding to JSON. It would be really convenient if
Rust could do the same thing. This was even suggested recently in IRC.
Although since it's actually a generic Encodable/Decodable that's
implemented rather than specifically JSON support, I'd be really cautious
about trying to do that.

That said, the existence of #[default] does not mean we must go hog-wild
with everything. With the exception of the unfortunate aspect that the
expression must be contained in a string, #[default] is rather
straightforward and turns the currently nearly useless
#[deriving(Default)] into something that we can actually use. None of the
other suggestions are straightforward or simple, and thus I don't expect
anyone to naturally assume that the existence of #[default] means the
others are obviously desired.


Reply to this email directly or view it on GitHubhttps://github.com//pull/14219#issuecomment-43173716
.

@alexcrichton
Copy link
Member

If merged un-feature-gated, then need to have a story for customizing all deriving modes, whether that be through these manual attributes or some other method. It doesn't sound like that story currently exists.

@lilyball
Copy link
Contributor Author

@alexcrichton As discussed in #14268, I'm in favor of coming up with a solution for customizing all deriving modes.

I'm going to close this PR for the time being, until we've come up with the answer to that.

@lilyball lilyball closed this May 19, 2014
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 20, 2025
The `comparison_chain` lint might suggest code which seems less natural
to beginners.

[Discussion](https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/demote.20some.20lints.20to.20.60pedantic.60)

changelog: [`comparison_chain`]: change lint category to `pedantic`
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.

5 participants