Skip to content

Improve placement of use suggestions #43929

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

Merged
merged 2 commits into from
Aug 21, 2017
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Aug 17, 2017

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.

Love it! The new locations are exactly where they should be. The only wart I see is that by applying these suggestions without any smarts the resulting output would be the following:

use a::A;
struct S;

I don't think we should change the compiler output, I think clients could perform local reformatting to comply with rustfmt instead.

let def_id = this.current_module.normal_ancestor_id;
let node_id = this.definitions.as_local_node_id(def_id).unwrap();
let better = def.is_some();
this.use_injections.push(UseError{ err, candidates, node_id, better });
Copy link
Contributor

Choose a reason for hiding this comment

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

Space between UseError and {?

_: Span,
_: &[ast::Attribute],
node_id: NodeId,
) {
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 following slightly easier to follow?

    fn visit_mod(&mut self,
                 module: &'tcx ast::Mod,
                 _: Span,
                 _: &[ast::Attribute],
                 node_id: NodeId) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rustfmt has my version as the new style and I prefer it, too, but I can change it to the other style to fit more into what's still more prominent in rustc.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 17, 2017

The only wart I see is that by applying these suggestions without any smarts the resulting output would be the following:

use a::A;
struct S;

I don't think we should change the compiler output, I think clients could perform local reformatting to comply with rustfmt instead.

I'm confused... so you want the final output to be use a::A;struct S;?

@estebank
Copy link
Contributor

estebank commented Aug 17, 2017

I'd want the final output to be

use a::A;

struct S;

But wouldn't change rustc for it, as it makes the cli output worse.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 17, 2017

But wouldn't change rustc for it, as it makes the cli output worse.

Oh I already have a hack that makes sure the cli is fine ;) Adding another newline to the suggestion output in case the following item is not a use would definitely be possible.

@nrc
Copy link
Member

nrc commented Aug 21, 2017

@bors: r+

@bors
Copy link
Collaborator

bors commented Aug 21, 2017

📌 Commit 8f56322 has been approved by nrc

@bors
Copy link
Collaborator

bors commented Aug 21, 2017

⌛ Testing commit 8f56322 with merge 06bf94a...

bors added a commit that referenced this pull request Aug 21, 2017
Improve placement of `use` suggestions

r? @nrc

cc @estebank @Mark-Simulacrum

fixes #42835
fixes #42548
fixes #43769
@bors
Copy link
Collaborator

bors commented Aug 21, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 06bf94a to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants