Provide autofixes for always_ff_non_blocking_rule

[IEncinas10]

This PR adds autofixes for every case currently flagged in always_ff_non_blocking_rule. I added a separate formatting commit just in case it made review easier.

Quick summary:

• Added getters for kAssignModifyStatements
• Added getters for kIncrementDecrementExpressions
• Added a getter for kNetVariableAssignments
  • I'm not sure if this should be placed in statement{.h,.cc,_test.cc}

I'm not 100% sure if the autofixes are ok as my experience with verilog is pretty small, so let me know if I missed something.

Outdated examples (*)

(*) Edit: see comments / test cases for updated information

As an example, applying autofixes would transform

always_ff @(posedge clk) begin
  c = b - a;
  c++;
  ++c;
  c &= (k + 1);
end

into this

always_ff @(posedge clk) begin
  c <= b - a;
  c <= c + 1;
  c <= c + 1;
  c <= c & (k + 1);
end

---

• Decide what to do with changes in code logic (try to substitute code, leave as is and let the user handle it)
• Fix parenthesis issues in x *= a + c

[codecov-commenter]

Codecov Report

Patch coverage: 98.96% and project coverage change: +0.01% 🎉

Comparison is base (0fc3a5e) 92.85% compared to head (6825e90) 92.87%.
Report is 32 commits behind head on master.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1912      +/-   ##
==========================================
+ Coverage   92.85%   92.87%   +0.01%     
==========================================
  Files         355      355              
  Lines       26272    26355      +83     
==========================================
+ Hits        24395    24477      +82     
- Misses       1877     1878       +1     

Files Changed	Coverage Δ
verilog/CST/expression.h	100.00% <ø> (ø)
common/analysis/linter_test_utils.h	96.00% <85.71%> (-1.88%)	⬇️
verilog/CST/expression.cc	96.77% <100.00%> (+0.57%)	⬆️
verilog/CST/statement.cc	97.88% <100.00%> (+0.09%)	⬆️
...g/analysis/checkers/always_ff_non_blocking_rule.cc	97.24% <100.00%> (+1.63%)	⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dbesse]

Does it add parentheses if needed for priority of operation?

c *= k + 1;

into

c <= c * (k + 1);

whereas without it would mean something logically different

c <= c * k + 1; // (c*k)+1

[IEncinas10]

Missed that, thanks for the catch! I guess wrapping the whole right hand side expression into a parenthesis would be always correct, right?

The fix is very simple. I'll add a check and avoid adding two parenthesis levels if the rhs is already between parenthesis.

To clarify:

a *= b + 1;
a *= (b + 1);

would both become

a <= a * (b + 1);
a <= a * (b + 1);

[dbesse]

Actually, even just changing the blocking to non-blocking (or reversely) may change the logic. I fully understand that it would report as a lint message, but this

  always_ff @(posedge clk) begin
    x++;
    y <= x;
  end

would do something different than the fixed version

  always_ff @(posedge clk) begin
    x <= x + 1;
    y <= x;
  end

Depending on the policy for autofix, we might actually need the user to provide the correct fix

  always_ff @(posedge clk) begin
    x <= x + 1;
    y <= x + 1;
  end

[IEncinas10]

Yeah, I see could be a problem. My initial thought was to just provide the quick fix and rely on the user to fix the logic changes.

Otherwise, if we modify an blocking assignment ( x=..., ++a, a++, x &=...) we would need to search the whole always_ff block and substitute every reference to x for the appropiate update, depending on if it is before the blocking assignment or after. I don't know how hard that could become.

Are blocking assignments even well defined inside always_ff blocks? If they're not, I guess we could provide the naive autofix and suggest the user to check the logic?

[hzeller]

I think we should leave it up to the user to recognize that they have some more work to do when they apply a fix mentioned in #1912 (comment) - so I think it is fine if we don't try to over-analyze the whole situation and fix everything.

The only thing we could do maybe later is to recognize that x (in this case) is used later in the same block as RHS and thus we might not offer the auto-fix at all as it would be wrong, just reporting the lint error.

[hzeller]

Adding @snsokolov for verilog expertise: do you think the above auto-fix suggestions would be reasonable and correct ?

[hzeller]

why underscore in the operator_ name ?

[IEncinas10]

operator is a reserved keyword, so I couldn't use it.

[hzeller]

The corner cases in #1912 (comment) are interesting

