extend unused_parens lint to break/return ()

[llogiq]

No description provided.

[rust-highfive]

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

[joshtriplett]

Please don't include unrelated formatting/ordering changes like these in commits.

[joshtriplett]

Seems like a good idea. However, it's difficult to review the change when the commit includes numerous unrelated formatting changes. Please don't do that. Formatting changes should go in a separate commit.

[llogiq]

I have removed the formatting changes for now, and tried to fix the tests instead. Let's keep formatting out of this.

[joshtriplett]

This still seems to be unrelated to this change. (It's a good change, but not related to this commit.)

[llogiq]

OK, I backed this out.

[joshtriplett]

Thanks! (Please do submit this cleanup separately.)

[joshtriplett]

Looks much better now. I commented on one remaining nit; with that fixed, LGTM.

[scottmcm]

This seems qualitatively different from the other situations that this lint covers, because from a grammar perspective these aren't parens, they're a unit expression.

If we're going to have this (which I'm not at all opposed to), then it feels to me like it's a different lint (maybe unecessary_unit) that would also do things like

fn foo() { () }
           ^^ help: remove this unit value

Edit: Would probably cover things like this, too:

fn bar() -> () {}
        ^^^^^^ help: remove this unit value

[joshtriplett]

@scottmcm That's a fair point. Same check, different lint name?

[llogiq]

I'm OK with that. Still I remember that we usually don't add new lints to rustc, unless for specific issues like forwards compatibility.

cc @Manishearth

[Manishearth]

I'm okay with this but I'm not sure if it's my decision to make.

[llogiq]

In that case I'll make it a new lint and we start a FCP. If it doesn't get merged, we can still add it to clippy.

[Manishearth]

I think it makes more sense to extend the existing lint actually. It fits within the description pretty well, I'm actually surprised this lint doesn't handle this already.

Edit: I misunderstood what this change does, it doesn't make as much sense to be part of the same lint now. Still could be though. I don't feel strongly on this either way.

[llogiq]

@Manishearth judgement call; I could well find arguments for extending unused_paren or creating a new lint. Anyway, it's a good idea to also cover () in return type position.

[joshtriplett]

So, some thoughts on this:

• I do agree that we should also lint on () in return position, but let's not delay implementing this check just because we could potentially extend it with further checks.

• I can see arguments for making this a separate lint, since it involves tuples rather than just parenthesizing. I can also see arguments for putting this under unused_parens, since that would mean people changing the disposition of that lint will also affect this, and since it does seem somewhat related. I don't feel strongly either way, but I'd like to avoid bikeshedding too much. Since we have a patch proposed, I'd suggest we start with "extend the existing lint" for now.

• I don't know if this needs a full FCP to merge, but I'm going to go ahead and start one to seek consensus. For my part, I believe we should make this change.

@rfcbot fcp merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Centril
• @aturon
• @cramertj
• @eddyb
• @joshtriplett
• @nikomatsakis
• @nrc
• @pnkfelix
• @scottmcm
• @withoutboats

Concerns:

• bake-in-clippy (extend unused_parens lint to break/return () #54440 (comment))
• should-be-a-separate-lint (extend unused_parens lint to break/return () #54440 (comment))

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Centril]

I agree with @scottmcm about the lint name here and would add that I find this behavior coupled with the name unused_parens to be actively misleading in a way that hurts learning.

@rfcbot concern should-be-a-separate-lint

I do agree that we should also lint on () in return position, but let's not delay implementing this check just because we could potentially extend it with further checks.

I don't feel it needs to happen quickly since this is about style and not correctness; return () is not such a bad thing to do for those who want to be more explicit about what is happening. So I'd suggest that this could bake in clippy first (perhaps in the style category).

@rfcbot concern bake-in-clippy

As for linting on -> () I'm not so sure I would want that; While fn foo() { .. } is convenient; I've never been too pleased with this corner case in the language and I think that it is fine to write -> (). Maybe rustfmt should format it away (which it doesn't right now); but I don't feel it rises to the level of a compiler lint at the moment.

I don't know if this needs a full FCP to merge

Since I think this is essentially a new lint that is different from what unused_parens does currently, I do think that this constitute a lang change and our policy around that is that it needs an RFC. I feel we have been lax in the department of requiring RFCs and I would like us to be a bit more strict moving forward...

[Manishearth]

I feel we have been lax in the department of requiring RFCs and I would like us to be a bit more strict moving forward...

I've said this before: That part of the policy has not really been exercised before (and AIUI it wasn't added with an RFC), I think we really should get an explicit RFC reestablishing that policy or something else in its place. The governance landscape has changed significantly since then: There is a style WG, a rustfmt subteam, and a clippy sub-WG ( eventually subteam) as well.

I find lints not much different from documentation when it comes to their relationship with language design.

[scottmcm]

Is this the kind of thing that // run-rustfix is for? Checking the full JSON feels fragile...

[llogiq]

Cool, I didn't know about run-rustfix! I mainly wanted to check the applicability, so this should work nicely.

[petrochenkov]

The implementation looks good to me, relabeling as waiting-on-team.

[ghost]

I disagree with this lint. Here's my reasoning:

• Explicitly using the unit type is never going to do the wrong thing. At worst it's confusing because it looks like you can use break and return as functions, however the same applies to any other tuples. break(1, 2, 3) looks just as much like a function.
• Macros benefit immensely from being able to generalise over return types and values. I.e. having one syntax for all functions rather than one syntax for X type and another for Y type. Functions are therefore just fn name() -> $ty { $tyval } rather than having to special case () or adding #[allow] so as to avoid tripping a lint.

The lint therefore provides zero tangible benefit (or at least, none has been given here in this thread) at the expense of making macros more annoying.

[llogiq]

@whataloadofwhat

• Your "... is never going to do the wrong thing" argument also applies to the rest of the unused_parens lint. Are you suggesting we should remove that, too? Good point on the possible formatting ambiguity, though I suspect rustfmt should clear it up; perhaps we should create a clippy issue for this? I don't see it fitting in the realm of compiler lints.
• Also this lint has a macro check so it won't even fire on macro-generated code. This check may be a bit easy to overlook, perhaps I should add a test case that makes it clear?

[llogiq]

Also it should be easy to extend the lint to cover () in block return statment position, and it should be possible to also cover return types (though I'm not sure how to reliably get the correct span for -> ()).

[ghost]

If the lint won't trigger for macros then I retract my objection. Apologies.

[llogiq]

@whataloadofwhat it's all good, I can improve the code based on your input.

[bors]

☔ The latest upstream changes (presumably #54453) made this pull request unmergeable. Please resolve the merge conflicts.

[scottmcm]

I agree with the already-raised concerns here.

I feel like explicit-() is a stylistic choice which would likely be considered valuable by those who want to reinforce the expressionist nature of rust. It feels like it's in the same philosophy question of whether to write fn foo() { bar() } or fn foo() { bar(); } (assuming fn bar();) -- or even, I suppose, fn foo() -> () { bar() }, though probably not fn foo() -> () { bar(); }.

And that seems completely separate from excessively-parenthesizing expressions, even though the unicode codepoints involved are the same.

[withoutboats]

We discussed this in the lang team meeting and several of us don't think this belongs under the unused_parens lint, which is for wrapping expressions in unnecessary parens, whereas this is an unnecessary tuple expression.

Some of us also preferred that a new lint bake in clippy before adding it to rustc.

[llogiq]

@withoutboats in that case I'll close this here and put the code in a clippy PR. Thanks for the review, folks!