7 files changed, 109 insertions, 57 deletions

diff --git a/crates/ra_analysis/src/completion/complete_scope.rs b/crates/ra_analysis/src/completion/complete_scope.rs
index 514fd2f88..cd98efe95 100644
--- a/crates/ra_analysis/src/completion/complete_scope.rs
+++ b/crates/ra_analysis/src/completion/complete_scope.rs
@@ -74,7 +74,7 @@ mod tests {
                 let z = ();
             }
             ",
-            "y;x;quux",
+            r#"y;x;quux "quux($0)""#,
         );
     }
 
@@ -92,7 +92,7 @@ mod tests {
                 }
             }
             ",
-            "b;a;quux",
+            r#"b;a;quux "quux()$0""#,
         );
     }
 
@@ -106,7 +106,7 @@ mod tests {
                 }
             }
             ",
-            "x;quux",
+            r#"x;quux "quux()$0""#,
         );
     }
 
@@ -120,7 +120,7 @@ mod tests {
                 <|>
             }
             ",
-            "quux;Foo;Baz",
+            r#"quux "quux()$0";Foo;Baz"#,
         );
     }
 
@@ -134,7 +134,7 @@ mod tests {
                 fn quux() { <|> }
             }
             ",
-            "quux;Bar",
+            r#"quux "quux()$0";Bar"#,
         );
     }
 
@@ -145,12 +145,12 @@ mod tests {
             struct Foo;
             fn x() -> <|>
             ",
-            "Foo;x",
+            r#"Foo;x "x()$0""#,
         )
     }
 
     #[test]
-    fn dont_show_to_completions_for_shadowing() {
+    fn dont_show_both_completions_for_shadowing() {
         check_reference_completion(
             r"
             fn foo() -> {
@@ -161,7 +161,7 @@ mod tests {
                 }
             }
             ",
-            "bar;foo",
+            r#"bar;foo "foo()$0""#,
         )
     }
 
@@ -169,4 +169,24 @@ mod tests {
     fn completes_self_in_methods() {
         check_reference_completion(r"impl S { fn foo(&self) { <|> } }", "self")
     }
+
+    #[test]
+    fn inserts_parens_for_function_calls() {
+        check_reference_completion(
+            r"
+            fn no_args() {}
+            fn main() { no_<|> }
+            ",
+            r#"no_args "no_args()$0"
+               main "main()$0""#,
+        );
+        check_reference_completion(
+            r"
+            fn with_args(x: i32, y: String) {}
+            fn main() { with_<|> }
+            ",
+            r#"main "main()$0"
+               with_args "with_args($0)""#,
+        );
+    }
 }
diff --git a/crates/ra_analysis/src/completion/completion_item.rs b/crates/ra_analysis/src/completion/completion_item.rs
index 1d294c553..b8fa39ae3 100644
--- a/crates/ra_analysis/src/completion/completion_item.rs
+++ b/crates/ra_analysis/src/completion/completion_item.rs
@@ -138,9 +138,18 @@ impl Builder {
                 ..
             } => CompletionItemKind::Enum,
             PerNs {
-                values: Some(hir::Def::Function(..)),
+                values: Some(hir::Def::Function(function)),
                 ..
-            } => CompletionItemKind::Function,
+            } => {
+                if let Some(sig_info) = function.signature_info(db) {
+                    if sig_info.params.is_empty() {
+                        self.snippet = Some(format!("{}()$0", self.label));
+                    } else {
+                        self.snippet = Some(format!("{}($0)", self.label));
+                    }
+                }
+                CompletionItemKind::Function
+            }
             _ => return self,
         };
         self.kind = Some(kind);
diff --git a/crates/ra_hir/src/db.rs b/crates/ra_hir/src/db.rs
index b41a7429a..5a8ca3b47 100644
--- a/crates/ra_hir/src/db.rs
+++ b/crates/ra_hir/src/db.rs
@@ -36,17 +36,17 @@ pub trait HirDatabase: SyntaxDatabase
 
     fn infer(def_id: DefId) -> Cancelable<Arc<InferenceResult>> {
         type InferQuery;
-        use fn query_definitions::infer;
+        use fn crate::ty::infer;
     }
 
     fn type_for_def(def_id: DefId) -> Cancelable<Ty> {
         type TypeForDefQuery;
-        use fn query_definitions::type_for_def;
+        use fn crate::ty::type_for_def;
     }
 
     fn type_for_field(def_id: DefId, field: Name) -> Cancelable<Ty> {
         type TypeForFieldQuery;
-        use fn query_definitions::type_for_field;
+        use fn crate::ty::type_for_field;
     }
 
     fn file_items(file_id: FileId) -> Arc<SourceFileItems> {
diff --git a/crates/ra_hir/src/query_definitions.rs b/crates/ra_hir/src/query_definitions.rs
index 016d86ee6..721bd4195 100644
--- a/crates/ra_hir/src/query_definitions.rs
+++ b/crates/ra_hir/src/query_definitions.rs
@@ -19,7 +19,6 @@ use crate::{
         imp::Submodule,
         nameres::{InputModuleItems, ItemMap, Resolver},
     },
-    ty::{self, InferenceResult, Ty},
     adt::{StructData, EnumData},
 };
 
