don't make crazy suggestion for unreachable braced pub-use #50476
Conversation
rust-highfive
added
the
S-waiting-on-review
|
Could you instead still output the suggestion, but using _with_applicability and marking it PossiblyIncorrect? See baa7b32 for how this works. We still need to figure out a way to suggest in this case; edition lints should be automatable as much as possible. |
zackmdavis
force-pushed
the
tame_unreachable_pub_suggestion
branch
from
9561b45 to
0fd6c30
Compare
|
@Manishearth (updated)
I see; humans who see the bad
|
zackmdavis
force-pushed
the
tame_unreachable_pub_suggestion
branch
from
0fd6c30 to
891fc7a
Compare
|
(force-pushed again, had forgotten to update the commit message) |
|
@zackmdavis that is the eventual plan, for now we're going to use _with_applicability, and when everything is updated we rename the methods. |
|
@bors r+ I'd prefer something using span_to_snippet but I don't think there's a way to make that work better here. |
|
📌 Commit 891fc7a has been approved by |
bors
added
S-waiting-on-bors
|
☔ The latest upstream changes (presumably #50454) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
zackmdavis
force-pushed
the
tame_unreachable_pub_suggestion
branch
from
891fc7a to
d74636d
Compare
|
(updated) @Manishearth Rebase conflict was with the macro-context check you added in #50454—is there a specifically known case where macros mess up the suggestion, or was that generalized skepticism of the I-agree-very-regrettable span-chopping being OK in the presence of macros? (The revision I just pushed just takes my branch's side of the conflict, only doing |
|
Generalized skepticism. If rustfix isn't sure of something it shouldn't try
to autofix it (it can prompt instead)
…On Thu, May 10, 2018, 7:04 PM Zack M. Davis ***@***.***> wrote:
(updated)
@Manishearth <https://github.com/Manishearth> Rebase conflict was with the
macro-context check you added
<https://github.com/rust-lang/rust/blame/acd3871ba17316419c644e17547887787628ec2f/src/librustc_lint/builtin.rs#L1303>
in #50454 <#50454>—is there a
specifically known case where macros mess up the suggestion, or was that
generalized skepticism of the I-agree-very-regrettable span-chopping being
OK in the presence of macros? (The revision I just pushed just takes my
branch's side of the conflict, only doing MaybeIncorrect for use items.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#50476 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABivSBHjo5vgPLi2iRJ-ootrS6zW4ZfEks5txPGcgaJpZM4Tz517>
.
|
The Higher Intermediate Representation doesn't have spans for visibility keywords, so we were assuming that the first whitespace-delimited token in the item span was the `pub` to be weakened. This doesn't work for brace-grouped `use`s, which get lowered as if they were several individual `use` statements, but with spans that only cover the braced path-segments. Constructing a correct suggestion here presents some challenges—until someone works those out, we can at least protect the dignity of our compiler marking the suggestion for `use` items as potentially incorrect. This resolves rust-lang#50455 (but again, it would be desirable in the future to make a correct suggestion instead of copping out like this).
zackmdavis
force-pushed
the
tame_unreachable_pub_suggestion
branch
from
d74636d to
7006018
Compare
|
@bors: p=1 We'll want to land this pretty quickly for the edition preview, so raising priority |
|
@bors r+ |
|
📌 Commit 7006018 has been approved by |
bors
added
S-waiting-on-bors
…gestion, r=Manishearth don't make crazy suggestion for unreachable braced pub-use The Higher Intermediate Representation doesn't have spans for visibility keywords, so we were assuming that the first whitespace-delimited token in the item span was the `pub` to be weakened. This doesn't work for brace-grouped `use`s, which get lowered as if they were several individual `use` statements, but with spans that only cover the braced path-segments. Constructing a correct suggestion here presents some challenges—until someone works those out, we can at least protect the dignity of our compiler by not offering any suggestion at all for `use` items. This resolves rust-lang#50455 (but again, it would be desirable in the future to make a correct suggestion instead of copping out like this). r? @Manishearth
|
⌛ Testing commit 7006018 with merge 3b946e637fb5d314379cbc620ddc3000994520fb... |
|
@bors: p=0 Moving back to a normal priority |
|
@zackmdavis I noticed that the fixes here were centered around the |
…Manishearth don't make crazy suggestion for unreachable braced pub-use The Higher Intermediate Representation doesn't have spans for visibility keywords, so we were assuming that the first whitespace-delimited token in the item span was the `pub` to be weakened. This doesn't work for brace-grouped `use`s, which get lowered as if they were several individual `use` statements, but with spans that only cover the braced path-segments. Constructing a correct suggestion here presents some challenges—until someone works those out, we can at least protect the dignity of our compiler by not offering any suggestion at all for `use` items. This resolves #50455 (but again, it would be desirable in the future to make a correct suggestion instead of copping out like this). r? @Manishearth
|
☀️ Test successful - status-appveyor, status-travis |
|
(um, probably a future PR) |
This is a true fix for rust-lang#50455, superior to the mere bandage offered in rust-lang#50476.
…petrochenkov add modifier keyword spans to hir::Visibility; improve unreachable-pub, private-no-mangle lint suggestions #50455 pointed out that the unreachable-pub suggestion for brace-grouped `use`s was bogus; #50476 partially ameliorated this by marking the suggestion as `Applicability::MaybeIncorrect`, but this is the actual fix. Meanwhile, another application of having spans available in `hir::Visibility` is found in the private-no-mangle lints, where we can now issue a suggestion to use `pub` if the item has a more restricted visibility marker (this seems much less likely to come up in practice than not having any visibility keyword at all, but thoroughness is a virtue). While we're there, we can also add a helpful note if the item does have a `pub` (but triggered the lint presumably because enclosing modules were private).  r? @nrc cc @Manishearth
The Higher Intermediate Representation doesn't have spans for visibility
keywords, so we were assuming that the first whitespace-delimited token
in the item span was the
pubto be weakened. This doesn't work forbrace-grouped
uses, which get lowered as if they were severalindividual
usestatements, but with spans that only cover the bracedpath-segments. Constructing a correct suggestion here presents some
challenges—until someone works those out, we can at least protect the
dignity of our compiler by not offering any suggestion at all for
useitems.This resolves #50455 (but again, it would be desirable in the future to
make a correct suggestion instead of copping out like this).
r? @Manishearth