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

lint against impl trait in the argument position #2789

Closed
warlord500 opened this Issue May 22, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@warlord500
Copy link

warlord500 commented May 22, 2018

while rust has just introduced impl trait to stable, impl trait in the argument position should be an official discouraged practice because there are now three ways to write a generic function.

by linting against this we can encourage the older syntax more often
it also allows those who disagree with it to discourage it in their code base

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented May 22, 2018

I don't think it currently is an officially discouraged practice especially since the RFC landed pretty recently.

@warlord500

This comment has been minimized.

Copy link
Author

warlord500 commented May 23, 2018

It currently isnt a discouraged practice just that one of the three ways to write generic function. With that, i think impl trait in args should be discouraged in code bases that disagree with using it. Having a lint for those who disagree with it would beneficial to those people. Those who dont just leave clippy lint
on allow which would hardly affect them, thus I see no reason to close this so suddenly just because,
it isnt officially discouraged yet.

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented May 23, 2018

just because it isnt officially discouraged yet.

rust-lang/rfcs#2444 (comment) seems to suggest that that will never happen.

While we do have some unusual restriction lints, this is a special situation as there's loads of emotional stuff going on. I do not want to drag clippy into that discussion by offering a lint that goes against the majority of the community and against the official process for making changes.

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented May 23, 2018

Agreed. Clippy does have restriction lints but those are not for "I dislike this", those are for specialized situations where you want additional restrictions.

And yeah, as an emotional subject we're probably not going to touch it anyway.

Clippy isn't an escape hatch around community process.

@warlord500

This comment has been minimized.

Copy link
Author

warlord500 commented May 23, 2018

I agree that dragging clippy into emotional battle would be absolutely horrible idea.
if you accept nearly every lint, but that lint always start out as allow in the beginning, then via some formal process for making these denied by default, clippy wont be drag into battle. yes the formal process might be a bit of a debate but that doesn't stop clippy for accepting any lint.

the point, I want to make crystal clear is that if someone feels strongly enough about the topic to write a lint against the behaivor, I think having the ability for clippy to accept even though it might go against the grain but set to allow by default seems perfectly reasonable. if clippy cant do that for a techinical reason then possibly a new project needs to be created for unusual lints "community members" would like to support.

in way, by being inclusive, clippy informal way of showing what kinda of code the community actually likes compared to what other "community members" or leaders thinks the community likes.

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented May 23, 2018

Yes, clippy can accept such a lint, that doesn't mean it should.

A separate project for this sounds great.

@warlord500

This comment has been minimized.

Copy link
Author

warlord500 commented May 23, 2018

for your comment @oli-obk, clippy has definitely did accept a lint that falls under "I dislike this".
and those lints are shadow_reuse, shadow_same, shadow_ as well as else if without else.

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented May 23, 2018

The shadow lints and the "else if without else" actually catch bugs in projects that have high safety requirements.

The only lint that comes to mind that violates this rule is the assign_ops lint, and I'll happily accept a PR that removes this lint!

@warlord500

This comment has been minimized.

Copy link
Author

warlord500 commented May 23, 2018

I have to disagree with @oli-obk, if you actually bother to read the why for those clipply lints, they all mention, "community members" believe having those features makes readability worse.
readability is inherently subjective.

on, a finer note, being against the official process, isn't necessarily bad thing, It can be a good thing to show, how possibly broken an official process is. (I don't believe it is quite yet). my hope is that lint inclusion wasn't as bureaucratic as RFC process. however it is looking like it is.
thank you for reading all of my comments, I appreciate any thought you have put into it.

@oli-obk

This comment has been minimized.

Copy link
Collaborator

oli-obk commented May 23, 2018

bother to read the why

uhm... I literally read and categorized every lint description two months ago

The else if without else lint has an actual safety standard notice:

Some coding guidelines require this (e.g. MISRA-C:2004 Rule 14.10).

The shadow* lints say

  • Still, some may opt to avoid it in their code base
  • Still, some argue that name shadowing like this hurts readability, because a value may be bound to different things depending on position in the code.
  • Name shadowing can hurt readability, especially in large code bases, because it is easy to lose track of the active binding at any place in the code

If you prefer to have citations of coding guidelines or safety standards here, please open an issue or better yet a PR.

Our lints and their descriptions depend on community members maintaining them, this is not our dayjob.

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented May 23, 2018

The "why" in the codebase need not be the full set of reasons why. We're not great about documentation.

Shadowing actually is problematic, but not too problematic. Those in safety-critical environments may like it.

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.