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

trait with a len() method but without is_empty() method #60

Closed
llogiq opened this issue May 19, 2015 · 3 comments
Closed

trait with a len() method but without is_empty() method #60

llogiq opened this issue May 19, 2015 · 3 comments
Assignees

Comments

@llogiq
Copy link
Contributor

llogiq commented May 19, 2015

Regarding my failed attempt to fully solve #32 (which would require more typeck than external lints can currently acquire), I thought about the social contract around .len() and .is_empty(). While we don't have a Bounded trait that defines .len() and implements .is_empty(), we can still ask implementers to provide the latter.

There is some precedence to the use of is_empty: E.g. in Java, the Collection interface defines boolean isEmpty() with the usual meaning. C++'s STL defines empty() on all containers, Ocaml at least defines is_empty for stacks. Python's lists and Lua's tables are basically arrays, so they have no problem with using len(_).

In any event, we could add a lint that looks for traits which define .len() but do not define .is_empty() and ask the implementer to consider adding it. This would also remove the need for method lookup on that other lint – if most traits have .is_empty(), we won't need to care for the few remaining false positives, right?

A basic implementation would check_item for ItemTraits and see if the contained TraitItems fail to adhere to the social contract.

@tamird
Copy link
Contributor

tamird commented May 19, 2015

I agree with all of this except:

we won't need to care for the few remaining false positives, right?

I don't think false positives are something people should live with and suppress on a case-by-case basis.

@llogiq
Copy link
Contributor Author

llogiq commented May 19, 2015

@tamird Thanks for the heads-up. Just to clarify: A false positive in this case would be using an item (trait or type impl) that has a .len(), but no .is_empty() method (and that would have been flagged by this lint). So I think the cost of suppressing the one or two stray false positives is outweighed by the benefit of the lint I have for issue #32 without method lookup.

Please note that this is only an interim solution until rustc_typeck is made accessible to third party lint writers. The process to do this has already begun, so it's only a matter of time until we can do the method lookup and get rid of the remaining false positives.

@Manishearth
Copy link
Member

Note that this crate is about lints, and false positives if not too common are okay.

A lot of the things here, like Box<Vec<T>> have legitimate uses too, but very few especially from a newbie perspective.

@llogiq llogiq self-assigned this May 20, 2015
@llogiq llogiq closed this as completed May 20, 2015
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 a pull request may close this issue.

3 participants