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

#[derive(Copy="something")] should be rejected #21435

Closed
aochagavia opened this Issue Jan 20, 2015 · 12 comments

Comments

Projects
None yet
5 participants
@aochagavia
Copy link
Contributor

aochagavia commented Jan 20, 2015

This compiles:

#[derive(Clone="2", Show(Wrong))]
struct MyStruct;

Other wrong attributes:

#[deprecated(Very, Wrong)]
#[test="something"]
#[test(Wrong)]
#[stable(Wrong)]
#[unstable(Wrong)]
@aochagavia

This comment has been minimized.

Copy link
Contributor Author

aochagavia commented Jan 20, 2015

@nrc nrc added the A-diagnostics label Jan 20, 2015

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jan 20, 2015

I don't think it is intentional, the derive parser will collect arguments, and then the copy or clone impls will silently ignore them. It would be good to error or warn if we can here.

@aochagavia

This comment has been minimized.

Copy link
Contributor Author

aochagavia commented Jan 20, 2015

I could add a fix in #21265. Or do you prefer that I open a new PR for that?

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jan 20, 2015

new PR please

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jan 20, 2015

I guess this is technically a backwards incompatible change, since it will break programs with extraneous params. Although it is also just a bug. In any case, nomintating.

@nrc nrc added the I-nominated label Jan 20, 2015

@kmcallister

This comment has been minimized.

Copy link
Contributor

kmcallister commented Jan 20, 2015

I think we have this problem throughout attribute handling code and not just on derive :/

@aochagavia

This comment has been minimized.

Copy link
Contributor Author

aochagavia commented Jan 21, 2015

I have updated the code example, since it is also possible to do #[derive(Clone(Wrong))]

I think the problem is in L61 and L62. We should only accept MetaWord and produce errors for MetaNameVariable and MetaList.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 22, 2015

many cases like this in the compiler where we are not vigilent about rejecting malformed attributes.

@pnkfelix pnkfelix added this to the 1.0 milestone Jan 22, 2015

@brson brson added the E-easy label Jan 22, 2015

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jan 22, 2015

polish issue, where it would not be the end of the world if we did not get around to addressing it, though it would be nice if it were fixed.

Assigning 1.0, P-low.

@pnkfelix pnkfelix added P-low and removed I-nominated labels Jan 22, 2015

@kmcallister

This comment has been minimized.

Copy link
Contributor

kmcallister commented Jan 22, 2015

I think people will cut us a little slack on backwards compatibility of crap like

#[derive(Clone="2", Show(Wrong))]

The only way I could see this mattering is if a plugin makes an ill-advised attempt to extend the derive syntax. But plugins are unstable anyway.

@aochagavia

This comment has been minimized.

Copy link
Contributor Author

aochagavia commented Jan 25, 2015

I have updated the list of attributes (added #[test], #[deprecated], #[stable], #[unstable])

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Apr 2, 2015

This appears to be fixed; closing.

@pnkfelix pnkfelix closed this Apr 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.