Rollup merge of rust-lang#24242 - nikomatsakis:escaping-closure-error…

…-message, r=brson Example showing sample inputs, old message, new message: https://gist.github.com/nikomatsakis/11126784ac678b7eb6ba Also adds infrastructure for reporting suggestions "in situ" and does some (minor) cleanups to `CodeMap`. r? @brson
steveklabnik · Apr 10, 2015 · c6ec2a2 · c6ec2a2
2 parents 702149c + e313b33
commit c6ec2a2
Show file tree

Hide file tree

Showing 11 changed files with 370 additions and 84 deletions.
diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs
@@ -143,6 +143,13 @@ impl Session {
     pub fn span_end_note(&self, sp: Span, msg: &str) {
         self.diagnostic().span_end_note(sp, msg)
     }
+
+    /// Prints out a message with a suggested edit of the code.
+    ///
+    /// See `diagnostic::RenderSpan::Suggestion` for more information.
+    pub fn span_suggestion(&self, sp: Span, msg: &str, suggestion: String) {
+        self.diagnostic().span_suggestion(sp, msg, suggestion)
+    }
     pub fn span_help(&self, sp: Span, msg: &str) {
         self.diagnostic().span_help(sp, msg)
     }

diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs
@@ -522,6 +522,16 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
     }
 
     pub fn report(&self, err: BckError<'tcx>) {
+        // Catch and handle some particular cases.
+        match (&err.code, &err.cause) {
+            (&err_out_of_scope(ty::ReScope(_), ty::ReStatic), &euv::ClosureCapture(span)) |
+            (&err_out_of_scope(ty::ReScope(_), ty::ReFree(..)), &euv::ClosureCapture(span)) => {
+                return self.report_out_of_scope_escaping_closure_capture(&err, span);
+            }
+            _ => { }
+        }
+
+        // General fallback.
         self.span_err(
             err.span,
             &self.bckerr_to_string(&err));
@@ -796,16 +806,10 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
                 format!("{} does not live long enough", msg)
             }
             err_borrowed_pointer_too_short(..) => {
-                let descr = match opt_loan_path(&err.cmt) {
-                    Some(lp) => {
-                        format!("`{}`", self.loan_path_to_string(&*lp))
-                    }
-                    None => self.cmt_to_string(&*err.cmt),
-                };
-
+                let descr = self.cmt_to_path_or_string(&err.cmt);
                 format!("lifetime of {} is too short to guarantee \
-                                its contents can be safely reborrowed",
-                               descr)
+                         its contents can be safely reborrowed",
+                        descr)
             }
         }
     }
@@ -888,6 +892,39 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
         }
     }
 
