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

Lint `set_*` and `get_*` methods that do nothing but access the field #1673

Open
oli-obk opened this Issue Apr 12, 2017 · 17 comments

Comments

Projects
None yet
5 participants
@oli-obk
Collaborator

oli-obk commented Apr 12, 2017

It is not idiomatic in Rust to have setters and getters. Make the field public instead.

If the type only has a get_foo method but not a set_foo method, suggest naming it foo instead.

@0xazure

This comment has been minimized.

0xazure commented Nov 20, 2018

Hey @oli-obk I'm interested in having a go at implementing this lint. Is it still up for grabs?

@Manishearth

This comment has been minimized.

Member

Manishearth commented Nov 20, 2018

Actually, I'm not sure if this really is valid rust style -- setters and getters may be added to future proof an API, for example.

@0xazure

This comment has been minimized.

0xazure commented Nov 20, 2018

future proof an API

That's a really good point, I can definitely see that use case.

That would also nix linting if it only has a get_foo since the setter might be implemented at a later date.

Move to close this?

@hcpl

This comment has been minimized.

hcpl commented Nov 20, 2018

Move to close this?

The "If the type only has a get_foo method but not a set_foo method, suggest naming it foo instead" part is still relevant IMO, so I vote against.

@0xazure

This comment has been minimized.

0xazure commented Nov 20, 2018

If we're considering the case of future-proofing an API, if the method named foo gets the value what should be done if a setter needs to be implemented at a later date? The foo method would need to be renamed get_foo before set_foo is introduced, which I think breaks the same kind of compatibility as changing from public fields to methods @Manishearth pointed out.

Maybe I'm missing something about the use case though, @hcpl do you have an example?

@hcpl

This comment has been minimized.

hcpl commented Nov 20, 2018

@0xazure yeah, for example, std::net::SocketAddr has ip as the getter and set_ip as the setter.

Not sure if this particular instance is idiomatic by any means, but it shows that this naming scheme has a precedent in the standard library.

Also, foo doesn't need to be renamed: SocketAddr::ip() was introduced in Rust 1.7.0 and SocketAddr::set_ip() --- in Rust 1.9.0 (again, I don't know whether this was idiomatic or a workaround to keep backwards compatibility, but it works).

@0xazure

This comment has been minimized.

0xazure commented Nov 20, 2018

Thanks for the example, @hcpl!

I don't think it's idiomatic, but it looks like ip was stabilized in 1.7.0 and set_ip was stabilized later in 1.9.0 (it also looks like port and set_port from that same enum follow the same pattern).

This proves the point of future-proofing though; ip couldn't be renamed get_ip when set_ip was introduced, so it should have been named get_ip in the first place. However, naming it get_ip would not pass the proposed lint because at the time there was no corresponding set_ip method so the lint would suggest naming it ip instead.

@hcpl

This comment has been minimized.

hcpl commented Nov 20, 2018

Personally, I find the get_foo/set_foo naming scheme less pleasant than foo/set_foo because when reading code foo/set_foo having different lengths makes them more visually distinct than get_foo/set_foo which only differ in the first letter. Also I tend to access data more than modify in my code so with foo I have to type less, but since code tends to be read more than written, that's a minor point compared to the previous one.

And I just found this in API guidelines: https://rust-lang-nursery.github.io/api-guidelines/naming.html#c-getter. Curiously, setters aren't mentioned anywhere in the guidelines (would be good to notify maintainers over there about this topic).

@0xazure

This comment has been minimized.

0xazure commented Nov 20, 2018

I don't personally have any preference for get_foo/set_foo over foo/set_foo, I think the consistency is more important.

That API Guidelines section is a great find!

With a few exceptions, the get_ prefix is not used for getters in Rust code.

Based on that section the ip/set_ip naming is intentional, and I've turned up a bunch more examples in std::fs like len/set_len, permissions/set_permissions, etc.

It looks like we should lint to disallow get_ prefixes, especially if there's a matching set_ method, though I'm not 100% sure how we'd want to go about allowing the special cases as in Cell::get and get_mut or get_unchecked unless those are the only cases get appears.

@flip1995

This comment has been minimized.

Collaborator

flip1995 commented Nov 20, 2018

Searching the std lib of rust for get* you can find the special cases

  • get
  • get_mut
  • get_ref
  • get_unchecked
  • (get_type_id)
    and combinations of the functions above.

I think we can add a style lint for getters prefixed with get_, that aren't one of the special cases above and link to the API guidelines in the documentation of the lint.

@0xazure

This comment has been minimized.

0xazure commented Nov 20, 2018

get_type_id stabilization is tracked as rust-lang/rust#27745, there's a comment noting it could be renamed to type_id so I think it's an outlier in this case.

@flip1995

This comment has been minimized.

Collaborator

flip1995 commented Nov 21, 2018

Yes I don't think this should be included in the special cases, just listed it for completeness.

bors bot added a commit to comprakt/comprakt that referenced this issue Nov 21, 2018

Merge #122
122: rename getter functions r=reiner-dolp a=flip1995

See [API-guidelines](https://rust-lang-nursery.github.io/api-guidelines/naming.html?highlight=getter#getter-names-follow-rust-convention-c-getter)

Soon this will be a Clippy lint: rust-lang/rust-clippy#1673

Co-authored-by: flip1995 <hello@philkrones.com>
@0xazure

This comment has been minimized.

0xazure commented Nov 23, 2018

To summarize, if the type has a get_foo method we should suggest naming it foo instead to follow the API Guidelines for Rust getter name conventions except for cases of:

  • get
  • get_mut
  • get_unchecked
  • get_unchecked_mut
  • get_ref

This should be enough to get me started on a style lint for this convention, I should have some time over the next couple days to start digging into this.

@flip1995

This comment has been minimized.

Collaborator

flip1995 commented Nov 23, 2018

Exactly! It shouldn't be required to explicitly filter get_unchecked_mut when already filtering get_unchecked, if you check for prefixes.

@0xazure

This comment has been minimized.

0xazure commented Nov 23, 2018

I think that's an interesting question, should we lint for only the prefixes (e.g. get_mut*, get_unchecked*)? I foresee the potential for method names like get_unchecked_foo_bar or get_reference_for_foo that would pass a test for only the prefixes.

@flip1995

This comment has been minimized.

Collaborator

flip1995 commented Nov 23, 2018

Yeah this methods would pass the lint. But is this bad? get_unchecked_foo vs. foo_unchecked: I'm not sure what I'd prefer. I tend to the former since get_unchecked is common in rust.

@0xazure

This comment has been minimized.

0xazure commented Dec 11, 2018

Just wanted to post an update, I'm still working on this but have other demands on my time right now. I should get back to it in a week or so, hopefully have a PR up soon after that.

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