GaloisInc / mir-json

Plugin for rustc to dump MIR in JSON format
Apache License 2.0
9 stars 2 forks source link

Explicitly version the JSON schema #75

Closed RyanGlScott closed 1 month ago

RyanGlScott commented 1 month ago

We now include a "version": <N> entry in the top-level JSON map that gets generated for each MIR JSON file, where <N> represents the version of the JSON schema that is used to generate the file. Going forward, any changes to the JSON schema will be accompanied by a corresponding schema version bump. I have also included some logic in link_crates to ensure that all of the crates being linked together use the same schema version.

Fixes #45.

RyanGlScott commented 1 month ago

I've pushed a reworked version of this PR that uses only a single number for the schema version, and which explicitly checks that all linked crates have the same schema version number. PTAL.

sauclovian-g commented 1 month ago

It occurred to me just now (in the context of the crucible side) that there's a second factor in here, which is the rustc version. Eventually we'd like not to be tied to the one outdated rustc version, but since we don't control what rustc does we might have trouble encoding different rustc's MIR in the same schema version. So I was thinking we might actually want two major version numbers, one for essentially the MIR revision and one for our own JSON version within that. (Then crucible might support the last three JSON schema versions for each of the currently supported MIR revisions or whatever.)

also there were a couple things I'd add to the docs in here but since it's too late I'll just make another pull request.

RyanGlScott commented 1 month ago

Eventually we'd like not to be tied to the one outdated rustc version, but since we don't control what rustc does we might have trouble encoding different rustc's MIR in the same schema version.

I am skeptical that we will ever be able to build a particular version of mir-json with anything more than a single rustc version. Given that mir-json is implemented using the rustc API, it's far too coupled to rustc's internals to be able to support multiple rustc versions without using a significant amount of #ifs (which would be very painful).

That being said, I think it would be fine to bump the schema version whenever we upgrade the version of rustc that we build against.

sauclovian-g commented 1 month ago

In that case the corresponding situation is that we have branches of mir-json for different rustc versions. It's not inconceivable that we might want to do this and maintain more than one at a time. In that case, you don't want to end up in the situation where e.g. the 5.1 branch is using schema version 169 and the 5.2 branch is on schema version 174 after having gone through 170-173, and then some issue comes along where both need to be bumped.

There's no reason we need to add the extra sequence number now, but then we want to remember to add it as soon as we introduce multiple branches or anything of the sort. Which is fine I guess...