Feature: Evaluate Logical Truthiness

[adrian-burlacu-software]

Istanbul is a great tool that I intend on using :D Yet, it fails to report the simple matter of logical truthiness of logical expressions.

For example, expressions such as:

x && y

This does report coverage of X and not Y even when X is false. But I want to know if these branches are ever able to become true.

Another simple example:

x || y

When Y is true, the coverage of X is reported even though it never becomes true.

This change handles these cases in a new counter that piggybacks on the branches count, called branchesTrue(bT).

Some caveats:

• Minor version bump, istanbul-lib-instrument depends on the new istanbul-lib-coverage.
• Zero configuration, this always runs.
• Evaluates the expression, so the code should not mutate state in a logical expression.
• Only for logical expressions, otherwise the instrumented code needs to save the state to a temp variable(memoization), and re-evaluate its truthiness.

Thanks to the team working on this beloved library, I hope you find the time and willingness to merge this tested and necessary pull request into the master branch.

[bcoe]

@adrian-burlacu-software I'm open to an extension like this, but I think there are two things I'd like to see first:

1. we don't have the current Istanbul format documented in this repo (I don't believe.); adding a markdown file that describes it in docs/ would be valuable for implementers.
2. this change has the potential to impact performance on large codebases, for a feature only some people require. I suggest we make it a configuration setting that downstream tooling can enable.

[adrian-burlacu-software]

@bcoe Thanks, will do!

[adrian-burlacu-software]

@bcoe OK, I think all the requested changes are committed. Please take a look at:

1. docs/raw-output.md Describes the Istanbul format. I do indeed recall looking at v.1 for help with this.
2. Check out the packages/...-instrument/api.md as I have introduced the opts.reportLogic (optional, default: false) option. Tests conform to this :)

The checks won't be successful likely because of the istanbul-lib-converage dependency 🥇

[bcoe]

The checks won't be successful likely because of the istanbul-lib-converage dependency

Could we split the changes to the instrumenter out into a separate PR, that way we can release a new version of the instrumenter and ensure tests pass for the updated istanbul-lib-coverage.

[adrian-burlacu-software]

@bcoe
All committed and blocking on publishing the istanbul-lib-coverage pull request.

[bcoe]

This was clearly in the caveats. Don't expect to update state and evaluate logical expressions at the same time if you expect to allow for straightforward analysis.

Apologies missed this.

Yes, this is evaluating the expression once for counting, once for the program. Ok,

I think I was misunderstanding, reading your code you represent the statement as this right:

a() && b ➡️ (a() && (cov.bT++, null)) && (b && (cov.bT++, null))

a() || b ➡️ (a() && (cov.bT++, null) || (b && (cov.bT++, null))

This would only evaluate a() once right?

[bcoe]

In fact, I wrote Fast-Fuzz, a coverage-guided fuzzing tool based on this commit, and it's on V2 already.

@adrian-burlacu-software that's quite cool, I'd love to play with try this out on some of my libraries.

[adrian-burlacu-software]

This was clearly in the caveats. Don't expect to update state and evaluate logical expressions at the same time if you expect to allow for straightforward analysis.

Apologies missed this.

Yes, this is evaluating the expression once for counting, once for the program. Ok,

I think I was misunderstanding, reading your code you represent the statement as this right:

a() && b ➡️ (a() && (cov.bT++, null)) && (b && (cov.bT++, null))

a() || b ➡️ (a() && (cov.bT++, null) || (b && (cov.bT++, null))

This would only evaluate a() once right?

@bcoe
What a neat idea :O No I feel like I will need to change if from the dumb way I was doing it:
(a() ? cov.bT++ : null), a(). See the Babel spec here.

As soon as I can, I'll update to your suggested method with conjuction, fix tests, also make a new set of tests, and move the options parameter to last. This way no variable is needed and there's no caveats -- state change and evaluation will be able to happen simultaneously too.

Wow, thanks so much for your input! Now I'll definitely need to update my Fast-Fuzz library with this update.

[bcoe]

@adrian-burlacu-software I think I'm slightly off, you'd actually want the terminal value to always be truthy right:

(a() && (cov.bT++, true) || (b && (cov.bT++, true))

☝️ I think that would work for logical operations, with the same logical output, and only evaluate once 😄

[adrian-burlacu-software]

@adrian-burlacu-software I think I'm slightly off, you'd actually want the terminal value to always be truthy right:

(a() && (cov.bT++, true) || (b && (cov.bT++, true))

☝️ I think that would work for logical operations, with the same logical output, and only evaluate once 😄

@bcoe
Was a good idea but the && operator actually returns the second(not the first) operand. This breaks a test: truthy/or with object expression (part 2).

I have tried the following:
(cov.b++, function() {const varName_temp = EXPRESSION; if(varName_temp) { cov.bT++;} return varName_temp; }())

The bad thing is now I had to detect await somewhere in the AST so that I can await an async function that wraps the original await...OR detect an async function context so that I can just await no matter what the expression.

I also tried to set up variables, but this is not a valid expression because variable declarations have to be hoisted:
(let varName_temp = EXPRESSION; a ? cov.bT++ : null, varName_temp)

So now I'm just going to reuse a variable this.fileVarName[fileVarName_temp} and make the mutate+evaluate test that passes.

Everything is done on my end. That being said, I can't wait for any more ideas/comments. Also hope we can finally land this one soon too given it's behind an option that's off by default.

Cheerio and thank you kindly :D

[adrian-burlacu-software]

@bcoe
TLDR: Everything should be done.

[bcoe]

@adrian-burlacu-software I'm really happy with where this landed 👍

I'm excited that code coverage should be exactly as efficient, but you can now use Istanbul for a fuzzing tool.

[bcoe]

@adrian-burlacu-software great work on this one, I'm going to go ahead and release, let me know if it works for you.

[adrian-burlacu-software]

@bcoe
Thank you, this sounds great! Was a great pleasure working with you! I'll try to figure out how to pass in the reportLogic argument and reference this from NPM on my Fast-Fuzz.

[adrian-burlacu-software]

@bcoe
Fast-Fuzz has been updated with this fix and all the tests pass. Feel free to check out the new features on my fuzzer, it's now been tested more realistically.

Thanks again for helping me with all the suggestions :)