Make find_call_name return a Span

Summary: If we ever want to do further manipulations (e.g. find hover locations) then a Span is better than a ResolvedSpan.

Reviewed By: milend

Differential Revision: D47190448

fbshipit-source-id: a09242bc150c9f5b9a1f7ebf3e9afa52e3418dbf

Author: ndmitchell

starlark/src/analysis/find_call_name.rs

@@ -15,7 +15,6 @@
  * limitations under the License.
  */
 
-use crate::codemap::ResolvedSpan;
 use crate::codemap::Span;
 use crate::codemap::Spanned;
 use crate::syntax::ast::Argument;
@@ -30,7 +29,7 @@ impl AstModule {
     ///
     /// NOTE: If the AST is exposed in the future, this function may be removed and implemented
     ///       by specific programs instead.
-    pub fn find_function_call_with_name(&self, name: &str) -> Option<ResolvedSpan> {
+    pub fn find_function_call_with_name(&self, name: &str) -> Option<Span> {
         let mut ret = None;
 
         fn visit_expr(ret: &mut Option<Span>, name: &str, node: &AstExpr) {
@@ -64,7 +63,7 @@ impl AstModule {
         }
 
         self.statement.visit_expr(|x| visit_expr(&mut ret, name, x));
-        ret.map(|span| self.codemap.resolve_span(span))
+        ret
     }
 }
 
@@ -95,7 +94,9 @@ def x(name = "foo_name"):
                 end_line: 1,
                 end_column: 3
             }),
-            module.find_function_call_with_name("foo_name")
+            module
+                .find_function_call_with_name("foo_name")
+                .map(|span| module.codemap.resolve_span(span))
         );
         assert_eq!(None, module.find_function_call_with_name("bar_name"));
         Ok(())

starlark/src/lsp/server.rs

@@ -67,6 +67,7 @@ use serde::Serialize;
 use serde::Serializer;
 
 use crate::codemap::ResolvedSpan;
+use crate::codemap::Span;
 use crate::lsp::definition::Definition;
 use crate::lsp::definition::DottedDefinition;
 use crate::lsp::definition::IdentifierDefinition;
@@ -219,8 +220,7 @@ pub struct StringLiteralResult {
     ///
     /// If `None`, then just jump to the URL. Do not attempt to load the file.
     #[derivative(Debug = "ignore")]
-    pub location_finder:
-        Option<Box<dyn FnOnce(&AstModule) -> anyhow::Result<Option<Range>> + Send>>,
+    pub location_finder: Option<Box<dyn FnOnce(&AstModule) -> anyhow::Result<Option<Span>> + Send>>,
 }
 
 fn _assert_string_literal_result_is_send() {
@@ -498,7 +498,9 @@ impl<T: LspContext> Backend<T> {
                         let result =
                             self.get_ast_or_load_from_disk(&url)
                                 .and_then(|ast| match ast {
-                                    Some(module) => location_finder(&module.ast),
+                                    Some(module) => location_finder(&module.ast).map(|span| {
+                                        span.map(|span| module.ast.codemap.resolve_span(span))
+                                    }),
                                     None => Ok(None),
                                 });
                         if let Err(e) = &result {
@@ -1358,10 +1360,10 @@ mod test {
 
         let bar_contents = r#""Just <bar>a string</bar>""#;
         let bar = FixtureWithRanges::from_fixture(bar_uri.path(), bar_contents)?;
-        let bar_range = bar.resolved_span("bar");
-        let bar_range_str = format!(
-            "{}:{}:{}:{}",
-            bar_range.begin_line, bar_range.begin_column, bar_range.end_line, bar_range.end_column
+        let bar_resolved_span = bar.resolved_span("bar");
+        let bar_span_str = format!(
+            "{}:{}",
+            bar_resolved_span.begin_column, bar_resolved_span.end_column
         );
 
         let foo_contents = dedent(
@@ -1442,7 +1444,7 @@ mod test {
             "#,
         )
         .trim()
-        .replace("{bar_range}", &bar_range_str);
+        .replace("{bar_range}", &bar_span_str);
         let foo = FixtureWithRanges::from_fixture(foo_uri.path(), &foo_contents)?;
 
         let mut server = TestServer::new()?;
@@ -1451,7 +1453,7 @@ mod test {
 
         let mut test = |name: &str, expect_range: bool| -> anyhow::Result<()> {
             let range = if expect_range {
-                bar_range
+                bar_resolved_span
             } else {
                 Default::default()
             };

starlark/src/lsp/test.rs

@@ -47,8 +47,6 @@ use lsp_types::GotoCapability;
 use lsp_types::InitializeParams;
 use lsp_types::InitializeResult;
 use lsp_types::InitializedParams;
-use lsp_types::Position;
-use lsp_types::Range;
 use lsp_types::TextDocumentClientCapabilities;
 use lsp_types::TextDocumentContentChangeEvent;
 use lsp_types::TextDocumentItem;
@@ -57,6 +55,8 @@ use lsp_types::VersionedTextDocumentIdentifier;
 use maplit::hashmap;
 use serde::de::DeserializeOwned;
 
+use crate::codemap::Pos;
+use crate::codemap::Span;
 use crate::docs::render_docs_as_code;
 use crate::docs::Doc;
 use crate::docs::DocFunction;
@@ -170,20 +170,15 @@ impl LspContext for TestServerContext {
         literal: &str,
         current_file: &LspUrl,
     ) -> anyhow::Result<Option<StringLiteralResult>> {
-        let re = regex::Regex::new(r#"--(\d+):(\d+):(\d+):(\d+)$"#)?;
-        let (literal, range) = match re.captures(literal) {
+        let re = regex::Regex::new(r#"--(\d+):(\d+)$"#)?;
+        let (literal, span) = match re.captures(literal) {
             Some(cap) => {
-                let start_line = cap.get(1).unwrap().as_str().parse().unwrap();
-                let start_col = cap.get(2).unwrap().as_str().parse().unwrap();
-                let end_line = cap.get(3).unwrap().as_str().parse().unwrap();
-                let end_col = cap.get(4).unwrap().as_str().parse().unwrap();
-                let range = Range::new(
-                    Position::new(start_line, start_col),
-                    Position::new(end_line, end_col),
-                );
+                let start_pos = cap.get(1).unwrap().as_str().parse().unwrap();
+                let end_pos = cap.get(2).unwrap().as_str().parse().unwrap();
+                let span = Span::new(Pos::new(start_pos), Pos::new(end_pos));
                 (
                     literal[0..cap.get(0).unwrap().start()].to_owned(),
-                    Some(range),
+                    Some(span),
                 )
             }
             None => (literal.to_owned(), None),
@@ -193,7 +188,7 @@ impl LspContext for TestServerContext {
                 LspUrl::File(u) => match u.extension() {
                     Some(e) if e == "star" => Some(StringLiteralResult {
                         url,
-                        location_finder: Some(Box::new(move |_ast| Ok(range))),
+                        location_finder: Some(Box::new(move |_ast| Ok(span))),
                     }),
                     _ => Some(StringLiteralResult {
                         url,