Skip to content

Provide a note if method lookup fails and there are static definitions with the same name. #13685

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

Merged
merged 1 commit into from
May 3, 2014
Merged

Provide a note if method lookup fails and there are static definitions with the same name. #13685

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
40 changes: 21 additions & 19 deletions src/librustc/middle/resolve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use metadata::decoder::{DefLike, DlDef, DlField, DlImpl};
use middle::lang_items::LanguageItems;
use middle::lint::{UnnecessaryQualification, UnusedImports};
use middle::pat_util::pat_bindings;
use util::nodemap::{NodeMap, DefIdSet, FnvHashSet};
use util::nodemap::{NodeMap, DefIdSet, FnvHashMap};

use syntax::ast::*;
use syntax::ast;
Expand Down Expand Up @@ -821,7 +821,7 @@ fn Resolver<'a>(session: &'a Session,

graph_root: graph_root,

method_set: RefCell::new(FnvHashSet::new()),
method_map: RefCell::new(FnvHashMap::new()),
structs: HashSet::new(),

unresolved_imports: 0,
Expand Down Expand Up @@ -860,7 +860,7 @@ struct Resolver<'a> {

graph_root: NameBindings,

method_set: RefCell<FnvHashSet<(Name, DefId)>>,
method_map: RefCell<FnvHashMap<(Name, DefId), ast::ExplicitSelf_>>,
structs: HashSet<DefId>,

// The number of imports that are currently unresolved.
Expand Down Expand Up @@ -1371,10 +1371,8 @@ impl<'a> Resolver<'a> {
ty_m.span);
method_name_bindings.define_value(def, ty_m.span, true);

// Add it to the trait info if not static.
if ty_m.explicit_self.node != SelfStatic {
self.method_set.borrow_mut().insert((ident.name, def_id));
}
self.method_map.borrow_mut().insert((ident.name, def_id),
ty_m.explicit_self.node);
}

name_bindings.define_type(DefTrait(def_id), sp, is_public);
Expand Down Expand Up @@ -1660,10 +1658,8 @@ impl<'a> Resolver<'a> {
trait method '{}'",
token::get_ident(method_name));

// Add it to the trait info if not static.
if explicit_self != SelfStatic {
self.method_set.borrow_mut().insert((method_name.name, def_id));
}
self.method_map.borrow_mut().insert((method_name.name, def_id), explicit_self);

