Privatize members

[chrysn]

This is a sketch of what the changes of #141 could look like. The currently only commit changes ByteString (in a way that allows later fixing #132, but not doing it in here).

The changes to the API are way easier than the changes to the tests, because there are a lot of constructions, and I know of no good tooling to replace the calls. This also means that the fallout on applications will be hard, but I expect them to have way fewer construction places.

I've noticed that at least the ByteString type had no need for access or changes to the data in the tests, so it's not implemented either. Given that mutation easily causes inconsistencies, I think that only having .data() and .bitwidth() accessors, (and possibly an Into<Vec<u8>> destructor) would suffice, so that mutation would happen by copy or by destruction.

---

Is this a direction of solving #141 that sounds practical to you?

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.78% ⚠️

Comparison is base (7b1726f) 86.72% compared to head (d6c178b) 85.95%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
- Coverage   86.72%   85.95%   -0.78%     
==========================================
  Files           8        9       +1     
  Lines        1243     1246       +3     
==========================================
- Hits         1078     1071       -7     
- Misses        165      175      +10     

Files Changed	Coverage Δ
src/syntax.rs	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

[chrysn]

Given that there are lots of tests that will need to switch over to a constructor based API, and some user code is probably affected as well, a proper systematic version of this might best be done using replacement patterns. The semgrep tool seems suitable, and a rules file like this could be applied and do the bulk of tests changes:

rules:
- id: plain-bytestring
  pattern: 'ByteString { data: $X, bitwidth: IntegerWidth::Unknown }'
  fix: 'ByteString::new($X)'
  message: ByteString details are private (and Unknown is the default width in the new constructor).
  severity: WARNING
  languages: [rust]
- id: bytestring-with-width
  pattern: 'ByteString { data: $X, bitwidth: $Y }'
  fix: 'ByteString::new($X).with_bitwidth($Y)'
  message: ByteString details are private.
  severity: WARNING
  languages: [rust]

(Invoked as semgrep scan --config ./migrate-rules.yaml --autofix, which edits all affected files in-place. Beware that the default .semgrepignore ignores tests; a touch .semgrepignore overrides that).

[chrysn]

Before doing this in full with semgrep, I should also look into whether Cocinelle (that nowadays can also do Rust) may do it better or easier.

At any rate, the next step here is for @Nemo157 to decide whether this is the direction in which to evolve the API.

[chrysn]

As some of the later changes are stuck on this: What's your view here? Is this a direction this project can take?

[chrysn]

With diagnostic notation making good progress toward standardization, I'd like to do updates here that would accompany that – but some of those depend on progressing this issue. I'm fine with any outcome (even an "I don't know" is something I can work from), even though obviously a "go ahead" would be my ideal one.