Add more exhaustive tests for .asTypeOf

[jackkoenig]

This includes changing AsTypeOfSpec to be FunSpec instead of FlatSpec so that the tests can have more structure.

I'm working on making it possible to preserve literals across .asTypeOf casts (which will help with unit testing and is much more convenient for async reset). In doing so, I found the existing tests extremely lacking so I decided to lay out the existing behavior exhaustively. Please do not review this PR based on what you think should happen¹, rather, treat this as documenting the existing behavior so that at least changes that should be functionally equivalent will be and we can be intentional in changing the behavior going forward.

I marked this as documentation since, at least for development purposes, it is documenting the behavior.

Marked for backporting to 6.x since I want to backport the literal preservation as well.

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• Documentation or website-related

Desired Merge Strategy

• Squash

Release Notes

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
• Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

Footnotes

1. All truncation should become an error when we get rid of implicit truncation and the "implicit extensions" for non-arithmetic types also should be an error, but that is obviously not the purpose of this PR. ↩

[jackkoenig]

I'll add that I wrote these tests against the Verilog because:

1. They're very simple and it presents a nice specification of behavior--it's easier to see that the behavior doesn't change than checking the CHIRRTL which I am actively trying to change.
2. It allows showing truncation behavior across FIRRTL as well. Since this may change at some point I want to call it out.
3. I am in the process of changing the implementation of asTypeOf quite a bit so I want tests that will check that I don't screw up--checks of the CHIRRTL would have to be changed anyway and thus may be prone for accidental behavioral changes.

[seldridge]

SGTM

I glanced through the tests. This is locking in existing behavior so they, by definition, should be fine.

The checking of the exact Verilog output is going to be somewhat brittle as this could change in arbitrary ways in the future. One idea is to avoid the wire keyword and width specification when it is not necessary.

[seldridge]

This type of test is going to be somewhat annoyingly brittle. You could probably skip the wire (probably in all tests, too).

[jackkoenig]

The checking of the exact Verilog output is going to be somewhat brittle as this could change in arbitrary ways in the future. One idea is to avoid the wire keyword and width specification when it is not necessary.

So I originally wrote the tests against an output port which would be better, but since we can no longer have inferred width output ports (at least for the top-level module), that didn't work for the tests with the inferred widths. That being said, I suspect these should be pretty stable as written. I intentionally used widths <= 10 bits so that there would not be any alignment issues due to wire + bitwidth. The tests are also pretty simple--there's not a lot of degrees of freedom for the Verilog to change. The main thing could be if the default setting changes to splitting declaration from assignment, but hopefully that would be exposed via a lowering option to snag the old (i.e. current) behavior.

In any case, if the Verilog does change, then I will vehemently agree with tweaking the tests to be more stable 🙂, but I would be comfortable going ahead as is and fixing that if/when necessary.