if is_exported {
self.external_exports.insert(method_def_id);
}
Expand Down Expand Up @@ -4718,10 +4714,16 @@ impl<'a> Resolver<'a> {
match containing_module.kind.get() {
TraitModuleKind | ImplModuleKind => {
match containing_module.def_id.get() {
Some(def_id) if self.method_set.borrow().contains(&(ident.name, def_id)) => {
debug!("containing module was a trait or impl \
Some(def_id) => {
match self.method_map.borrow().find(&(ident.name, def_id)) {
Some(x) if *x == SelfStatic => (),
None => (),
_ => {
debug!("containing module was a trait or impl \
and name was a method -> not resolved");
return None;
return None;
}
}
},
_ => (),
}
Expand Down Expand Up @@ -5110,9 +5112,9 @@ impl<'a> Resolver<'a> {
// Look for the current trait.
match self.current_trait_refs {
Some(ref trait_def_ids) => {
let method_set = self.method_set.borrow();
let method_map = self.method_map.borrow();
for &trait_def_id in trait_def_ids.iter() {
if method_set.contains(&(name, trait_def_id)) {
if method_map.contains_key(&(name, trait_def_id)) {
add_trait_info(&mut found_traits, trait_def_id, name);
}
}
Expand All @@ -5126,7 +5128,7 @@ impl<'a> Resolver<'a> {
self.populate_module_if_necessary(&search_module);

{
let method_set = self.method_set.borrow();
let method_map = self.method_map.borrow();
for (_, child_names) in search_module.children.borrow().iter() {
let def = match child_names.def_for_namespace(TypeNS) {
Some(def) => def,
Expand All @@ -5136,7 +5138,7 @@ impl<'a> Resolver<'a> {
DefTrait(trait_def_id) => trait_def_id,
_ => continue,
};
if method_set.contains(&(name, trait_def_id)) {
if method_map.contains_key(&(name, trait_def_id)) {
add_trait_info(&mut found_traits, trait_def_id, name);
}
}
Expand All @@ -5152,7 +5154,7 @@ impl<'a> Resolver<'a> {
Some(DefTrait(trait_def_id)) => trait_def_id,
Some(..) | None => continue,
};
if self.method_set.borrow().contains(&(name, did)) {
if self.method_map.borrow().contains_key(&(name, did)) {
Copy link
Member

Choose a reason for hiding this comment

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

These changes to resolve look like they'll start resolving static functions as candidates for methods (possibly). Can you make sure that code like this still compiles?

trait Foo {
    fn new() {}
}

trait Bar {
    fn new(&self) {}
}

impl Bar for int {}
impl Foo for int {}

fn main() {
    1i.new();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only checks for static methods on failed method lookup (and it's going to report an error regardless of finding a static, it just sugars the error), so this code will compile fine as the method lookup won't fail.

I couldn't find a runpass test for this name overlap, so I just added one for posterity. (In latest commit)

Copy link
Member

Choose a reason for hiding this comment

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

Oh excellent, thank you!

add_trait_info(&mut found_traits, did, name);
self.used_imports.insert((import.type_id, TypeNS));
}
Expand Down
69 changes: 58 additions & 11 deletions src/librustc/middle/typeck/check/method.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,12 @@ pub enum AutoderefReceiverFlag {
DontAutoderefReceiver,
}

#[deriving(Eq)]
pub enum StaticMethodsFlag {
ReportStaticMethods,
IgnoreStaticMethods,
}

pub fn lookup<'a>(
fcx: &'a FnCtxt<'a>,

Expand All @@ -129,7 +135,8 @@ pub fn lookup<'a>(
supplied_tps: &'a [ty::t], // The list of types X, Y, ... .
deref_args: check::DerefArgs, // Whether we autopointer first.
check_traits: CheckTraitsFlag, // Whether we check traits only.
autoderef_receiver: AutoderefReceiverFlag)
autoderef_receiver: AutoderefReceiverFlag,
report_statics: StaticMethodsFlag)
-> Option<MethodCallee> {
let mut lcx = LookupContext {
fcx: fcx,
Expand All @@ -143,6 +150,7 @@ pub fn lookup<'a>(
deref_args: deref_args,
check_traits: check_traits,
autoderef_receiver: autoderef_receiver,
report_statics: report_statics,
};

debug!("method lookup(self_ty={}, expr={}, self_expr={})",
Expand Down Expand Up @@ -173,7 +181,8 @@ pub fn lookup_in_trait<'a>(
trait_did: DefId, // The trait to limit the lookup to.
self_ty: ty::t, // The type of `a`.
supplied_tps: &'a [ty::t], // The list of types X, Y, ... .
autoderef_receiver: AutoderefReceiverFlag)
autoderef_receiver: AutoderefReceiverFlag,
report_statics: StaticMethodsFlag)
-> Option<MethodCallee> {
let mut lcx = LookupContext {
fcx: fcx,
Expand All @@ -187,6 +196,7 @@ pub fn lookup_in_trait<'a>(
deref_args: check::DoDerefArgs,
check_traits: CheckTraitsOnly,
autoderef_receiver: autoderef_receiver,
report_statics: report_statics,
};

debug!("method lookup_in_trait(self_ty={}, self_expr={})",
Expand All @@ -197,8 +207,6 @@ pub fn lookup_in_trait<'a>(
lcx.search(self_ty)
}



// Determine the index of a method in the list of all methods belonging
// to a trait and its supertraits.
fn get_method_index(tcx: &ty::ctxt,
Expand Down Expand Up @@ -301,6 +309,7 @@ struct LookupContext<'a> {
deref_args: check::DerefArgs,
check_traits: CheckTraitsFlag,
autoderef_receiver: AutoderefReceiverFlag,
report_statics: StaticMethodsFlag,
}

/**
Expand Down Expand Up @@ -661,7 +670,17 @@ impl<'a> LookupContext<'a> {
impl_did: DefId,
impl_methods: &[DefId],
is_extension: bool) {
if !self.impl_dups.insert(impl_did) {
let did = if self.report_statics == ReportStaticMethods {
// we only want to report each base trait once
match ty::impl_trait_ref(self.tcx(), impl_did) {
Some(trait_ref) => trait_ref.def_id,
None => impl_did
}
} else {
impl_did
};

if !self.impl_dups.insert(did) {
return; // already visited
}

Expand Down Expand Up @@ -1018,6 +1037,25 @@ impl<'a> LookupContext<'a> {
return None;
}

if self.report_statics == ReportStaticMethods {
// lookup should only be called with ReportStaticMethods if a regular lookup failed
assert!(relevant_candidates.iter().all(|c| c.method_ty.explicit_self == SelfStatic));

self.tcx().sess.fileline_note(self.span,
"found defined static methods, maybe a `self` is missing?");

for (idx, candidate) in relevant_candidates.iter().enumerate() {
self.report_candidate(idx, &candidate.origin);
}

// return something so we don't get errors for every mutability
return Some(MethodCallee {
origin: relevant_candidates.get(0).origin,
ty: ty::mk_err(),
substs: substs::empty()
});
}

if relevant_candidates.len() > 1 {
self.tcx().sess.span_err(
self.span,
Expand Down Expand Up @@ -1305,7 +1343,7 @@ impl<'a> LookupContext<'a> {
return match candidate.method_ty.explicit_self {
SelfStatic => {
debug!("(is relevant?) explicit self is static");
false
self.report_statics == ReportStaticMethods
}

SelfValue => {
Expand Down Expand Up @@ -1391,11 +1429,20 @@ impl<'a> LookupContext<'a> {
fn report_candidate(&self, idx: uint, origin: &MethodOrigin) {
match *origin {
MethodStatic(impl_did) => {
// If it is an instantiated default method, use the original
// default method for error reporting.
let did = match provided_source(self.tcx(), impl_did) {
None => impl_did,
Some(did) => did
let did = if self.report_statics == ReportStaticMethods {
// If we're reporting statics, we want to report the trait
// definition if possible, rather than an impl
match ty::trait_method_of_method(self.tcx(), impl_did) {
None => {debug!("(report candidate) No trait method found"); impl_did},
Some(trait_did) => {debug!("(report candidate) Found trait ref"); trait_did}
}
} else {
// If it is an instantiated default method, use the original
// default method for error reporting.
match provided_source(self.tcx(), impl_did) {
None => impl_did,
Some(did) => did
}
};
self.report_static_candidate(idx, did)
}
Expand Down
22 changes: 17 additions & 5 deletions src/librustc/middle/typeck/check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ use middle::typeck::check::method::{AutoderefReceiver};
use middle::typeck::check::method::{AutoderefReceiverFlag};
use middle::typeck::check::method::{CheckTraitsAndInherentMethods};
use middle::typeck::check::method::{DontAutoderefReceiver};
use middle::typeck::check::method::{IgnoreStaticMethods, ReportStaticMethods};
use middle::typeck::check::regionmanip::replace_late_bound_regions_in_fn_sig;
use middle::typeck::check::regionmanip::relate_free_regions;
use middle::typeck::check::vtable::VtableContext;
Expand Down Expand Up @@ -1360,7 +1361,7 @@ fn try_overloaded_deref(fcx: &FnCtxt,
(PreferMutLvalue, Some(trait_did)) => {
method::lookup_in_trait(fcx, span, base_expr.map(|x| &*x),
token::intern("deref_mut"), trait_did,
base_ty, [], DontAutoderefReceiver)
base_ty, [], DontAutoderefReceiver, IgnoreStaticMethods)
}
_ => None
};
Expand All @@ -1370,7 +1371,7 @@ fn try_overloaded_deref(fcx: &FnCtxt,
(None, Some(trait_did)) => {
method::lookup_in_trait(fcx, span, base_expr.map(|x| &*x),
token::intern("deref"), trait_did,
base_ty, [], DontAutoderefReceiver)
base_ty, [], DontAutoderefReceiver, IgnoreStaticMethods)
}
(method, _) => method
};
Expand Down Expand Up @@ -1956,7 +1957,7 @@ fn check_expr_with_unifier(fcx: &FnCtxt,
expr_t, tps.as_slice(),
DontDerefArgs,
CheckTraitsAndInherentMethods,
AutoderefReceiver) {
AutoderefReceiver, IgnoreStaticMethods) {
Some(method) => {
let method_ty = method.ty;
let method_call = MethodCall::expr(expr.id);
Expand All @@ -1976,6 +1977,15 @@ fn check_expr_with_unifier(fcx: &FnCtxt,

// Add error type for the result
fcx.write_error(expr.id);

// Check for potential static matches (missing self parameters)
method::lookup(fcx, expr, rcvr,
method_name.node.name,
expr_t, tps.as_slice(),
DontDerefArgs,
CheckTraitsAndInherentMethods,
DontAutoderefReceiver, ReportStaticMethods);

ty::mk_err()
}
};
Expand Down Expand Up @@ -2040,7 +2050,8 @@ fn check_expr_with_unifier(fcx: &FnCtxt,
let method = match trait_did {
Some(trait_did) => {
method::lookup_in_trait(fcx, op_ex.span, Some(&*args[0]), opname,
trait_did, self_t, [], autoderef_receiver)
trait_did, self_t, [], autoderef_receiver,
IgnoreStaticMethods)
}
None => None
};
Expand Down Expand Up @@ -2348,7 +2359,8 @@ fn check_expr_with_unifier(fcx: &FnCtxt,
tps.as_slice(),
DontDerefArgs,
CheckTraitsAndInherentMethods,
AutoderefReceiver) {
AutoderefReceiver,
IgnoreStaticMethods) {
Some(_) => {
fcx.type_error_message(
expr.span,
Expand Down
Loading