Merge pull request #135 from immunant/feature/comment_spans

Track comments by source code span
immunant · Aug 2, 2019 · e2dc471 · e2dc471
2 parents 7187de4 + 1fab638
commit e2dc471
Show file tree

Hide file tree

Showing 21 changed files with 984 additions and 460 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/c2rust-ast-exporter/src/AstExporter.cpp b/c2rust-ast-exporter/src/AstExporter.cpp
@@ -472,9 +472,9 @@ class TranslateASTVisitor final
     }
 
     // Template required because Decl and Stmt don't share a common base class
-    void encode_entry_raw(void *ast, ASTEntryTag tag, SourceLocation loc,
-                          const QualType ty, bool rvalue, bool isVaList,
-                          bool encodeMacroExpansions,
+    void encode_entry_raw(void *ast, ASTEntryTag tag, SourceRange loc,
+                          const QualType ty, bool rvalue,
+                          bool isVaList, bool encodeMacroExpansions,
                           const std::vector<void *> &childIds,
                           std::function<void(CborEncoder *)> extra) {
         if (!markForExport(ast, tag))
@@ -501,17 +501,19 @@ class TranslateASTVisitor final
         cbor_encoder_close_container(&local, &childEnc);
 
         // 3 - File number
-        // 4 - Line number
-        // 5 - Column number
-        encodeSourcePos(&local, loc, isVaList);
+        // 4 - Begin Line number
+        // 5 - Begin Column number
+        // 6 - End Line number
+        // 7 - End Column number
+        encodeSourceSpan(&local, loc, isVaList);
 
-        // 6 - Type ID (only for expressions)
+        // 8 - Type ID (only for expressions)
         encode_qualtype(&local, ty);
 
-        // 7 - Is Rvalue (only for expressions)
+        // 9 - Is Rvalue (only for expressions)
         cbor_encode_boolean(&local, rvalue);
 
-        // 8 - Macro expansion stack, starting with initial macro call and ending
+        // 10 - Macro expansion stack, starting with initial macro call and ending
         // with the innermost replacement.
         cbor_encoder_create_array(&local, &childEnc,
                                   encodeMacroExpansions ? curMacroExpansionStack.size() : 0);
@@ -523,7 +525,7 @@ class TranslateASTVisitor final
         }
         cbor_encoder_close_container(&local, &childEnc);
 
-        // 9.. - Extra entries
+        // 11.. - Extra entries
         extra(&local);
 
         cbor_encoder_close_container(encoder, &local);
@@ -543,13 +545,8 @@ class TranslateASTVisitor final
         auto ty = ast->getType();
         auto isVaList = false;
         auto encodeMacroExpansions = true;
-#if CLANG_VERSION_MAJOR < 8
-        SourceLocation loc = ast->getLocStart();
-#else
-        SourceLocation loc = ast->getBeginLoc();
-#endif // CLANG_VERSION_MAJOR
-        encode_entry_raw(ast, tag, loc, ty, ast->isRValue(), isVaList, encodeMacroExpansions,
-                         childIds, extra);
+        encode_entry_raw(ast, tag, ast->getSourceRange(), ty, ast->isRValue(), isVaList,
+                         encodeMacroExpansions, childIds, extra);
         typeEncoder.VisitQualType(ty);
     }
 
@@ -560,13 +557,8 @@ class TranslateASTVisitor final
         auto rvalue = false;
         auto isVaList = false;
         auto encodeMacroExpansions = false;
