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

Warn when assigning a block that doesn't have an implicit return #44881

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@estebank
Contributor

estebank commented Sep 27, 2017

Fix #44173:

warning: assinging result of block that evaluates to `()`
  --> $DIR/assigning-block.rs:13:13
   |
13 |       let x = {
   |  _____________^
14 | |         println!("foo");
15 | |         "bar";
   | |              - help: consider removing this semicolon
16 | |     };
   | |_____^
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Sep 27, 2017

Collaborator

r? @pnkfelix

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

Collaborator

rust-highfive commented Sep 27, 2017

r? @pnkfelix

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

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Sep 27, 2017

Contributor

r? @petrochenkov

I'm not sure the parser is a best place to place this lint. Maybe use the early lint pass? Now that lints are emitted eagerly, it should work

Contributor

arielb1 commented Sep 27, 2017

r? @petrochenkov

I'm not sure the parser is a best place to place this lint. Maybe use the early lint pass? Now that lints are emitted eagerly, it should work

@zackmdavis

This comment has been minimized.

Show comment
Hide comment
@zackmdavis

zackmdavis Sep 27, 2017

Member

What is our policy on hardcoded warnings that aren't lints (and therefore configurable with #[allow(...)], &c.)? However useless it may be, if let x = { "bar"; }; isn't an error, it should be possible to compile it without a warning (even if most the vast majority of users would want the warning).

Member

zackmdavis commented Sep 27, 2017

What is our policy on hardcoded warnings that aren't lints (and therefore configurable with #[allow(...)], &c.)? However useless it may be, if let x = { "bar"; }; isn't an error, it should be possible to compile it without a warning (even if most the vast majority of users would want the warning).

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 30, 2017

Contributor

This looks like a Clippy lint material (and certainly not something requiring a hard-coded warning in rustc).
I'd rather close #44173 as WONTFIX and open an equivalent issue in the Clippy repo.

Contributor

petrochenkov commented Sep 30, 2017

This looks like a Clippy lint material (and certainly not something requiring a hard-coded warning in rustc).
I'd rather close #44173 as WONTFIX and open an equivalent issue in the Clippy repo.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 30, 2017

Contributor

If the goal is to give better diagnostics in cases where type error happens (the original example in #44173), then it's probably better attach some extra note to that error if the undesired () originates from { ...; something; } expression.

Contributor

petrochenkov commented Sep 30, 2017

If the goal is to give better diagnostics in cases where type error happens (the original example in #44173), then it's probably better attach some extra note to that error if the undesired () originates from { ...; something; } expression.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Oct 5, 2017

Member

I agree with @petrochenkov 's comments.

@estebank : do you want to try to revise this PR to be something more like @petrochenkov 's second suggestion ? Or should I just close this PR?

Member

pnkfelix commented Oct 5, 2017

I agree with @petrochenkov 's comments.

@estebank : do you want to try to revise this PR to be something more like @petrochenkov 's second suggestion ? Or should I just close this PR?

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Oct 5, 2017

Contributor

@pnkfelix I'll rework it, but it'll probably not be before next week due to limited availability.

Contributor

estebank commented Oct 5, 2017

@pnkfelix I'll rework it, but it'll probably not be before next week due to limited availability.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 8, 2017

Contributor

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

Contributor

bors commented Oct 8, 2017

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Oct 19, 2017

Member

I'm going to close this now due to inactivity to help clear out the queue, but please feel free to resubmit with a rebase!

Member

alexcrichton commented Oct 19, 2017

I'm going to close this now due to inactivity to help clear out the queue, but please feel free to resubmit with a rebase!

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