From 74648b5d647526e421d5015417968b64c1900512 Mon Sep 17 00:00:00 2001 From: Theo Belaire Date: Sat, 25 Apr 2015 14:09:41 -0400 Subject: [PATCH 1/6] First attempt at fixing #20591 This isn't quite right, but it's interesting. --- src/librustc_resolve/resolve_imports.rs | 11 +++++++++++ src/test/compile-fail/double-import.rs | 25 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 src/test/compile-fail/double-import.rs diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 4b488981bfbc6..c099123ad259b 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -903,6 +903,17 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { }, &token::get_name(name)); span_err!(self.resolver.session, import_span, E0252, "{}", &msg[..]); + if let Some(sp) = target.bindings.span_for_namespace(namespace) { + span_note!(self.resolver.session, sp, + "first import of {} `{}` here", + match namespace { + TypeNS => "type", + ValueNS => "value", + }, + token::get_name(name)); + } else { + span_note!(self.resolver.session, import_span, "I can't find where it was previously imported"); + } } Some(_) | None => {} } diff --git a/src/test/compile-fail/double-import.rs b/src/test/compile-fail/double-import.rs new file mode 100644 index 0000000000000..3d0158a0830d4 --- /dev/null +++ b/src/test/compile-fail/double-import.rs @@ -0,0 +1,25 @@ +// Copyright 2013 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. +#![feature(no_std)] +#![no_std] + + +mod sub1 { + fn foo() {} // Implementation 1 +} + +mod sub2 { + fn foo() {} // Implementation 2 +} + +use sub1::foo; //~ NOTE first imported here +use sub2::foo; //~ ERROR a value named `foo` has already been imported in this module [E0252] + +fn main() {} From 168615f869ea560aa95e12a3c043c19110368be6 Mon Sep 17 00:00:00 2001 From: Theo Belaire Date: Sat, 25 Apr 2015 14:35:47 -0400 Subject: [PATCH 2/6] Now passing in the ImportResolver to check_conflict... It compiles, yay. --- src/librustc_resolve/resolve_imports.rs | 31 +++++++++++-------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index c099123ad259b..daffe08fe3a13 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -618,7 +618,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { namespace_name, name_bindings.def_for_namespace(namespace)); self.check_for_conflicting_import( - &import_resolution.target_for_namespace(namespace), + &import_resolution, directive.span, target, namespace); @@ -755,7 +755,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // Continue. } Some(ref value_target) => { - self.check_for_conflicting_import(&dest_import_resolution.value_target, + self.check_for_conflicting_import(&dest_import_resolution, import_directive.span, *ident, ValueNS); @@ -767,7 +767,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { // Continue. } Some(ref type_target) => { - self.check_for_conflicting_import(&dest_import_resolution.type_target, + self.check_for_conflicting_import(&dest_import_resolution, import_directive.span, *ident, TypeNS); @@ -885,32 +885,29 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { /// Checks that imported names and items don't have the same name. fn check_for_conflicting_import(&mut self, - target: &Option, + import_resolution: &ImportResolution, import_span: Span, name: Name, namespace: Namespace) { + let target = import_resolution.target_for_namespace(namespace); debug!("check_for_conflicting_import: {}; target exists: {}", &token::get_name(name), target.is_some()); - match *target { + match target { Some(ref target) if target.shadowable != Shadowable::Always => { - let msg = format!("a {} named `{}` has already been imported \ - in this module", - match namespace { - TypeNS => "type", - ValueNS => "value", - }, + let ns_word = match namespace { + TypeNS => "type", + ValueNS => "value", + }; + span_err!(self.resolver.session, import_span, E0252, + "a {} named `{}` has already been imported \ + in this module", ns_word, &token::get_name(name)); - span_err!(self.resolver.session, import_span, E0252, "{}", &msg[..]); if let Some(sp) = target.bindings.span_for_namespace(namespace) { span_note!(self.resolver.session, sp, "first import of {} `{}` here", - match namespace { - TypeNS => "type", - ValueNS => "value", - }, - token::get_name(name)); + ns_word, token::get_name(name)); } else { span_note!(self.resolver.session, import_span, "I can't find where it was previously imported"); } From 69a5c379dfbfdb17319eab061bef554845eca407 Mon Sep 17 00:00:00 2001 From: Theo Belaire Date: Sat, 25 Apr 2015 15:11:56 -0400 Subject: [PATCH 3/6] Maybe it works Still compiling, but I think I have it! --- src/librustc_resolve/resolve_imports.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index daffe08fe3a13..10068cf373ec5 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -896,6 +896,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { match target { Some(ref target) if target.shadowable != Shadowable::Always => { + use syntax::ast_map::NodeItem; let ns_word = match namespace { TypeNS => "type", ValueNS => "value", @@ -904,12 +905,21 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { "a {} named `{}` has already been imported \ in this module", ns_word, &token::get_name(name)); + let use_id = import_resolution.id(namespace); + if let NodeItem(item) = self.resolver.ast_map.get(use_id) { + // Assert item.node is ItemUse + // I feel like this should maybe mention the type, + // as it's otherwise a bit of work to look up... + // use syntax::ast::Item; + span_note!(self.resolver.session, item.span, + "Previously import of {} `{}` here", + ns_word, token::get_name(name)); + } + // Also showing the definition is reasonable? if let Some(sp) = target.bindings.span_for_namespace(namespace) { span_note!(self.resolver.session, sp, - "first import of {} `{}` here", + "definition of {} `{}` here", ns_word, token::get_name(name)); - } else { - span_note!(self.resolver.session, import_span, "I can't find where it was previously imported"); } } Some(_) | None => {} From 5c05278fecf9b7d4ff64fe374fb5b4598908d064 Mon Sep 17 00:00:00 2001 From: Theo Belaire Date: Sat, 25 Apr 2015 15:45:29 -0400 Subject: [PATCH 4/6] Fixed types, and slimmed down code I don't this we need to print the definition of the imported value too, though it's quite possible. --- src/librustc_resolve/resolve_imports.rs | 16 ++++------------ src/test/compile-fail/double-import.rs | 8 ++++---- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 10068cf373ec5..e731e8a5ce211 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -897,6 +897,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { match target { Some(ref target) if target.shadowable != Shadowable::Always => { use syntax::ast_map::NodeItem; + let ns_word = match namespace { TypeNS => "type", ValueNS => "value", @@ -907,19 +908,10 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { &token::get_name(name)); let use_id = import_resolution.id(namespace); if let NodeItem(item) = self.resolver.ast_map.get(use_id) { - // Assert item.node is ItemUse - // I feel like this should maybe mention the type, - // as it's otherwise a bit of work to look up... - // use syntax::ast::Item; + // item is syntax::ast::Item; span_note!(self.resolver.session, item.span, - "Previously import of {} `{}` here", - ns_word, token::get_name(name)); - } - // Also showing the definition is reasonable? - if let Some(sp) = target.bindings.span_for_namespace(namespace) { - span_note!(self.resolver.session, sp, - "definition of {} `{}` here", - ns_word, token::get_name(name)); + "previous import of `{}` here", + token::get_name(name)); } } Some(_) | None => {} diff --git a/src/test/compile-fail/double-import.rs b/src/test/compile-fail/double-import.rs index 3d0158a0830d4..d267efe8b25e7 100644 --- a/src/test/compile-fail/double-import.rs +++ b/src/test/compile-fail/double-import.rs @@ -12,14 +12,14 @@ mod sub1 { - fn foo() {} // Implementation 1 + fn foo() {} // implementation 1 } mod sub2 { - fn foo() {} // Implementation 2 + fn foo() {} // implementation 2 } -use sub1::foo; //~ NOTE first imported here -use sub2::foo; //~ ERROR a value named `foo` has already been imported in this module [E0252] +use sub1::foo; //~ note previous import of `foo` here +use sub2::foo; //~ error a value named `foo` has already been imported in this module [e0252] fn main() {} From 372c69d8d293985ad3bffbe5c5e527dc37e90169 Mon Sep 17 00:00:00 2001 From: Theo Belaire Date: Mon, 27 Apr 2015 00:45:51 -0400 Subject: [PATCH 5/6] Fixed test text --- src/test/compile-fail/double-import.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/compile-fail/double-import.rs b/src/test/compile-fail/double-import.rs index d267efe8b25e7..e564d39f80de7 100644 --- a/src/test/compile-fail/double-import.rs +++ b/src/test/compile-fail/double-import.rs @@ -19,7 +19,7 @@ mod sub2 { fn foo() {} // implementation 2 } -use sub1::foo; //~ note previous import of `foo` here -use sub2::foo; //~ error a value named `foo` has already been imported in this module [e0252] +use sub1::foo; //~ NOTE previous import of `foo` here +use sub2::foo; //~ ERROR a value named `foo` has already been imported in this module [E0252] fn main() {} From db9d01842450fe07f77ca97d9a68d105366f407e Mon Sep 17 00:00:00 2001 From: Theo Belaire Date: Thu, 30 Apr 2015 22:46:15 -0400 Subject: [PATCH 6/6] Fixed some nits --- src/librustc_resolve/resolve_imports.rs | 13 +++++-------- src/test/compile-fail/double-import.rs | 4 +++- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index e731e8a5ce211..91eb068e845a1 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -896,8 +896,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { match target { Some(ref target) if target.shadowable != Shadowable::Always => { - use syntax::ast_map::NodeItem; - let ns_word = match namespace { TypeNS => "type", ValueNS => "value", @@ -907,12 +905,11 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { in this module", ns_word, &token::get_name(name)); let use_id = import_resolution.id(namespace); - if let NodeItem(item) = self.resolver.ast_map.get(use_id) { - // item is syntax::ast::Item; - span_note!(self.resolver.session, item.span, - "previous import of `{}` here", - token::get_name(name)); - } + let item = self.resolver.ast_map.expect_item(use_id); + // item is syntax::ast::Item; + span_note!(self.resolver.session, item.span, + "previous import of `{}` here", + token::get_name(name)); } Some(_) | None => {} } diff --git a/src/test/compile-fail/double-import.rs b/src/test/compile-fail/double-import.rs index e564d39f80de7..cbf13c0a55909 100644 --- a/src/test/compile-fail/double-import.rs +++ b/src/test/compile-fail/double-import.rs @@ -1,4 +1,4 @@ -// Copyright 2013 The Rust Project Developers. See the COPYRIGHT +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT // file at the top-level directory of this distribution and at // http://rust-lang.org/COPYRIGHT. // @@ -10,6 +10,8 @@ #![feature(no_std)] #![no_std] +// This tests that conflicting imports shows both `use` lines +// when reporting the error. mod sub1 { fn foo() {} // implementation 1