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

Don't expand macros in some suggestions #3366

Merged
merged 3 commits into from Oct 28, 2018

Conversation

Projects
None yet
2 participants
@phansch
Collaborator

phansch commented Oct 27, 2018

Fixes #1148
Fixes #1628
Fixes #2455
Fixes #3023
Fixes #3333
Fixes #3360

@@ -72,7 +72,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IdentityConversion {
let a = cx.tables.expr_ty(e);
let b = cx.tables.expr_ty(&args[0]);
if same_tys(cx, a, b) {
let sugg = snippet(cx, args[0].span, "<expr>").into_owned();
let sugg = if in_macro(args[0].span) {
snippet(cx, args[0].span.source_callsite(), "<expr>").into_owned()

This comment has been minimized.

@oli-obk

oli-obk Oct 27, 2018

Collaborator

Can't we unconditionally call the source callsite methods? if so, can we move it into the snippet function so all lints benefit from it?

This comment has been minimized.

@phansch

phansch Oct 27, 2018

Collaborator

Yea, I was thinking that this is probably becoming a recurring pattern. I will give it a try!

This comment has been minimized.

@phansch

phansch Oct 27, 2018

Collaborator

One problem occurs when span.source_callsite() is called on a format! argument. This happens in useless_format lint (and a few other lints):

https://github.com/rust-lang-nursery/rust-clippy/blob/457e7f12e9fc028eae182f23b279194e5344b676/clippy_lints/src/format.rs#L78-L79

Unconditionally calling source_callsite will result in the wrong suggestion because the source_callsite exists and is the format! macro in this case:

help: consider using .to_string(): `format!("foo");.to_string()`

I think it still makes sense to have a separate snippet_with_macro_callsite function for the other cases, though.

This comment has been minimized.

@oli-obk

oli-obk Oct 27, 2018

Collaborator

Makes sense. This generally only works if the entire situation is outside a macro, but most of our lints are skipping those situations via in_macro on the full span anyway.

@phansch phansch changed the title from Don't expand macro in identity_conversion suggestion to Don't expand some macros in suggestions Oct 27, 2018

@phansch phansch changed the title from Don't expand some macros in suggestions to WIP: Don't expand some macros in suggestions Oct 27, 2018

@phansch phansch changed the title from WIP: Don't expand some macros in suggestions to Don't expand macros in some suggestions Oct 27, 2018

@phansch phansch force-pushed the phansch:behebt_problem_3360 branch from d853b66 to 2be39c0 Oct 27, 2018

@phansch

This comment has been minimized.

Collaborator

phansch commented Oct 27, 2018

I added a separate function called snippet_with_macro_callsite that can be used when it's clear that the input span is not a macro argument. I'm.. not sure if there is a better way. At least this way there are fewer conditionals around.

I will also go through all the other issues and try to find more cases that can be fixed easily.

edit: Ok, I can't find any more issues that will be fixed by this.

phansch added some commits Oct 27, 2018

@phansch phansch force-pushed the phansch:behebt_problem_3360 branch from bde2f49 to 840e50e Oct 27, 2018

@oli-obk

This comment has been minimized.

Collaborator

oli-obk commented Oct 27, 2018

bors r+

bors bot added a commit that referenced this pull request Oct 27, 2018

Merge #3366
3366: Don't expand macros in some suggestions r=oli-obk a=phansch

Fixes #1148 
Fixes #1628
Fixes #2455
Fixes #3023
Fixes #3333
Fixes #3360

Co-authored-by: Philipp Hansch <dev@phansch.net>
@bors

This comment has been minimized.

Contributor

bors bot commented Oct 27, 2018

@phansch

This comment has been minimized.

Collaborator

phansch commented Oct 28, 2018

bors r=oli-obk

bors bot added a commit that referenced this pull request Oct 28, 2018

Merge #3217 #3366
3217: Fix string_lit_as_bytes lint for macros r=phansch a=yaahallo

Prior to this change, string_lit_as_bytes would trigger for constructs
like `include_str!("filename").as_bytes()` and would recommend fixing it
by rewriting as `binclude_str!("filename")`.

This change updates the lint to act as an EarlyLintPass lint. It then
differentiates between string literals and macros that have bytes
yielding alternatives.

Closes #3205

3366: Don't expand macros in some suggestions r=oli-obk a=phansch

Fixes #1148 
Fixes #1628
Fixes #2455
Fixes #3023
Fixes #3333
Fixes #3360

Co-authored-by: Jane Lusby <jlusby42@gmail.com>
Co-authored-by: Philipp Hansch <dev@phansch.net>
@bors

This comment has been minimized.

Contributor

bors bot commented Oct 28, 2018

@bors bors bot merged commit 840e50e into rust-lang:master Oct 28, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
bors Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@phansch phansch deleted the phansch:behebt_problem_3360 branch Oct 28, 2018

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