don't suggest placing use statements into expanded code
#44215
Conversation
…inside expansions
shepmaster
added
the
S-waiting-on-author
|
I need a decision on how to continue here.
|
|
These things happen, because we are not producing help messages telling you what to insert, but we are producing suggestions, which contain a span that is supposed to be replaced. The human readable interface then applies the replacement and prints it. If the span is bogus, we get things like these. Figuring out where to place things can be difficult. Much more than I anticipated when I did the original change. But doing this is the right thing imo, because it will allow IDEs to enable users to insert missing import statements with a click |
What do you mean by that? |
Ah, I see. So, the span for import suggestions is not span of |
We could differentiate between insertion suggestions and replacement suggestions by not showing surroundings for insertion suggestions (at least if they insert a whole new line) |
|
We can also merge this (it will solve the visual bugs, but not the structured placement of the suggestions), and fix #44215 (comment) later |
|
r? @arielb1 irc discussion seems to suggest some urgency on this. I can do changes until tuesday, then I'm on vacation. My suggestion is to merge it as it is, and fix the structured json output later (the spans are wrong, the command line output can now never be wrong again barring missing expansion info on spans) |
|
I will be on vacation starting now, so if there are any change requests, I won't be able to do anything about them until Rustfest. The travis failure is bogus (not in the travis builder). |
|
r? @nrc I don't know this area of the code |
|
This is, however, an annoying beta regression. The beta deadline is less than 2 weeks from now. It would be nice to see what can be done to get this fixed for beta. |
|
Yeah, this fix looks good to me, we can address making the IDE case perfect later, but this is already much better than before. |
|
@bors: r+ |
|
📋 Looks like this PR is still in progress, ignoring approval |
oli-obk
changed the title
use statements into expanded codeuse statements into expanded code
|
Removed WIP from title |
|
@bors r=nrc |
|
📌 Commit f0e7a5b has been approved by |
nikomatsakis
added
the
beta-nominated
|
Nominating for backport as this fixes a stable-to-beta regression (#44566) |
nikomatsakis
added
T-compiler
don't suggest placing `use` statements into expanded code r? @nrc fixes #44210 ```rust #[derive(Debug)] struct Foo; type X = Path; ``` will try to place `use std::path::Path;` between `#[derive(Debug)]` and `struct Foo;` I am not sure how to obtain a span before the first attribute, because derive attributes are removed during expansion. It would be trivial to detect this case and place the `use` after the item, but that would be somewhat weird I think.
|
☀️ Test successful - status-appveyor, status-travis |
|
@oli-obk or @nikomatsakis would y'all be willing to help out with the backport here? Apparentlythis doesn't apply cleanly |
[beta] Backport accepted PRs to 1.21 Backport of: - ~don't suggest placing `use` statements into expanded code #44215 - stabilize tcpstream_connect_timeout #44563 - stabilized iterator_for_each #44567 - travis: Move sccache to the us-west-1 region #44574 - stabilized ord_max_min #44593 - stabilized compiler_fences #44595 - ci: Upload/download from a new S3 bucket #44617 - stabilized needs_drop #44639 - Stabilized vec_splice and modified splice tracking issue #44640 - Backport libs stabilizations to 1.21 beta #44824
Backport of #44215 Backport of #44215. r? @alexcrichton cc @nikomatsakis
alexcrichton
removed
the
beta-nominated
Fix use placement for suggestions near main. This fixes an edge case for the suggestion to add a `use`. When running with `--test`, the `main` function will be annotated with an `#[allow(dead_code)]` attribute. The `UsePlacementFinder` would end up using the dummy span of that synthetic attribute. If there are top-level inner attributes, this would place the `use` in the wrong position. The solution here is to ignore attributes with dummy spans. In the process of working on this, I discovered that the `use_suggestion_placement` test was broken. `UsePlacementFinder` is unaware of active attributes. Attributes like `#[derive]` don't exist in the AST since they are removed. Fixing that is difficult, since the AST does not retain enough information. I considered trying to place the `use` towards the top of the module after any `extern crate` items, but I couldn't find a way to get a span for the start of a module block (the `mod` span starts at the `mod` keyword, and it seems tricky to find the spot just after the opening bracket and past inner attributes). For now, I just put some comments about the issue. This appears to have been a known issue in rust-lang#44215 where the test for it was introduced, and the fix seemed to be deferred to later.
r? @nrc
fixes #44210
will try to place
use std::path::Path;between#[derive(Debug)]andstruct Foo;I am not sure how to obtain a span before the first attribute, because derive attributes are removed during expansion.
It would be trivial to detect this case and place the
useafter the item, but that would be somewhat weird I think.