Skip to content

rustfmt librustc_resolve #34584

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

Closed
wants to merge 1 commit into from
Closed

rustfmt librustc_resolve #34584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 79 additions & 51 deletions src/librustc_resolve/build_reduced_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,21 @@ trait ToNameBinding<'a> {

impl<'a> ToNameBinding<'a> for (Module<'a>, Span, ty::Visibility) {
fn to_name_binding(self) -> NameBinding<'a> {
NameBinding { kind: NameBindingKind::Module(self.0), span: self.1, vis: self.2 }
NameBinding {
kind: NameBindingKind::Module(self.0),
span: self.1,
vis: self.2,
}
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

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

Struct expression fits into one line but splitted into several lines anyway (1).

Copy link
Member Author

@tshepang tshepang Jul 3, 2016

Choose a reason for hiding this comment

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

I find this more readable

}
}

impl<'a> ToNameBinding<'a> for (Def, Span, ty::Visibility) {
fn to_name_binding(self) -> NameBinding<'a> {
NameBinding { kind: NameBindingKind::Def(self.0), span: self.1, vis: self.2 }
NameBinding {
kind: NameBindingKind::Def(self.0),
span: self.1,
vis: self.2,
}
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

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

(1) again.

}
}

Expand Down Expand Up @@ -108,20 +116,20 @@ impl<'b> Resolver<'b> {
let module_path: Vec<Name> = match view_path.node {
ViewPathSimple(_, ref full_path) => {
full_path.segments
.split_last()
.unwrap()
.1
.iter()
.map(|seg| seg.identifier.name)
.collect()
.split_last()
.unwrap()
.1
.iter()
.map(|seg| seg.identifier.name)
.collect()
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The indentation is incorrect (indentation to the dot vs one tab indentation) (2).

Copy link
Contributor

@jseyfried jseyfried Jul 3, 2016

Choose a reason for hiding this comment

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

I actually don't mind the block indentation here. Then again, I almost always prefer block indenting to visual indenting since visual indenting causes rightward drift, bad diffs, and (imo) ugliness.

That being said, I'd probably format this

full_path.segments.split_last().unwrap().1.iter().map(|seg| {
    seg.identifier.name
}).collect()

Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

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

I usually use

chain.chain.chain.map(|args| {
    closure_body
}).collect()

form too if chain.chain.chain fits into one line.

}

ViewPathGlob(ref module_ident_path) |
ViewPathList(ref module_ident_path, _) => {
module_ident_path.segments
.iter()
.map(|seg| seg.identifier.name)
.collect()
.iter()
.map(|seg| seg.identifier.name)
.collect()
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

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

(2) again.

Copy link
Member Author

@tshepang tshepang Jul 3, 2016

Choose a reason for hiding this comment

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

Advantage here is that, if the binding name length changes, those methods would not need re-indenting

}
};

Expand All @@ -144,12 +152,14 @@ impl<'b> Resolver<'b> {
}
ViewPathList(_, ref source_items) => {
// Make sure there's at most one `mod` import in the list.
let mod_spans = source_items.iter().filter_map(|item| {
match item.node {
PathListItemKind::Mod { .. } => Some(item.span),
_ => None,
}
}).collect::<Vec<Span>>();
let mod_spans = source_items.iter()
.filter_map(|item| {
match item.node {
PathListItemKind::Mod { .. } => Some(item.span),
_ => None,
}
})
.collect::<Vec<Span>>();
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

.filter_map(|item| { is moved one line down despite fitting into the line. (3)

Copy link
Contributor

@jseyfried jseyfried Jul 3, 2016

Choose a reason for hiding this comment

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

I strongly prefer the original formatting here.


if mod_spans.len() > 1 {
let mut e = resolve_struct_error(self,
Expand All @@ -163,8 +173,9 @@ impl<'b> Resolver<'b> {

for source_item in source_items {
let (module_path, name, rename) = match source_item.node {
PathListItemKind::Ident { name, rename, .. } =>
(module_path.clone(), name.name, rename.unwrap_or(name).name),
PathListItemKind::Ident { name, rename, .. } => {
(module_path.clone(), name.name, rename.unwrap_or(name).name)
}
PathListItemKind::Mod { rename, .. } => {
let name = match module_path.last() {
Some(name) => *name,
Expand Down Expand Up @@ -221,7 +232,7 @@ impl<'b> Resolver<'b> {
let module = self.new_module(parent_link, Some(def), false);
module.no_implicit_prelude.set({
parent.no_implicit_prelude.get() ||
attr::contains_name(&item.attrs, "no_implicit_prelude")
attr::contains_name(&item.attrs, "no_implicit_prelude")
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

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

It's hard to say why... but I feel that the tab was correct here.

});
self.define(parent, name, TypeNS, (module, sp, vis));
self.module_map.insert(item.id, module);
Expand Down Expand Up @@ -277,16 +288,22 @@ impl<'b> Resolver<'b> {
}

// Record the def ID and fields of this struct.
let field_names = struct_def.fields().iter().enumerate().map(|(index, field)| {
self.resolve_visibility(&field.vis);
field.ident.map(|ident| ident.name)
.unwrap_or_else(|| token::intern(&index.to_string()))
}).collect();
let field_names = struct_def.fields()
.iter()
.enumerate()
.map(|(index, field)| {
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

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

(3) again

self.resolve_visibility(&field.vis);
field.ident
.map(|ident| ident.name)
.unwrap_or_else(|| token::intern(&index.to_string()))
})
.collect();
let item_def_id = self.definitions.local_def_id(item.id);
self.structs.insert(item_def_id, field_names);
}

ItemKind::DefaultImpl(_, _) | ItemKind::Impl(..) => {}
ItemKind::DefaultImpl(_, _) |
ItemKind::Impl(..) => {}

ItemKind::Trait(_, _, _, ref items) => {
let def_id = self.definitions.local_def_id(item.id);
Expand Down Expand Up @@ -336,7 +353,8 @@ impl<'b> Resolver<'b> {

// Variants are always treated as importable to allow them to be glob used.
// All variants are defined in both type and value namespaces as future-proofing.
let def = Def::Variant(item_id, self.definitions.local_def_id(variant.node.data.id()));
let def = Def::Variant(item_id,
self.definitions.local_def_id(variant.node.data.id()));
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

What.
Does rustfmt use different line length settings than rustc code? (4)

self.define(parent, name, ValueNS, (def, variant.span, vis));
self.define(parent, name, TypeNS, (def, variant.span, vis));
}
Expand All @@ -348,9 +366,7 @@ impl<'b> Resolver<'b> {
let name = foreign_item.ident.name;

let def = match foreign_item.node {
ForeignItemKind::Fn(..) => {
Def::Fn(self.definitions.local_def_id(foreign_item.id))
}
ForeignItemKind::Fn(..) => Def::Fn(self.definitions.local_def_id(foreign_item.id)),
ForeignItemKind::Static(_, m) => {
Def::Static(self.definitions.local_def_id(foreign_item.id), m)
}
Expand All @@ -376,7 +392,9 @@ impl<'b> Resolver<'b> {
}

/// Builds the reduced graph for a single item in an external crate.
fn build_reduced_graph_for_external_crate_def(&mut self, parent: Module<'b>, xcdef: ChildItem) {
fn build_reduced_graph_for_external_crate_def(&mut self,
parent: Module<'b>,
xcdef: ChildItem) {
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

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

(4) again

let def = match xcdef.def {
DlDef(def) => def,
_ => return,
Expand All @@ -391,18 +409,26 @@ impl<'b> Resolver<'b> {
}

let name = xcdef.name;
let vis = if parent.is_trait() { ty::Visibility::Public } else { xcdef.vis };
let vis = if parent.is_trait() {
ty::Visibility::Public
} else {
xcdef.vis
};
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

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

"Conditional operator" in Rust is already noisy as hell compared to C's ?: and rustfmt makes it even worse. (5)

Copy link
Member Author

@tshepang tshepang Jul 3, 2016

Choose a reason for hiding this comment

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

I find this more readable


match def {
Def::Mod(_) | Def::ForeignMod(_) | Def::Enum(..) => {
Def::Mod(_) |
Def::ForeignMod(_) |
Def::Enum(..) => {
debug!("(building reduced graph for external crate) building module {} {:?}",
name, vis);
name,
vis);
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

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

This hurts my aesthetic feelings. The line lenghts are... so different.

Copy link
Contributor

@jseyfried jseyfried Jul 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, I think it far more readable and aesthetically pleasing to format debug!s with long format strings as

debug!("x: {}, y: {}, z: {}, ... (long line) ...",
       x, y, z);

rather than

debug!("x: {}, y: {}, x: {}, ... (long line) ...",
       x,
       y,
       z);

Copy link
Member Author

@tshepang tshepang Jul 3, 2016

Choose a reason for hiding this comment

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

yeah, it's also more consistent

let parent_link = ModuleParentLink(parent, name);
let module = self.new_module(parent_link, Some(def), true);
self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis));
}
Def::Variant(_, variant_id) => {
debug!("(building reduced graph for external crate) building variant {}", name);
debug!("(building reduced graph for external crate) building variant {}",
name);
Copy link
Contributor

@petrochenkov petrochenkov Jul 3, 2016

Choose a reason for hiding this comment

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

(4) again, there are many of these below.

// Variants are always treated as importable to allow them to be glob used.
// All variants are defined in both type and value namespaces as future-proofing.
self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis));
Expand All @@ -422,15 +448,15 @@ impl<'b> Resolver<'b> {
self.try_define(parent, name, ValueNS, (def, DUMMY_SP, vis));
}
Def::Trait(def_id) => {
debug!("(building reduced graph for external crate) building type {}", name);
debug!("(building reduced graph for external crate) building type {}",
name);

// If this is a trait, add all the trait item names to the trait
// info.

let trait_item_def_ids = self.session.cstore.trait_item_def_ids(def_id);
for trait_item_def in &trait_item_def_ids {
let trait_item_name =
self.session.cstore.item_name(trait_item_def.def_id());
let trait_item_name = self.session.cstore.item_name(trait_item_def.def_id());

debug!("(building reduced graph for external crate) ... adding trait item \
'{}'",
Expand All @@ -443,13 +469,18 @@ impl<'b> Resolver<'b> {
let module = self.new_module(parent_link, Some(def), true);
self.try_define(parent, name, TypeNS, (module, DUMMY_SP, vis));
}
Def::TyAlias(..) | Def::AssociatedTy(..) => {
debug!("(building reduced graph for external crate) building type {}", name);
Def::TyAlias(..) |
Def::AssociatedTy(..) => {
debug!("(building reduced graph for external crate) building type {}",
name);
self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis));
}
Def::Struct(def_id)
if self.session.cstore.tuple_struct_definition_if_ctor(def_id).is_none() => {
debug!("(building reduced graph for external crate) building type and value for {}",
Def::Struct(def_id) if self.session
.cstore
.tuple_struct_definition_if_ctor(def_id)
.is_none() => {
debug!("(building reduced graph for external crate) building type and value for \
{}",
name);
self.try_define(parent, name, TypeNS, (def, DUMMY_SP, vis));
if let Some(ctor_def_id) = self.session.cstore.struct_ctor_def_id(def_id) {
Expand All @@ -462,13 +493,8 @@ impl<'b> Resolver<'b> {
self.structs.insert(def_id, fields);
}
Def::Struct(..) => {}
Def::Local(..) |
Def::PrimTy(..) |
Def::TyParam(..) |
Def::Upvar(..) |
Def::Label(..) |
Def::SelfTy(..) |
Def::Err => {
Def::Local(..) | Def::PrimTy(..) | Def::TyParam(..) | Def::Upvar(..) |
Def::Label(..) | Def::SelfTy(..) | Def::Err => {
bug!("didn't expect `{:?}`", def);
}
}
Expand All @@ -486,7 +512,9 @@ impl<'b> Resolver<'b> {
/// Ensures that the reduced graph rooted at the given external module
/// is built, building it if it is not.
pub fn populate_module_if_necessary(&mut self, module: Module<'b>) {
if module.populated.get() { return }
if module.populated.get() {
return;
}
for child in self.session.cstore.item_children(module.def_id().unwrap()) {
self.build_reduced_graph_for_external_crate_def(module, child);
}
Expand Down
9 changes: 2 additions & 7 deletions src/librustc_resolve/check_unused.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
// except according to those terms.


//
// Unused import checking
//
// Although this is mostly a lint pass, it lives in here because it depends on
Expand Down Expand Up @@ -95,18 +94,14 @@ impl<'a, 'b> Visitor for UnusedImportCheckVisitor<'a, 'b> {
}
ast::ItemKind::Use(ref p) => {
match p.node {
ViewPathSimple(_, _) => {
self.check_import(item.id, p.span)
}
ViewPathSimple(_, _) => self.check_import(item.id, p.span),

ViewPathList(_, ref list) => {
for i in list {
self.check_import(i.node.id(), i.span);
}
}
ViewPathGlob(_) => {
self.check_import(item.id, p.span)
}
ViewPathGlob(_) => self.check_import(item.id, p.span),
}
}
_ => {}
Expand Down
Loading