@@ -30,19 +29,6 @@ pub(super) fn fn_scopes(db: &impl HirDatabase, def_id: DefId) -> Arc<FnScopes> {
     Arc::new(res)
 }
 
-pub(super) fn infer(db: &impl HirDatabase, def_id: DefId) -> Cancelable<Arc<InferenceResult>> {
-    let function = Function::new(def_id);
-    ty::infer(db, function).map(Arc::new)
-}
-
-pub(super) fn type_for_def(db: &impl HirDatabase, def_id: DefId) -> Cancelable<Ty> {
-    ty::type_for_def(db, def_id)
-}
-
-pub(super) fn type_for_field(db: &impl HirDatabase, def_id: DefId, field: Name) -> Cancelable<Ty> {
-    ty::type_for_field(db, def_id, field)
-}
-
 pub(super) fn struct_data(db: &impl HirDatabase, def_id: DefId) -> Cancelable<Arc<StructData>> {
     let def_loc = def_id.loc(db);
     assert!(def_loc.kind == DefKind::Struct);
diff --git a/crates/ra_hir/src/ty.rs b/crates/ra_hir/src/ty.rs
index 4ebd44d27..719b3f7cd 100644
--- a/crates/ra_hir/src/ty.rs
+++ b/crates/ra_hir/src/ty.rs
@@ -1,3 +1,18 @@
+//! The type system. We currently use this to infer types for completion.
+//!
+//! For type inference, compare the implementations in rustc (the various
+//! check_* methods in librustc_typeck/check/mod.rs are a good entry point) and
+//! IntelliJ-Rust (org.rust.lang.core.types.infer). Our entry point for
+//! inference here is the `infer` function, which infers the types of all
+//! expressions in a given function.
+//!
+//! The central struct here is `Ty`, which represents a type. During inference,
+//! it can contain type 'variables' which represent currently unknown types; as
+//! we walk through the expressions, we might determine that certain variables
+//! need to be equal to each other, or to certain types. To record this, we use
+//! the union-find implementation from the `ena` crate, which is extracted from
+//! rustc.
+
 mod primitive;
 #[cfg(test)]
 mod tests;
@@ -21,6 +36,7 @@ use crate::{
     type_ref::{TypeRef, Mutability},
 };
 
+/// The ID of a type variable.
 #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
 pub struct TypeVarId(u32);
 
@@ -40,6 +56,8 @@ impl UnifyKey for TypeVarId {
     }
 }
 
