Skip to content

Emit a lint for unused assign to temporary variables #71681

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Emit a lint for unused assign to temporary variables #71681

wants to merge 2 commits into from

Conversation

LeSeulArtichaut
Copy link
Contributor

Resolves partially #16789.

r? @estebank cc @Dylan-DPC

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 29, 2020
@LeSeulArtichaut LeSeulArtichaut force-pushed the lint-unused-assign branch 3 times, most recently from 1eb0a15 to d1a2097 Compare April 29, 2020 19:37
estebank
estebank approved these changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mark-i-m
Copy link
Member

It feels odd that assignment to a literal expression is allowed at all...

@LeSeulArtichaut
Copy link
Contributor Author

@mark-i-m As Jonas Schievink pointed out in #68713 (comment), a field is a valid place expression. This wouldn't work if there wasn't the field access.

This means the following code is invalid and gives you an "invalid left-hand side of assignment" error:

Foo { x: false } = Foo { x: true };

But if you have a field access, the code is valid, though still not useful.

Foo { x: false }.x = true;

@LeSeulArtichaut
Copy link
Contributor Author

@estebank I tweaked the diagnostics according to your review. I didn't understand what you meant with "structured_suggestion" though, so I tried something 😅

LeSeulArtichaut
LeSeulArtichaut commented Apr 30, 2020
View reviewed changes
src/librustc_typeck/check/expr.rs
Comment on lines +835 to +843
let mut path_str = String::new();
for (i, seg) in segments.iter().enumerate() {
if i > 0 {
path_str.push_str("::");
}
if seg.ident.name != kw::PathRoot {
path_str.push_str(&seg.ident.as_str());
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any existing function that does this? I tried to search but didn't find anything but in rustdoc (which I... err... copy-pasted 😄)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the hir::Path itself already printable? You can use @ to bind the entire path while also keeping the filtering and reference to segments.

estebank
estebank reviewed Apr 30, 2020
View reviewed changes
src/test/ui/unused/unused-field-assign.stderr
| --- ^
| |
| this is not bound to any variable
| help: consider introducing a local variable: `let mut foo = FOO;`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause the final code applied by rustfix to be let mut foo = FOO;.x = 2;, which is incorrect.

estebank
estebank reviewed Apr 30, 2020
View reviewed changes
src/librustc_typeck/check/expr.rs
lint.build("unused assignement to temporary")
.span_label(lhs.span, "this is not bound to any variable")
.note("in Rust, constants are not associated with a specific memory location, and are inlined wherever they are used")
.span_suggestion(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this to be span_suggestion_verbose? This will make the suggestion not be a label, but show you the final code instead.

@crlf0710
Copy link
Member

crlf0710 commented May 8, 2020

@LeSeulArtichaut Ping from triage, could you address the comments above? thanks.

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2020
@LeSeulArtichaut
Copy link
Contributor Author

@estebank I haven't found a way to have the suggestion for introducing a local variable. Would that be OK to leave a FIXME and merge this without the suggestion?

@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2020
@Elinvynia Elinvynia added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2020
@Elinvynia Elinvynia added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2020
@Dylan-DPC-zz
Copy link

Had a discussion with the author, closing this due to inactivity and we will revisit it later when there's a better idea about this.

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank approved these changes

Assignees

@estebank estebank

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@LeSeulArtichaut @mark-i-m @crlf0710 @Dylan-DPC-zz @estebank @rust-highfive @joelpalmer @Elinvynia