search

florence / cover

a code coverage tool for racket
MIT License
38 stars 7 forks source link

vector-ref and vector-set! are not marked as covered in typed/racket #115

Closed SuzanneSoy closed 8 years ago

SuzanneSoy commented 8 years ago 
#lang typed/racket
(vector-ref (ann (vector 'a) (Vectorof Any)) 0)
(vector-ref (vector 'a) 0)

In the file above, the vector-ref identifier on the first line is marked as not covered (but not on the second). I believe this is because it gets expanded to the following code, where the syntax/loc isn't copied over to unsafe-vector-ref and unsafe-vector*-ref, as can be seen in the macro debugger.

(let ([new-i (quote 0)] [new-v (#%expression:69 (#%app:57 vector 'a))])
     (if (impersonator? new-v)
       (if (unsafe-fx< new-i (unsafe-vector-length new-v)) (unsafe-vector-ref new-v new-i) (vector-ref new-v new-i))
       (if (unsafe-fx< new-i (unsafe-vector*-length new-v)) (unsafe-vector*-ref new-v new-i) (vector-ref new-v new-i))))

When the size is known, the code is instead:

(unsafe-vector-ref (#%app vector 'a) (quote 0))

There too, the unsafe-vector-ref identifier doesn't have the proper source location, but somehow cover manages to mark it as covered.

The problem occurs too with vector-ref! when the size isn't known.

I made a patch for typed/racket to add the source location, but I'm not sure if that's enough: https://github.com/jsmaniac/typed-racket/tree/fix-coverage-vector-access . If you think that would solve the problem, I'll submit a pull request to typed/racket.

florence commented 8 years ago

There have been a ton of optimizer/src loc issues in the past, and this looks like another one. And looks like all the other fixes we did. If you see correct coverage with that patch then its probably good enough!

SuzanneSoy commented 8 years ago

@florence I've had a bit of trouble running a local version of typed/racket, so I might wait a bit before I try it out.

florence commented 8 years ago

@jsmaniac we're in agreement this is TR bug right?

SuzanneSoy commented 8 years ago

@florence Yes, the coverage is correct when using #lang racket. I haven't really had the time to check if this patch (https://github.com/jsmaniac/typed-racket/tree/fix-coverage-vector-access) fixes the coverage, sorry. Should I submit a PR anyway?

florence commented 8 years ago

Probably! Either way that would drop coverage eventually.