+/// The value of a type variable: either we already know the type, or we don't
+/// know it yet.
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub enum TypeVarValue {
     Known(Ty),
@@ -47,7 +65,7 @@ pub enum TypeVarValue {
 }
 
 impl TypeVarValue {
-    pub fn known(&self) -> Option<&Ty> {
+    fn known(&self) -> Option<&Ty> {
         match self {
             TypeVarValue::Known(ty) => Some(ty),
             TypeVarValue::Unknown => None,
@@ -75,10 +93,13 @@ impl UnifyValue for TypeVarValue {
     }
 }
 
+/// The kinds of placeholders we need during type inference. Currently, we only
+/// have type variables; in the future, we will probably also need int and float
+/// variables, for inference of literal values (e.g. `100` could be one of
+/// several integer types).
 #[derive(Clone, PartialEq, Eq, Hash, Debug)]
 pub enum InferTy {
     TypeVar(TypeVarId),
-    // later we'll have IntVar and FloatVar as well
 }
 
 /// When inferring an expression, we propagate downward whatever type hint we
@@ -92,15 +113,21 @@ struct Expectation {
 }
 
 impl Expectation {
+    /// The expectation that the type of the expression needs to equal the given
+    /// type.
     fn has_type(ty: Ty) -> Self {
         Expectation { ty }
     }
 
+    /// This expresses no expectation on the type.
     fn none() -> Self {
         Expectation { ty: Ty::Unknown }
     }
 }
 
+/// A type. This is based on the `TyKind` enum in rustc (librustc/ty/sty.rs).
+///
+/// This should be cheap to clone.
 #[derive(Clone, PartialEq, Eq, Hash, Debug)]
 pub enum Ty {
     /// The primitive boolean type. Written as `bool`.
@@ -134,14 +161,14 @@ pub enum Ty {
     // An array with the given length. Written as `[T; n]`.
     // Array(Ty, ty::Const),
     /// The pointee of an array slice.  Written as `[T]`.
-    Slice(TyRef),
+    Slice(Arc<Ty>),
 
     /// A raw pointer. Written as `*mut T` or `*const T`
-    RawPtr(TyRef, Mutability),
+    RawPtr(Arc<Ty>, Mutability),
 
     /// A reference; a pointer with an associated lifetime. Written as
     /// `&'a mut T` or `&'a T`.
-    Ref(TyRef, Mutability),
+    Ref(Arc<Ty>, Mutability),
 
     /// A pointer to a function.  Written as `fn() -> i32`.
     ///
@@ -153,6 +180,10 @@ pub enum Ty {
     /// ```
     FnPtr(Arc<FnSig>),
 
+    // rustc has a separate type for each function, which just coerces to the
+    // above function pointer type. Once we implement generics, we will probably
+    // need this as well.
+
     // A trait, defined with `dyn trait`.
     // Dynamic(),
     // The anonymous type of a closure. Used to represent the type of
@@ -166,7 +197,7 @@ pub enum Ty {
     // A type representin the types stored inside a generator.
     // This should only appear in GeneratorInteriors.
     // GeneratorWitness(Binder<&'tcx List<Ty<'tcx>>>),
-    /// The never type `!`
+    /// The never type `!`.
     Never,
 
     /// A tuple type.  For example, `(i32, bool)`.
@@ -177,10 +208,6 @@ pub enum Ty {
     // Projection(ProjectionTy),
 
     // Opaque (`impl Trait`) type found in a return type.
-    // The `DefId` comes either from
-    // * the `impl Trait` ast::Ty node,
-    // * or the `existential type` declaration
-    // The substitutions are for the generics of the function in question.
     // Opaque(DefId, Substs),
 
     // A type parameter; for example, `T` in `fn f<T>(x: T) {}
@@ -192,12 +219,12 @@ pub enum Ty {
     /// A placeholder for a type which could not be computed; this is propagated
     /// to avoid useless error messages. Doubles as a placeholder where type
     /// variables are inserted before type checking, since we want to try to
-    /// infer a better type here anyway.
+    /// infer a better type here anyway -- for the IDE use case, we want to try
+    /// to infer as much as possible even in the presence of type errors.
     Unknown,
 }
 
-type TyRef = Arc<Ty>;
-
+/// A function signature.
 #[derive(Clone, PartialEq, Eq, Hash, Debug)]
 pub struct FnSig {
     input: Vec<Ty>,
@@ -368,7 +395,11 @@ impl fmt::Display for Ty {
     }
 }
 
-pub fn type_for_fn(db: &impl HirDatabase, f: Function) -> Cancelable<Ty> {
+// Functions returning declared types for items
+
+/// Compute the declared type of a function. This should not need to look at the
+/// function body (but currently uses the function AST, so does anyway - TODO).
+fn type_for_fn(db: &impl HirDatabase, f: Function) -> Cancelable<Ty> {
     let syntax = f.syntax(db);
     let module = f.module(db)?;
     let node = syntax.borrowed();
@@ -390,7 +421,7 @@ pub fn type_for_fn(db: &impl HirDatabase, f: Function) -> Cancelable<Ty> {
     Ok(Ty::FnPtr(Arc::new(sig)))
 }
 
-pub fn type_for_struct(db: &impl HirDatabase, s: Struct) -> Cancelable<Ty> {
+fn type_for_struct(db: &impl HirDatabase, s: Struct) -> Cancelable<Ty> {
     Ok(Ty::Adt {
         def_id: s.def_id(),
         name: s.name(db)?.unwrap_or_else(Name::missing),
@@ -404,7 +435,7 @@ pub fn type_for_enum(db: &impl HirDatabase, s: Enum) -> Cancelable<Ty> {
     })
 }
 
-pub fn type_for_def(db: &impl HirDatabase, def_id: DefId) -> Cancelable<Ty> {
+pub(super) fn type_for_def(db: &impl HirDatabase, def_id: DefId) -> Cancelable<Ty> {
     let def = def_id.resolve(db)?;
     match def {
         Def::Module(..) => {
@@ -444,19 +475,25 @@ pub(super) fn type_for_field(db: &impl HirDatabase, def_id: DefId, field: Name)
     Ty::from_hir(db, &module, &type_ref)
 }
 
+/// The result of type inference: A mapping from expressions and patterns to types.
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub struct InferenceResult {
     type_of: FxHashMap<LocalSyntaxPtr, Ty>,
 }
 
 impl InferenceResult {
+    /// Returns the type of the given syntax node, if it was inferred. Will
+    /// return `None` for syntax nodes not in the inferred function or not
+    /// pointing to an expression/pattern, `Some(Ty::Unknown)` for
+    /// expressions/patterns that could not be inferred.
     pub fn type_of_node(&self, node: SyntaxNodeRef) -> Option<Ty> {
         self.type_of.get(&LocalSyntaxPtr::new(node)).cloned()
     }
 }
 
+/// The inference context contains all information needed during type inference.
 #[derive(Clone, Debug)]
-pub struct InferenceContext<'a, D: HirDatabase> {
+struct InferenceContext<'a, D: HirDatabase> {
     db: &'a D,
     scopes: Arc<FnScopes>,
     module: Module,
@@ -738,6 +775,7 @@ impl<'a, D: HirDatabase> InferenceContext<'a, D> {
             ast::Expr::ParenExpr(e) => self.infer_expr_opt(e.expr(), expected)?,
             ast::Expr::Label(_e) => Ty::Unknown,
             ast::Expr::ReturnExpr(e) => {
+                // TODO expect return type of function
                 self.infer_expr_opt(e.expr(), &Expectation::none())?;
                 Ty::Never
             }
@@ -870,7 +908,8 @@ impl<'a, D: HirDatabase> InferenceContext<'a, D> {
     }
 }
 
-pub fn infer(db: &impl HirDatabase, function: Function) -> Cancelable<InferenceResult> {
+pub fn infer(db: &impl HirDatabase, def_id: DefId) -> Cancelable<Arc<InferenceResult>> {
+    let function = Function::new(def_id); // TODO: consts also need inference
     let scopes = function.scopes(db);
     let module = function.module(db)?;
     let mut ctx = InferenceContext::new(db, scopes, module);
@@ -909,5 +948,5 @@ pub fn infer(db: &impl HirDatabase, function: Function) -> Cancelable<InferenceR
         ctx.infer_block(block, &Expectation::has_type(ret_ty))?;
     }
 
-    Ok(ctx.resolve_all())
+    Ok(Arc::new(ctx.resolve_all()))
 }
diff --git a/crates/ra_vfs/src/io.rs b/crates/ra_vfs/src/io.rs
index be400bae9..4cfdb83da 100644
--- a/crates/ra_vfs/src/io.rs
+++ b/crates/ra_vfs/src/io.rs
@@ -8,7 +8,7 @@ use walkdir::{DirEntry, WalkDir};
 use thread_worker::{WorkerHandle};
 use relative_path::RelativePathBuf;
 
-use crate::VfsRoot;
+use crate::{VfsRoot, has_rs_extension};
 
 pub(crate) struct Task {
     pub(crate) root: VfsRoot,
@@ -59,7 +59,7 @@ fn load_root(root: &Path, filter: &dyn Fn(&DirEntry) -> bool) -> Vec<(RelativePa
             continue;
         }
         let path = entry.path();
-        if path.extension().and_then(|os| os.to_str()) != Some("rs") {
+        if !has_rs_extension(path) {
             continue;
         }
         let text = match fs::read_to_string(path) {
diff --git a/crates/ra_vfs/src/lib.rs b/crates/ra_vfs/src/lib.rs
index 4de07b093..90d5e21f4 100644
--- a/crates/ra_vfs/src/lib.rs
+++ b/crates/ra_vfs/src/lib.rs
@@ -2,11 +2,13 @@
 //!
 //! When doing analysis, we don't want to do any IO, we want to keep all source
 //! code in memory. However, the actual source code is stored on disk, so you
-//! component which does this.
 //! need to get it into the memory in the first place somehow. VFS is the
+//! component which does this.
 //!
-//! It also is responsible for watching the disk for changes, and for merging
+//! It is also responsible for watching the disk for changes, and for merging
 //! editor state (modified, unsaved files) with disk state.
+//! TODO: Some LSP clients support watching the disk, so this crate should
+//! to support custom watcher events (related to https://github.com/rust-analyzer/rust-analyzer/issues/131)
 //!
 //! VFS is based on a concept of roots: a set of directories on the file system
 //! whihc are watched for changes. Typically, there will be a root for each
@@ -29,7 +31,7 @@ use rustc_hash::{FxHashMap, FxHashSet};
 use relative_path::RelativePathBuf;
 use crossbeam_channel::Receiver;
 use walkdir::DirEntry;
-use thread_worker::{WorkerHandle};
+use thread_worker::WorkerHandle;
 
 use crate::{
     arena::{ArenaId, Arena},
@@ -57,12 +59,8 @@ impl RootFilter {
         if !(self.file_filter)(path) {
             return None;
         }
-        if !(path.starts_with(&self.root)) {
-            return None;
-        }
-        let path = path.strip_prefix(&self.root).unwrap();
-        let path = RelativePathBuf::from_path(path).unwrap();
-        Some(path)
+        let path = path.strip_prefix(&self.root).ok()?;
+        RelativePathBuf::from_path(path).ok()
     }
 }