-#if CLANG_VERSION_MAJOR < 8
-        SourceLocation loc = ast->getLocStart();
-#else
-        SourceLocation loc = ast->getBeginLoc();
-#endif // CLANG_VERSION_MAJOR
-        encode_entry_raw(ast, tag, loc, s, rvalue, isVaList, encodeMacroExpansions,
-                         childIds, extra);
+        encode_entry_raw(ast, tag, ast->getSourceRange(), s, rvalue, isVaList,
+                         encodeMacroExpansions, childIds, extra);
     }
 
     void encode_entry(
@@ -575,15 +567,15 @@ class TranslateASTVisitor final
         std::function<void(CborEncoder *)> extra = [](CborEncoder *) {}) {
         auto rvalue = false;
         auto encodeMacroExpansions = false;
-        encode_entry_raw(ast, tag, ast->getLocation(), T, rvalue,
+        encode_entry_raw(ast, tag, ast->getSourceRange(), T, rvalue,
                          isVaList(ast, T), encodeMacroExpansions, childIds, extra);
     }
 
     /// Explicitly override the source location of this decl for cases where the
     /// definition location is not the same as the canonical declaration
     /// location.
     void encode_entry(
-        Decl *ast, ASTEntryTag tag, SourceLocation loc,
+        Decl *ast, ASTEntryTag tag, SourceRange loc,
         const std::vector<void *> &childIds, const QualType T,
         std::function<void(CborEncoder *)> extra = [](CborEncoder *) {}) {
         auto rvalue = false;
@@ -714,7 +706,8 @@ class TranslateASTVisitor final
             std::vector<void *> childIds(Info.Expressions.begin(),
                                          Info.Expressions.end());
 
-            encode_entry_raw(Mac, tag, Mac->getDefinitionLoc(), QualType(), false,
+            auto range = SourceRange(Mac->getDefinitionLoc(), Mac->getDefinitionEndLoc());
+            encode_entry_raw(Mac, tag, range, QualType(), false,
                              false, false, childIds, [Name](CborEncoder *local) {
                                  cbor_encode_string(local, Name.str());
                              });
@@ -731,15 +724,41 @@ class TranslateASTVisitor final
             manager.isMacroBodyExpansion(loc))
             loc = manager.getFileLoc(loc);
 
+        auto fileid = getExporterFileId(manager.getFileID(loc), isVaList);
         auto line = manager.getPresumedLineNumber(loc);
         auto col = manager.getPresumedColumnNumber(loc);
-        auto fileid = getExporterFileId(manager.getFileID(loc), isVaList);
 
         cbor_encode_uint(enc, fileid);
         cbor_encode_uint(enc, line);
         cbor_encode_uint(enc, col);
     }
 
+    void encodeSourceSpan(CborEncoder *enc, SourceRange loc, bool isVaList = false) {
+        auto &manager = Context->getSourceManager();
+
+        auto begin = loc.getBegin();
+        auto end = loc.getEnd();
+        // A check to see if the Source Location is a Macro
+        if (manager.isMacroArgExpansion(begin) ||
+            manager.isMacroBodyExpansion(begin))
+            begin = manager.getFileLoc(begin);
+        if (manager.isMacroArgExpansion(end) ||
+            manager.isMacroBodyExpansion(end))
+            end = manager.getFileLoc(end);
+
+        auto fileid = getExporterFileId(manager.getFileID(begin), isVaList);
+        auto begin_line = manager.getPresumedLineNumber(begin);
+        auto begin_col = manager.getPresumedColumnNumber(begin);
+        auto end_line = manager.getPresumedLineNumber(end);
+        auto end_col = manager.getPresumedColumnNumber(end);
+
+        cbor_encode_uint(enc, fileid);
+        cbor_encode_uint(enc, begin_line);
+        cbor_encode_uint(enc, begin_col);
+        cbor_encode_uint(enc, end_line);
+        cbor_encode_uint(enc, end_col);
+    }
+
     uint64_t getExporterFileId(FileID id, bool isVaList) {
         if (id.isInvalid())
             return 0;
@@ -1572,10 +1591,10 @@ class TranslateASTVisitor final
         if (!FD->isCanonicalDecl()) {
             // Emit non-canonical decl so we have a placeholder to attach comments to
             std::vector<void *> childIds = {FD->getCanonicalDecl()};
-            auto loc = FD->getLocation();
+            auto span = FD->getSourceRange();
             if (FD->doesThisDeclarationHaveABody())
-                loc = FD->getCanonicalDecl()->getLocation();
-            encode_entry(FD, TagNonCanonicalDecl, loc, childIds, FD->getType());
+                span = FD->getCanonicalDecl()->getSourceRange();
+            encode_entry(FD, TagNonCanonicalDecl, span, childIds, FD->getType());
             return true;
         }
 
@@ -1600,8 +1619,9 @@ class TranslateASTVisitor final
         childIds.push_back(body);
 
         auto functionType = FD->getType();
+        auto span = paramsFD->getSourceRange();
         encode_entry(
-            FD, TagFunctionDecl, paramsFD->getLocation(), childIds, functionType,
+            FD, TagFunctionDecl, span, childIds, functionType,
             [this, FD](CborEncoder *array) {
                 auto name = FD->getNameAsString();
                 cbor_encode_string(array, name);

diff --git a/c2rust-ast-exporter/src/clang_ast.rs b/c2rust-ast-exporter/src/clang_ast.rs
@@ -22,18 +22,56 @@ impl LRValue {
     }
 }
 
-#[derive(Debug, Clone, PartialOrd, PartialEq, Ord, Eq)]
+#[derive(Copy, Debug, Clone, PartialOrd, PartialEq, Ord, Eq)]
 pub struct SrcLoc {
     pub fileid: u64,
     pub line: u64,
     pub column: u64,
 }
 
+#[derive(Copy, Debug, Clone, PartialOrd, PartialEq, Ord, Eq)]
+pub struct SrcSpan {
+    pub fileid: u64,
+    pub begin_line: u64,
+    pub begin_column: u64,
+    pub end_line: u64,
+    pub end_column: u64,
+}
+
+impl From<SrcLoc> for SrcSpan {
+    fn from(loc: SrcLoc) -> Self {
+        Self {
+            fileid: loc.fileid,
+            begin_line: loc.line,
+            begin_column: loc.column,
+            end_line: loc.line,
+            end_column: loc.column,
+        }
+    }
+}
+
+impl SrcSpan {
+    pub fn begin(&self) -> SrcLoc {
+        SrcLoc {
+            fileid: self.fileid,
+            line: self.begin_line,
+            column: self.begin_column,
+        }
+    }
+    pub fn end(&self) -> SrcLoc {
+        SrcLoc {
+            fileid: self.fileid,
+            line: self.end_line,
+            column: self.end_column,
+        }
+    }
+}
+
 #[derive(Debug, Clone)]
 pub struct AstNode {
     pub tag: ASTEntryTag,
     pub children: Vec<Option<u64>>,
-    pub loc: SrcLoc,
+    pub loc: SrcSpan,
     pub type_id: Option<u64>,
     pub rvalue: LRValue,
 
@@ -152,10 +190,10 @@ pub fn process(items: Value) -> error::Result<AstContext> {
                 .map(|x| expect_opt_u64(x).unwrap())
                 .collect::<Vec<Option<u64>>>();
 
-            let type_id: Option<u64> = expect_opt_u64(&entry[6]).unwrap();
+            let type_id: Option<u64> = expect_opt_u64(&entry[8]).unwrap();
             let fileid = entry[3].as_u64().unwrap();
 
-            let macro_expansions = entry[8]
+            let macro_expansions = entry[10]
                 .as_array()
                 .unwrap()
                 .iter()
@@ -165,19 +203,21 @@ pub fn process(items: Value) -> error::Result<AstContext> {
             let node = AstNode {
                 tag: import_ast_tag(tag),
                 children,
-                loc: SrcLoc {
+                loc: SrcSpan {
                     fileid,
-                    line: entry[4].as_u64().unwrap(),
-                    column: entry[5].as_u64().unwrap(),
+                    begin_line: entry[4].as_u64().unwrap(),
+                    begin_column: entry[5].as_u64().unwrap(),
+                    end_line: entry[6].as_u64().unwrap(),
+                    end_column: entry[7].as_u64().unwrap(),
                 },
                 type_id,
-                rvalue: if entry[7].as_boolean().unwrap() {
+                rvalue: if entry[9].as_boolean().unwrap() {
                     LRValue::RValue
                 } else {
                     LRValue::LValue
                 },
                 macro_expansions,
-                extras: entry[9..].to_vec(),
+                extras: entry[11..].to_vec(),
             };
 
             asts.insert(entry_id, node);

diff --git a/c2rust-refactor/src/rewrite/base.rs b/c2rust-refactor/src/rewrite/base.rs
@@ -220,7 +220,7 @@ where
             diff::Result::Left(_) => {
                 // There's an item on the left corresponding to nothing on the right.
                 // Delete the item from the left.
-                let old_span = rewind_span_over_whitespace(ast(&old[i]).splice_span(), &rcx);
+                let old_span = ast(&old[i]).splice_span();
                 // let old_span = ast(&old[i]).splice_span();
                 let old_span = match old_ids[i] {
                     SeqItemId::Node(id) => extend_span_comments(&id, old_span, &rcx),
@@ -482,7 +482,7 @@ pub fn extend_span_comments(id: &NodeId, span: Span, rcx: &RewriteCtxt) -> Span
 
 /// Extend a node span to cover comments around it.  Returns Ok(span) if all
 /// comments were covered, and Err(span) if only some could be covered.
-pub fn extend_span_comments_strict(id: &NodeId, span: Span, rcx: &RewriteCtxt) -> Result<Span, Span> {
+pub fn extend_span_comments_strict(id: &NodeId, mut span: Span, rcx: &RewriteCtxt) -> Result<Span, Span> {
     let source_map = rcx.session().source_map();
 
     let comments = match rcx.comments().get(id) {
@@ -516,11 +516,11 @@ pub fn extend_span_comments_strict(id: &NodeId, span: Span, rcx: &RewriteCtxt) -
 
     let mut all_matched = true;
 
-    let mut span = rewind_span_over_whitespace(span, rcx);
     for comment in &before {
+        let cur_span = rewind_span_over_whitespace(span, rcx);
         let comment_size = usize::sum(comment.lines.iter().map(|l| l.len()));
-        let comment_start = BytePos::from_usize(span.lo().to_usize() - comment_size);
-        let comment_span = span.shrink_to_lo().with_lo(comment_start);
+        let comment_start = BytePos::from_usize(cur_span.lo().to_usize() - comment_size);
+        let comment_span = cur_span.shrink_to_lo().with_lo(comment_start);
         let source = match source_map.span_to_snippet(comment_span) {
             Ok(snippet) => snippet,
             Err(_) => {
@@ -529,15 +529,14 @@ pub fn extend_span_comments_strict(id: &NodeId, span: Span, rcx: &RewriteCtxt) -
             }
         };
         let matches = source.lines().zip(&comment.lines).all(|(src_line, comment_line)| {
+            if src_line.trim() != comment_line.trim() {
+                debug!("comment {:?} did not match source {:?}", comment_line, src_line);
+            }
             src_line.trim() == comment_line.trim()
         });
         if matches {
-            // Extend to previous newline because this is an isolated comment
-            let comment_span = rewind_span_over_whitespace(comment_span, rcx);
-
-            span = span.with_lo(comment_span.lo());
+            span = cur_span.with_lo(comment_span.lo());
         } else {
-            debug!("comment {:?} did not match source {:?}", comment, source);
             all_matched = false;
             break;
         }

diff --git a/c2rust-transpile/Cargo.toml b/c2rust-transpile/Cargo.toml
@@ -27,6 +27,7 @@ handlebars = "1.1.0"
 itertools = "0.8"
 pathdiff = "0.1.0"
 regex = "1"
+smallvec = "0.6"
 strum = "0.15"
 strum_macros = "0.15"
 log = "0.4"

diff --git a/c2rust-transpile/src/c_ast/conversion.rs b/c2rust-transpile/src/c_ast/conversion.rs
@@ -343,7 +343,7 @@ impl ConversionContext {
     fn convert(&mut self, untyped_context: &AstContext) -> () {
         for raw_comment in &untyped_context.comments {
             let comment = Located {
-                loc: Some(raw_comment.loc.clone()),
+                loc: Some(raw_comment.loc.into()),
                 kind: raw_comment.string.clone(),
             };
             self.typed_context.comments.push(comment);