Fix functionloc by adding line nodes to method signatures (JuliaLang#…

…35138) This comes in three pieces: * In the parser, put an additional LineNumberNode at the top of each function body to track the line of the function definition * In lowering, remove this LineNumberNode from the body and add it as the last parameter to method signature in `Expr(:method, name, sig, body)` * In the runtime, extract the line number node from sig and add it to the jl_method_t. Since the first line is now correctly tracked separately from the body, a special case in coverage codegen was removed. This was required when line numbers were tracked in a different way - see origin of this in the use of `toplineno` in bug JuliaLang#17442 and fix JuliaLang#17470.
oxinabox · Apr 8, 2020 · 4fe42f4 · 4fe42f4
1 parent ad893b9
commit 4fe42f4
Show file tree

Hide file tree

Showing 13 changed files with 72 additions and 44 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -63,6 +63,8 @@ Language changes
   Now the result is "a\n b", since the space before `b` is no longer considered to occur
   at the start of a line. The old behavior is considered a bug ([#35001]).
 
+* The line number of function definitions is now added by the parser as an
+  additional `LineNumberNode` at the start of each function body ([#35138]).
 
 Multi-threading changes
 -----------------------

diff --git a/doc/src/manual/constructors.md b/doc/src/manual/constructors.md
@@ -550,7 +550,7 @@ julia> struct SummedArray{T<:Number,S<:Number}
 julia> SummedArray(Int32[1; 2; 3], Int32(6))
 ERROR: MethodError: no method matching SummedArray(::Array{Int32,1}, ::Int32)
 Closest candidates are:
-  SummedArray(::Array{T,1}) where T at none:5
+  SummedArray(::Array{T,1}) where T at none:4
 ```
 
 This constructor will be invoked by the syntax `SummedArray(a)`. The syntax `new{T,S}` allows

diff --git a/doc/src/manual/types.md b/doc/src/manual/types.md
@@ -1207,8 +1207,7 @@ is raised:
 julia> supertype(Union{Float64,Int64})
 ERROR: MethodError: no method matching supertype(::Type{Union{Float64, Int64}})
 Closest candidates are:
-  supertype(!Matched::DataType) at operators.jl:42
-  supertype(!Matched::UnionAll) at operators.jl:47
+[...]
 ```
 
 ## [Custom pretty-printing](@id man-custom-pretty-printing)

diff --git a/src/codegen.cpp b/src/codegen.cpp
@@ -5820,10 +5820,13 @@ static std::pair<std::unique_ptr<Module>, jl_llvm_functions_t>
     auto coverageVisitStmt = [&] (size_t dbg) {
         if (dbg == 0)
             return;
+        // Compute inlining stack for current line, inner frame first
         while (dbg) {
             new_lineinfo.push_back(dbg);
             dbg = linetable.at(dbg).inlined_at;
         }
+        // Visit frames which differ from previous statement as tracked in
+        // current_lineinfo (tracked outer frame first).
         current_lineinfo.resize(new_lineinfo.size(), 0);
         for (dbg = 0; dbg < new_lineinfo.size(); dbg++) {
             unsigned newdbg = new_lineinfo[new_lineinfo.size() - dbg - 1];
@@ -5906,15 +5909,6 @@ static std::pair<std::unique_ptr<Module>, jl_llvm_functions_t>
         BB[label] = bb;
     }
 
-    if (do_coverage(mod_is_user_mod)) {
-        coverageVisitLine(ctx, ctx.file, toplineno);
-        if (linetable.size() >= 2) {
-            // avoid double-counting the entry line
-            const auto &info = linetable.at(1);
-            if (info.file == ctx.file && info.line == toplineno && info.is_user_code == mod_is_user_mod)
-                current_lineinfo.push_back(1);
-        }
-    }
     Value *sync_bytes = nullptr;
     if (do_malloc_log(true))
         sync_bytes = ctx.builder.CreateCall(prepare_call(diff_gc_total_bytes_func), {});

diff --git a/src/julia-parser.scm b/src/julia-parser.scm
@@ -1419,7 +1419,8 @@
               `(const ,assgn))))
 
        ((function macro)
-        (let* ((paren (eqv? (require-token s) #\())
+        (let* ((loc   (line-number-node s))
+               (paren (eqv? (require-token s) #\())
                (sig   (parse-def s (eq? word 'function) paren)))
           (if (and (not paren) (symbol-or-interpolate? sig))
               (begin (if (not (eq? (require-token s) 'end))
@@ -1440,7 +1441,7 @@
                                    sig)))
                      (body (parse-block s)))
                 (expect-end s word)
-                (list word def body)))))
+                (list word def (add-line-number body loc))))))
 
        ((abstract)
         (if (not (eq? (peek-token s) 'type))

diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm
@@ -231,7 +231,7 @@
   (let ((type-ex (caddr m)))
     (if (eq? (car type-ex) 'block)
         ;; extract ssavalue labels of sparams from the svec-of-sparams argument to `method`
-        (let ((sp-ssavals (cddr (last (last type-ex)))))
+        (let ((sp-ssavals (cddr (cadddr (last type-ex)))))
           (map (lambda (a)  ;; extract T from (= v (call (core TypeVar) (quote T) ...))
                  (cadr (caddr (caddr a))))
                (filter (lambda (e)
@@ -293,6 +293,24 @@
 (define (non-generated-version body)
   (generated-part- body #f))
 
+;; Remove and return the line number for the start of the function definition
+(define (maybe-remove-functionloc! body)
+  (let* ((prologue (extract-method-prologue body))
+         (prologue-lnos (filter linenum? prologue))
+         (functionloc (if (pair? prologue-lnos)
+                          (car prologue-lnos)
+                          ; Fallback - take first line anywhere in body
+                          (let ((lnos (filter linenum? body)))
+                            (if (null? lnos) '(line 0 none) (car lnos))))))
+    (if (length> prologue-lnos 1)
+        ; First of two line numbers in prologue is function definition location
+        ; which should be removed from the body.
+        (let loop ((stmts body))
+          (if (eq? functionloc (cadr stmts))
+              (set-cdr! stmts (cddr stmts))
+              (loop (cdr body)))))
+    functionloc))
+
 ;; construct the (method ...) expression for one primitive method definition,
 ;; assuming optional and keyword args are already handled
 (define (method-def-expr- name sparams argl body (rett '(core Any)))
@@ -328,12 +346,12 @@
          (error "function argument and static parameter names must be distinct"))
      (if (or (and name (not (sym-ref? name))) (not (valid-name? name)))
          (error (string "invalid function name \"" (deparse name) "\"")))
-     (let* ((generator (if (expr-contains-p if-generated? body (lambda (x) (not (function-def? x))))
+     (let* ((loc (maybe-remove-functionloc! body))
+            (generator (if (expr-contains-p if-generated? body (lambda (x) (not (function-def? x))))
                            (let* ((gen    (generated-version body))
                                   (nongen (non-generated-version body))
                                   (gname  (symbol (string (gensy) "#" (current-julia-module-counter))))
-                                  (gf     (make-generator-function gname names anames gen))
-                                  (loc    (function-body-lineno body)))
+                                  (gf     (make-generator-function gname names anames gen)))
                              (set! body (insert-after-meta
                                          nongen
                                          `((meta generated
@@ -343,8 +361,8 @@
                                                       ,(if (null? sparams)
                                                            'nothing
                                                            (cons 'list (map car sparams)))
-                                                      ,(if (null? loc) 0 (cadr loc))
-                                                      (inert ,(if (null? loc) 'none (caddr loc)))
+                                                      ,(cadr loc)
+                                                      (inert ,(caddr loc))
                                                       (false))))))
                              (list gf))
                            '()))
@@ -356,7 +374,11 @@
             (renames (map cons names temps))
             (mdef
              (if (null? sparams)
-                 `(method ,name (call (core svec) (call (core svec) ,@(dots->vararg types)) (call (core svec)))
+                 `(method ,name
+                          (call (core svec)
+                                (call (core svec) ,@(dots->vararg types))
+                                (call (core svec))
+                                (inert ,loc))
                           ,body)
                  `(method ,name
                           (block
@@ -377,7 +399,8 @@
                                                       (map (lambda (ty)
                                                              (replace-vars ty renames))
                                                            types)))
-                                 (call (core svec) ,@temps)))
+                                 (call (core svec) ,@temps)
+                                 (inert ,loc)))
                           ,body))))
        (if (or (symbol? name) (globalref? name))
            `(block ,@generator (method ,name) ,mdef (unnecessary ,name))  ;; return the function
@@ -793,10 +816,6 @@
                                     wheres)
                       ,(ctor-body body curlyargs sparams))))))
 
-(define (function-body-lineno body)
-  (let ((lnos (filter linenum? body)))
-    (if (null? lnos) '() (car lnos))))
-
 ;; rewrite calls to `new( ... )` to `new` expressions on the appropriate
 ;; type, determined by the containing constructor definition.
 (define (rewrite-ctor ctor Tname params field-names field-types)
@@ -3021,9 +3040,12 @@ f(x) = yt(x)
          (newtypes
           (if iskw
               `(,(car types) ,(cadr types) ,closure-type ,@(cdddr types))
-              `(,closure-type ,@(cdr types)))))
-    `(call (core svec) (call (core svec) ,@newtypes)
-           (call (core svec) ,@(append (cddr (cadddr te)) type-sp)))))
+              `(,closure-type ,@(cdr types))))
+         (loc (caddddr te)))
+    `(call (core svec)
+           (call (core svec) ,@newtypes)
+           (call (core svec) ,@(append (cddr (cadddr te)) type-sp))
+           ,loc)))
 
 ;; collect all toplevel-butfirst expressions inside `e`, and return
 ;; (ex . stmts), where `ex` is the expression to evaluated and

diff --git a/src/method.c b/src/method.c
@@ -504,12 +504,6 @@ static void jl_method_set_source(jl_method_t *m, jl_code_info_t *src)
     jl_array_t *stmts = (jl_array_t*)src->code;
     size_t i, n = jl_array_len(stmts);
     copy = jl_alloc_vec_any(n);
-    // set location from first LineInfoNode
-    if (jl_array_len(src->linetable) > 0) {
-        jl_value_t *ln = jl_array_ptr_ref(src->linetable, 0);
-        m->file = (jl_sym_t*)jl_fieldref(ln, 1);
-        m->line = jl_unbox_long(jl_fieldref(ln, 2));
-    }
     for (i = 0; i < n; i++) {
         jl_value_t *st = jl_array_ptr_ref(stmts, i);
         if (jl_is_expr(st) && ((jl_expr_t*)st)->head == meta_sym) {
@@ -698,9 +692,10 @@ JL_DLLEXPORT void jl_method_def(jl_svec_t *argdata,
                                 jl_code_info_t *f,
                                 jl_module_t *module)
 {
-    // argdata is svec(svec(types...), svec(typevars...))
+    // argdata is svec(svec(types...), svec(typevars...), functionloc)
     jl_svec_t *atypes = (jl_svec_t*)jl_svecref(argdata, 0);
     jl_svec_t *tvars = (jl_svec_t*)jl_svecref(argdata, 1);
+    jl_value_t *functionloc = jl_svecref(argdata, 2);
     size_t nargs = jl_svec_len(atypes);
     int isva = jl_is_vararg_type(jl_svecref(atypes, nargs - 1));
     assert(jl_is_svec(atypes));
@@ -755,6 +750,10 @@ JL_DLLEXPORT void jl_method_def(jl_svec_t *argdata,
     m->name = name;
     m->isva = isva;
     m->nargs = nargs;
+    assert(jl_is_linenode(functionloc));
+    jl_value_t *file = jl_linenode_file(functionloc);
+    m->file = jl_is_symbol(file) ? (jl_sym_t*)file : empty_sym;
+    m->line = jl_linenode_line(functionloc);
     jl_method_set_source(m, f);
 
     if (jl_has_free_typevars(argtype)) {

diff --git a/src/toplevel.c b/src/toplevel.c
@@ -830,6 +830,8 @@ JL_DLLEXPORT jl_value_t *jl_toplevel_eval_in(jl_module_t *m, jl_value_t *ex)
     jl_value_t *v = NULL;
     int last_lineno = jl_lineno;
     const char *last_filename = jl_filename;
+    jl_lineno = 1;
+    jl_filename = "none";
     if (jl_options.incremental && jl_generating_output()) {
         if (!ptrhash_has(&jl_current_modules, (void*)m)) {
             if (m != jl_main_module) { // TODO: this was grand-fathered in

diff --git a/test/reflection.jl b/test/reflection.jl
@@ -288,6 +288,7 @@ let rts = return_types(TLayout)
 end
 
 # issue #15447
+f15447_line = @__LINE__() + 1
 @noinline function f15447(s, a)
     if s
         return a
@@ -296,15 +297,15 @@ end
         return nb
     end
 end
-@test functionloc(f15447)[2] > 0
+@test functionloc(f15447)[2] == f15447_line
 
 # issue #14346
 @noinline function f14346(id, mask, limit)
     if id <= limit && mask[id]
         return true
     end
 end
-@test functionloc(f14346)[2] == @__LINE__() - 4
+@test functionloc(f14346)[2] == @__LINE__() - 5
 
 # issue #15714
 # show variable names for slots and suppress spurious type warnings

diff --git a/test/show.jl b/test/show.jl
@@ -922,6 +922,7 @@ end"""
 end""")) ==
 """
 :(macro m(a, b)
+      #= none:1 =#
       #= none:2 =#
       quote
           #= none:3 =#
@@ -944,7 +945,7 @@ end"""))) ==
 """macro m(a, b)
     :(\$a + \$b)
 end""")) ==
-":(macro m(a, b)\n      #= none:2 =#\n      :(\$a + \$b)\n  end)"
+":(macro m(a, b)\n      #= none:1 =#\n      #= none:2 =#\n      :(\$a + \$b)\n  end)"
 @test repr(Expr(:macro, Expr(:call, :m, :x), Expr(:quote, Expr(:call, :+, Expr(:($), :x), 1)))) ==
 ":(macro m(x)\n      :(\$x + 1)\n  end)"
 

diff --git a/test/syntax.jl b/test/syntax.jl
@@ -162,11 +162,12 @@ macro test999_str(args...); args; end
 @test parseall(":(a = &\nb)") == Expr(:quote, Expr(:(=), :a, Expr(:&, :b)))
 @test parseall(":(a = \$\nb)") == Expr(:quote, Expr(:(=), :a, Expr(:$, :b)))
 
-# issue 11970
+# issue 12027 - short macro name parsing vs _str suffix
 @test parseall("""
     macro f(args...) end; @f "macro argument"
 """) == Expr(:toplevel,
-             Expr(:macro, Expr(:call, :f, Expr(:..., :args)), Expr(:block, LineNumberNode(1, :none))),
+             Expr(:macro, Expr(:call, :f, Expr(:..., :args)),
+                  Expr(:block, LineNumberNode(1, :none), LineNumberNode(1, :none))),
              Expr(:macrocall, Symbol("@f"), LineNumberNode(1, :none), "macro argument"))
 
 # blocks vs. tuples
@@ -1423,6 +1424,7 @@ let ex = Meta.lower(@__MODULE__, Meta.parse("
     @test isa(ex, Expr) && ex.head === :error
     @test ex.args[1] == """
 invalid assignment location "function (s, o...)
+    # none, line 2
     # none, line 3
     f(a, b) do
         # none, line 4

diff --git a/test/testhelpers/coverage_file.info b/test/testhelpers/coverage_file.info
@@ -7,6 +7,7 @@ DA:9,5
 DA:11,1
 DA:12,1
 DA:14,0
-LH:6
-LF:8
+DA:17,1
+LH:7
+LF:9
 end_of_record
diff --git a/test/testhelpers/coverage_file.jl b/test/testhelpers/coverage_file.jl
@@ -14,6 +14,10 @@ function code_coverage_test()
     not_reached
 end
 
-exit(code_coverage_test() == [1, 2, 3] ? 0 : 1)
+short_form_func_coverage_test(x) = x*x
+
+success = code_coverage_test() == [1, 2, 3] &&
+          short_form_func_coverage_test(2) == 4
+exit(success ?  0 : 1)
 
 # end of file
-Original file line number
+Diff line change
@@ Expand Up / @@ -7,6 +7,7 @@ DA:9,5 @@
     DA:11,1
     DA:12,1
     DA:14,0
-    LH:6
-    LF:8
+    DA:17,1
+    LH:7
+    LF:9
     end_of_record