• if the RHS only is a tree with one leaf ( a *= b ), we don't have to worry about parenthizing
• if RHS has an operator with the same precedence, then one could think that parenthesis might not be needed. However, there are cases in which it matters (e.g adding real values a + (b + c) might result in different results as a + b + c. So adding parenthesis here is also a good idea. So bottom line: if the RHS is more than just a leaf, adding parenthesis is correct (and let the user remove them when needed).

[snsokolov]

Adding @snsokolov for verilog expertise: do you think the above auto-fix suggestions would be reasonable and correct ?

As it was noted above, changing blocking to non-blocking could be harmless, but could also change the user's logic in many cases. I don't have a good intuition about what percentage of lint violations the proposed autofix would break the code. I feel that it could be relatively high. When there is an unary operator or same variable is assigned and used - the chances of breaking the code will be very high.

So answering your question: The suggestion to auto-fix these violations is reasonable in most simple cases, though in some percentage of cases the auto-fixed code will stop behaving the same way.

[IEncinas10]

Thanks everyone for the reviews!

I'll work on the parenthesis thing, looked at it and shouldn't be too difficult.

Regarding the autofix problems, I agree that the safest way to proceed is to detect if the change (blocking -> nonblocking) would change code behaviour by checking if the affected variable is read somewhere else inside the always block, and in that case don't suggest the autofix. Not 100% sure how easy this will be, but I guess it won't be too much work.

always_ff @(posedge clk) begin
  x++;
  y <= x;
end

I think doing the correct autofix in every cornercase could be worth it although I will probably try it in a separate PR.

Update:

Should be ready for review soon

The approach was the following:

1. Collect every kReference node inside the always_ff block
2. For every violation, check if there are references AFTER the offending assignment (++, =, {&,|,...}=)

I'm relying on the fact that SearchSyntaxTree will return the references inside the always_ff block in program order (I guess this is ok?)

This check flags k = k + 1 as non-fixable as there is another reference to k after the first one, so I have to add a workaround for references inside the same assignment

[IEncinas10]

Had time to clean things up, but I encountered a problem with AutoFix tests. Right now, it doesn't allow for test cases
with several linting violations. But in order to test the 'safety' of autofixes, I need it.

The easiest workaround I see is to default to the first violation with autofixes, let me know what you think. If you think it is acceptable, let me know whether to submit a separate PR or include it in this one. @hzeller

Basically, the 'fix' would be (missing error handling if there are no violations with autofix):

-  CHECK_EQ(violations.size(), 1) << "TODO: apply multi-violation fixes";
-  CHECK_GT(violations.begin()->autofixes.size(), test.fix_alternative);
-  const verible::AutoFix& fix =
-      rule_status.violations.begin()->autofixes[test.fix_alternative];
+  // Default to first violation with available autofix
+  const LintViolation &violation =
+      *std::find_if(begin(violations), end(violations),
+                    [](const LintViolation &v) { return v.autofixes.size(); });
+
+  CHECK_GT(violation.autofixes.size(), test.fix_alternative);
+  const verible::AutoFix &fix = violation.autofixes[test.fix_alternative];

The need comes from this test:

  const std::initializer_list<verible::AutoFixInOut> kTestCases = {
      // We can't fix 'k &= 1', because it would affect
      // 'p <= k'
      {"module m;\nalways_ff begin\n"
       "k &= 1;\n"
       "p <= k;\n"
       "a++;"
       "end\nendmodule",
       "module m;\nalways_ff begin\n"
       "k &= 1;\n"
       "p <= k;\n"
       "a <= a + 1;"
       "end\nendmodule"},
};

Where I basically check that the first violation doesn't have an autofix available. It could be done changing the test but it is easier this way.

---

Regarding the fix of the issues discussed above (safe autofixes), I did my best to explain everything in the code. Let me know of any suggestions.

Edit: Pushed changes so they can be reviewed while we settle on the AutoFix thing

[IEncinas10]

Sorry for the mess, let me know if I should open another PR to ease review or not mess with the commits

[hzeller]

Let me run clang-format separately, then if you re-base the changes should not look so big.
Does that sound good ?

[IEncinas10]

Would it also work if I just force push after cleaning up the mess locally? I rebased needlessly and somehow my commits appear replicated.

Thanks!

[hzeller]

to rebase, you'd first

git fetch upstream
git checkout master
git merge upstream/master
git push

... so that your fork is up-to-date with the master.

Then, on your command line you can go into your branch

git checkout autofix_always_ff_non_blocking
git rebase master

Then, if there are any conflicts, resolve them locally. The git CLI tool guides you what to do.

Then, you can push your branch and then only your changes should show up.

git push

Don't worry about the changes introduced by the formatter, we can ignore them by adding ?w=1 to the review URL.
https://github.com/chipsalliance/verible/pull/1912/files?w=1

Maybe, the rebase attempt breaks even more though. In that case it is probably easier to create a copy of the files that you did change and create an entirely new pull request with that.

[IEncinas10]

Thank you very much for your efforts! In the end I just had to do the force push... I don't know what happened with my first rebase that I ended up replicating commits.

I just wasn't sure if doing a force push would create another mess in the GitHub interface. Anyways, problem solved.

[hzeller]

Just looking through old PRs, I guess this also fell along the wayside.
It looked ready to merge after some rebase situation, but there was this one open question on a test. Had that been resolved ?

[IEncinas10]

Good old times where I didn't know my git 😄

There were several problems:

1. There were changes required for both this PR and the dff-name-style one (helpers for IncrementDecrementExpression: var++, ++var)
2. Some issues with test helpers as you point out which I hacked away but wasn't sure if it was a good change
3. Potentially offering this autofix when it would change code behaviour

(1) will hopefully soon go away. (2) Isn't that bad but perhaps there is a better solution (I'll think about it). (3) is probably not perfect but I don't think it can be, I don't know verilog that well but probably non-automatic functions calls can modify local variables inside always_ff blocks which we can't possibly catch to avoid offering the autofix (or now that I think about it perhaps we can just get really scared and avoid offering the autofix if we see macros/function calls)