+    fn report_out_of_scope_escaping_closure_capture(&self,
+                                                    err: &BckError<'tcx>,
+                                                    capture_span: Span)
+    {
+        let cmt_path_or_string = self.cmt_to_path_or_string(&err.cmt);
+
+        span_err!(
+            self.tcx.sess, err.span, E0373,
+            "closure may outlive the current function, \
+             but it borrows {}, \
+             which is owned by the current function",
+            cmt_path_or_string);
+
+        self.tcx.sess.span_note(
+            capture_span,
+            &format!("{} is borrowed here",
+                     cmt_path_or_string));
+
+        let suggestion =
+            match self.tcx.sess.codemap().span_to_snippet(err.span) {
+                Ok(string) => format!("move {}", string),
+                Err(_) => format!("move |<args>| <body>")
+            };
+
+        self.tcx.sess.span_suggestion(
+            err.span,
+            &format!("to force the closure to take ownership of {} \
+                      (and any other referenced variables), \
+                      use the `move` keyword, as shown:",
+                     cmt_path_or_string),
+            suggestion);
+    }
+
     pub fn note_and_explain_bckerr(&self, err: BckError<'tcx>) {
         let code = err.code;
         match code {
@@ -1035,6 +1072,13 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
     pub fn cmt_to_string(&self, cmt: &mc::cmt_<'tcx>) -> String {
         cmt.descriptive_string(self.tcx)
     }
+
+    pub fn cmt_to_path_or_string(&self, cmt: &mc::cmt<'tcx>) -> String {
+        match opt_loan_path(cmt) {
+            Some(lp) => format!("`{}`", self.loan_path_to_string(&lp)),
+            None => self.cmt_to_string(cmt),
+        }
+    }
 }
 
 fn is_statement_scope(tcx: &ty::ctxt, region: ty::Region) -> bool {

diff --git a/src/librustc_borrowck/diagnostics.rs b/src/librustc_borrowck/diagnostics.rs
@@ -0,0 +1,17 @@
+// Copyright 2014 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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![allow(non_snake_case)]
+
+register_diagnostics! {
+    E0373 // closure may outlive current fn, but it borrows {}, which is owned by current fn
+}
+
+__build_diagnostic_array! { DIAGNOSTICS }
diff --git a/src/librustc_borrowck/lib.rs b/src/librustc_borrowck/lib.rs
@@ -40,6 +40,10 @@ pub use borrowck::check_crate;
 pub use borrowck::build_borrowck_dataflow_data_for_fn;
 pub use borrowck::FnPartsWithCFG;
 
+// NB: This module needs to be declared first so diagnostics are
+// registered before they are used.
+pub mod diagnostics;
+
 mod borrowck;
 
 pub mod graphviz;
diff --git a/src/libsyntax/codemap.rs b/src/libsyntax/codemap.rs
@@ -49,7 +49,7 @@ pub struct BytePos(pub u32);
 /// A character offset. Because of multibyte utf8 characters, a byte offset
 /// is not equivalent to a character offset. The CodeMap will convert BytePos
 /// values to CharPos values as necessary.
-#[derive(Copy, Clone, PartialEq, Hash, PartialOrd, Debug)]
+#[derive(Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Debug)]
 pub struct CharPos(pub usize);
 
 // FIXME: Lots of boilerplate in these impls, but so far my attempts to fix
@@ -305,9 +305,21 @@ impl ExpnId {
 
 pub type FileName = String;
 
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+pub struct LineInfo {
+    /// Index of line, starting from 0.
+    pub line_index: usize,
+
+    /// Column in line where span begins, starting from 0.
+    pub start_col: CharPos,
+
+    /// Column in line where span ends, starting from 0, exclusive.
+    pub end_col: CharPos,
+}
+
 pub struct FileLines {
     pub file: Rc<FileMap>,
-    pub lines: Vec<usize>
+    pub lines: Vec<LineInfo>
 }
 
 /// Identifies an offset of a multi-byte character in a FileMap
@@ -479,9 +491,9 @@ impl FileMap {
         lines.push(pos);
     }
 
-    /// get a line from the list of pre-computed line-beginnings
-    ///
-    pub fn get_line(&self, line_number: usize) -> Option<String> {
+    /// get a line from the list of pre-computed line-beginnings.
+    /// line-number here is 0-based.
+    pub fn get_line(&self, line_number: usize) -> Option<&str> {
         match self.src {
             Some(ref src) => {
                 let lines = self.lines.borrow();
@@ -492,7 +504,7 @@ impl FileMap {
                     match slice.find('\n') {
                         Some(e) => &slice[..e],
                         None => slice
-                    }.to_string()
+                    }
                 })
             }
             None => None
@@ -661,10 +673,29 @@ impl CodeMap {
     pub fn span_to_lines(&self, sp: Span) -> FileLines {
         let lo = self.lookup_char_pos(sp.lo);
         let hi = self.lookup_char_pos(sp.hi);
-        let mut lines = Vec::new();
-        for i in lo.line - 1..hi.line {
-            lines.push(i);
-        };
+        let mut lines = Vec::with_capacity(hi.line - lo.line + 1);
+
+        // The span starts partway through the first line,
+        // but after that it starts from offset 0.
+        let mut start_col = lo.col;
+
+        // For every line but the last, it extends from `start_col`
+        // and to the end of the line. Be careful because the line
+        // numbers in Loc are 1-based, so we subtract 1 to get 0-based
+        // lines.
+        for line_index in lo.line-1 .. hi.line-1 {
+            let line_len = lo.file.get_line(line_index).map(|s| s.len()).unwrap_or(0);
+            lines.push(LineInfo { line_index: line_index,
+                                  start_col: start_col,
+                                  end_col: CharPos::from_usize(line_len) });
+            start_col = CharPos::from_usize(0);
+        }
+
+        // For the last line, it extends from `start_col` to `hi.col`:
+        lines.push(LineInfo { line_index: hi.line - 1,
+                              start_col: start_col,
+                              end_col: hi.col });
+
         FileLines {file: lo.file, lines: lines}
     }
 
@@ -919,17 +950,18 @@ pub struct MalformedCodemapPositions {
 #[cfg(test)]
 mod test {
     use super::*;
+    use std::rc::Rc;
 
     #[test]
     fn t1 () {
         let cm = CodeMap::new();
         let fm = cm.new_filemap("blork.rs".to_string(),
                                 "first line.\nsecond line".to_string());
         fm.next_line(BytePos(0));
-        assert_eq!(fm.get_line(0), Some("first line.".to_string()));
+        assert_eq!(fm.get_line(0), Some("first line."));
         // TESTING BROKEN BEHAVIOR:
         fm.next_line(BytePos(10));
-        assert_eq!(fm.get_line(1), Some(".".to_string()));
+        assert_eq!(fm.get_line(1), Some("."));
     }
 
     #[test]
@@ -1057,7 +1089,54 @@ mod test {
 
         assert_eq!(file_lines.file.name, "blork.rs");
         assert_eq!(file_lines.lines.len(), 1);
-        assert_eq!(file_lines.lines[0], 1);
+        assert_eq!(file_lines.lines[0].line_index, 1);
+    }
+
+    /// Given a string like " ^~~~~~~~~~~~ ", produces a span
+    /// coverting that range. The idea is that the string has the same
+    /// length as the input, and we uncover the byte positions.  Note
+    /// that this can span lines and so on.
+    fn span_from_selection(input: &str, selection: &str) -> Span {
+        assert_eq!(input.len(), selection.len());
+        let left_index = selection.find('^').unwrap() as u32;
+        let right_index = selection.rfind('~').unwrap() as u32;
+        Span { lo: BytePos(left_index), hi: BytePos(right_index + 1), expn_id: NO_EXPANSION }
+    }
+
+    fn new_filemap_and_lines(cm: &CodeMap, filename: &str, input: &str) -> Rc<FileMap> {
+        let fm = cm.new_filemap(filename.to_string(), input.to_string());
+        let mut byte_pos: u32 = 0;
+        for line in input.lines() {
+            // register the start of this line
+            fm.next_line(BytePos(byte_pos));
+
+            // update byte_pos to include this line and the \n at the end
+            byte_pos += line.len() as u32 + 1;
+        }
+        fm
+    }
+
+    /// Test span_to_snippet and span_to_lines for a span coverting 3
+    /// lines in the middle of a file.
+    #[test]
+    fn span_to_snippet_and_lines_spanning_multiple_lines() {
+        let cm = CodeMap::new();
+        let inputtext = "aaaaa\nbbbbBB\nCCC\nDDDDDddddd\neee\n";
+        let selection = "     \n    ^~\n~~~\n~~~~~     \n   \n";
+        new_filemap_and_lines(&cm, "blork.rs", inputtext);
+        let span = span_from_selection(inputtext, selection);
+
+        // check that we are extracting the text we thought we were extracting
+        assert_eq!(&cm.span_to_snippet(span).unwrap(), "BB\nCCC\nDDDDD");
+
+        // check that span_to_lines gives us the complete result with the lines/cols we expected
+        let lines = cm.span_to_lines(span);
+        let expected = vec![
+            LineInfo { line_index: 1, start_col: CharPos(4), end_col: CharPos(6) },
+            LineInfo { line_index: 2, start_col: CharPos(0), end_col: CharPos(3) },
+            LineInfo { line_index: 3, start_col: CharPos(0), end_col: CharPos(5) }
+            ];
+        assert_eq!(lines.lines, expected);
     }
 
     #[test]