save-analysis: dump in JSON format #33208
Conversation
...in which we make the spans nice.
In fact, we make JSOn the default and add an option for save-analysis-csv for the legacy behaviour. We also rename some bits and pieces `dxr` -> `save-analysis`
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
| fn use_data(&mut self, UseData); | ||
| fn use_glob(&mut self, UseGlobData); | ||
| fn variable(&mut self, VariableData); | ||
| fn variable_ref(&mut self, VariableRefData); |
There was a problem hiding this comment.
The advantage of default methods is that you can easily implement a dumper that dumps only a subset of the data. For instance, in RRT there is a dumper for collecting data about glob imports that only implements the use_glob method.
If there is no strong reason to change it, I would rather keep the default methods.
There was a problem hiding this comment.
Methods which are empty by default are trivial to implement, so there is not much advantage to not having to write them out. On the other hand, if we have defaults and we add another, then any implementers will be silently broken. With required methods, the compiler tells us we have to fix the impls.
On the other hand, I can see that if you just want (e.g.) globs, then it is painful to write out all the other methods. I guess Rust wants more nuanced inheritance for this kind of thing. I'll revert this change.
|
@nrc I've only started looking at the first commit, so maybe this Q is resolved later in the series, but: In removing the dump spans stuff, is your intention that spans should not be part of any serialized output? Or do the spans come back later in a different form? |
|
@pnkfelix dump spans was an old hack for just dumping spans of AST nodes without any other data. I only ever used it for debugging and it has no users, afaik. The stuff I removed is not really related to the rest of save-analysis, the spans are still there as part of the data structs. |
|
@nrc oh okay I misunderstood the role of "dump spans", thanks for the clarification. |
|
@bors r+ |
|
📌 Commit 129c3bb has been approved by |
|
⌛ Testing commit 129c3bb with merge e3c0c09... |
|
💔 Test failed - auto-linux-64-opt-rustbuild |
|
@bors r=pnkfelix |
|
📌 Commit 7ca2b94 has been approved by |
save-analysis: dump in JSON format cc #18582
|
@nrc I like the fact that you created new structs which are decoupled from the compiler's internals. This makes working with spans much easier. Would it make sense to feed For instance: Could become: Maybe we could also extract the A concrete benefit of this is that the lowering code only needs to be written one time and can be shared across multiple |
|
@aochagavia I had thought about this, and think it would be an improvement. However, I also have an alternate approach in mind (although not fully fleshed out): I would like that rather than have a I guess moving the I plan to discuss the long term plans for save-analysis with the rest of the compiler team next week, so there will probably be more input then. |
cc #18582