Propagate lint attributes from the parent item to the constructors

[aldanor]

Ok, one more :) Now there should be no outstanding issues left other than releasing 1.0.

This propagates these types of attrs from the body to the ctors:

• #[allow(..)]
• #[deny(..)]
• #[forbid(..)]
• #[warn(..)]
• #[cfg_attr(.., allow(..))]
• #[cfg_attr(.., deny(..))]
• #[cfg_attr(.., forbid(..))]
• #[cfg_attr(.., warn(..))]

Closes #13.

@nrc @Arnavion @dtolnay

[Arnavion]

There isn't any established convention on this, is there? I'd really like to be able to select which attributes get inherited by which custom derive. Eg I want to disable too_many_arguments on derive-new's output but not on the output of other derives, so if all custom derives start inheriting all #[allow], etc attributes then I won't want to use #[allow(too_many_arguments)]

Maybe this can go in as it is anyway (for when the user does want an attribute to be inherited by all custom derives), but I think the fix for #13 should be #[new(allow(too_many_arguments))] instead.

[nrc]

Looks good, thanks! A couple of minor improvements inline.

[nrc]

if you extract this out into a helper function like is_lint, then the closure body could be just is_lint(...) || is_cfg_attr_lint(...)

[aldanor]

Good point, I've refactored that a bit which also shaved a few lines off.

[nrc]

Either in the function name or a comment, you should make clear that this collects only attrs which are lints, not all attrs.

[aldanor]

Renamed to collect_parent_lint_attrs, also the related variable.

[aldanor]

@Arnavion While #[new(allow(..))] and #[new(cfg_attr(..))] would definitely be more generic, you would have to write

#[derive(new)]
#[allow(non_snake_case)]
#[new(allow(non_snake_case))]
struct Foo { X: () }

which isn't overly nice. Plus, that would complicate attr parsing somewhat since now we'd need to support all of that inner syntax (or I guess we could just propagate all new(..) attached to the struct/enum body directly to the constructors without parsing any of it).

Besides, if at some point we support new(..) on the parent item, it could be used alongside the propagated attrs with no problems.

(Similarly, one could argue that the way we detect PhantomData is flaky/hacky -- which it is, but it would work in 99.9% real cases so it's "good enough"?..)

[Arnavion]

@aldanor No, you misunderstand what I said. With this change, #[allow(non_snake_case)] will be inherited on the impl block generated by derive-new and any other custom derives that have the same behavior. So it's fine if that's what the user who wants, and the extra #[new(allow(non_snake_case))] would be pointless.

I'm saying that if I only want #[allow(non_snake_case)] to be emitted on the impl block generated by derive-new and not on any other code generated by any other custom derives, then I'd use only #[new(allow(non_snake_case))] if derive-new supported such a thing. I would not use #[allow(non_snake_case)] in that case.

So I'm saying that this change is fine for the problem it's trying to solve, but that problem is not the one in #13

[Arnavion]

Personally I also think that a custom derive should not inherit any allow / deny from the original item unless explicitly told to do so, but I don't participate in any rust discussion lists (only IRC) so I don't know if there's already discussion / consensus about this. I can see why it would be inconvenient for things like snake_case because it's likely that all custom derives will require it if the original item requires it, but for things like too_many_arguments selectivity is required.

[aldanor]

@Arnavion I understood you quite well, maybe I didn't make myself clear enough :)

The easy solution is to just propagate all #[new(<foo>)] as #[<foo>] on the constructor. It's quite straightforward to do as I have implemented it on a local branch to see if it would work.

However, as I was thinking of a test, I don't think one exists (without using #[new(value = "..")]) if you only use standard library where this fails:

#![deny(some_lint)]

#[derive(new)]
struct Foo { ... }

but this works

#![deny(some_lint)]

#[derive(new)]
#[new(allow(some_lint))]
struct Foo { ... }

^ You're welcome to correct me if I'm wrong.

(Even with clippy, I suspect too_many_arguments is about the only lint that could potentially fit this. This sounds like a very specific use case which may not be worth introducing new syntax.)

If you use new(value = "..."), it's sure possible to have garbage code inside which would trigger lints:

#![deny(unused_unsafe)]

#[derive(new)
struct Foo { #[new(value = "unsafe { 42 }")] x: i32 }

but then this code is completely user-controlled so they could just fix it or add #[allow(..)] inside it.

[nrc]

@aldanor thank you!

[Arnavion]

The easy solution is to just propagate all #[new(<foo>)] as #[<foo>] on the constructor. It's quite straightforward to do as I have implemented it on a local branch to see if it would work.

Please make a PR of that then, otherwise @nrc could you please reopen #13 ? It is important to me to have no lint warnings so I'm using my own custom derive that always emits #[allow(too_many_arguments)]. I would like to switch back to derive-new but a cleaner lint is more important.

This sounds like a very specific use case which may not be worth introducing new syntax.

It will be... unfortunate... if that is the decision, especially if other custom derives use this as a precedent to do the same.

[aldanor]

I'll leave this to @nrc to decide whether we need to support propagating arbitrary parent attrs on demand. If so, will need to figure the details and then I can push my local branch as a PR.

I personally don't think a single clippy lint is worth adding new syntax; that being said, there may be non-lint use cases, like doing #[new(doc = "My custom docstring, just because.")]. It gets more complicated for enums though...