Adjust formatting of blocks and scopes #5474
Conversation
toolchain/check/testdata/basics/no_prelude/dump_sem_ir_ranges.carbon
Outdated
Show resolved
Hide resolved
toolchain/sem_ir/formatter.cpp
Outdated
| if (ShouldFormatInst(inst_id)) { | ||
| if (is_tentative) { | ||
| TentativeOutputScope scope(*this, |
There was a problem hiding this comment.
According to their comments, ShouldFormatInst queries whether the inst should be included in the output, and TentativeOutputScope is for when we don't know whether it's included in the output. So once ShouldFormatInst has told us it be output, why do we still need to be tentative about it? It seems like there are two different notions of whether an inst should be included in the output, and I don't understand the difference between them.
There was a problem hiding this comment.
IIUC, you're just asking in general how the formatter works, not specific to this PR?
For the formatter, tentative output and final output are distinct. "Tentative" formatted output may be included in the final output.
As a common example, importing impls creates a lot of instructions that are important but often unused. The goal is to hide those (see the delta of #4534, which added the logic you're asking about).
Because constants are printed first, we haven't done the work to determine whether they're referenced. We want to not print constants that are not referenced (per TentativeOutputScope, "we don't yet know whether to include it in the final formatted SemIR.") But by creating this tentative output, we are able to exclude a bunch of constants for interfaces and other things which are typically taken for granted (as excluded), unless they're directly referenced.
Does that help?
There was a problem hiding this comment.
BTW, I'll note too the existence of constants {\n} in files occurs when all constants were tentative and excluded. We may be able to do better there, but not trying in this PR (tentative formatting doesn't fit in that cleanly).
There was a problem hiding this comment.
IIUC, you're just asking in general how the formatter works, not specific to this PR?
I think that's right; I already find this situation confusing at HEAD; this PR just heightens my confusion by juxtaposing ShouldFormatInst so closely with TentativeOutputScope.
For the formatter, tentative output and final output are distinct. "Tentative" formatted output may be included in the final output.
As a common example, importing
impls creates a lot of instructions that are important but often unused. The goal is to hide those (see the delta of #4534, which added the logic you're asking about).Because constants are printed first, we haven't done the work to determine whether they're referenced. We want to not print constants that are not referenced (per
TentativeOutputScope, "we don't yet know whether to include it in the final formatted SemIR.") But by creating this tentative output, we are able to exclude a bunch of constants for interfaces and other things which are typically taken for granted (as excluded), unless they're directly referenced.Does that help?
That makes sense as far as TentativeOutputScope is concerned, but then what is ShouldFormatInst for?
There was a problem hiding this comment.
That makes sense as far as
TentativeOutputScopeis concerned, but then what isShouldFormatInstfor?
ShouldFormatInst determines if an instruction should be passed on to formatting based on whether it's in a given range.
TentativeOutputScope determines whether an instruction that has already been formatted should be included in final output.
toolchain/sem_ir/formatter.h
Outdated
| // are used. Instructions may optionally use `TentativeOutputScope` (see type | ||
| // for details). |
There was a problem hiding this comment.
This phrasing makes it unclear whether this is giving permission to the caller or the callee, and how it relates to is_tentative.
| // are used. Instructions may optionally use `TentativeOutputScope` (see type | |
| // for details). | |
| // are used. If `is_tentative` is true, the instructions will be formatted using | |
| // `TentativeOutputScope` (see type for details). |
I think it would be better to document this without relying so heavily on implementation details like TentativeOutputScope, but I don't understand TentativeOutputScope well enough to suggest how to phrase that.
There was a problem hiding this comment.
This phrasing makes it unclear whether this is giving permission to the caller or the callee, and how it relates to is_tentative.
How about this: renamed to use_tentative_output_scopes for explicitness, dropped the comment since the correlation between use_tentative_output_scopes and TentativeOutputScope should be more implicit.
I think it would be better to document this without relying so heavily on implementation details like
TentativeOutputScope, but I don't understandTentativeOutputScopewell enough to suggest how to phrase that.
This is getting to complex Formatter behavior, so while maybe that could be more clearly documented, I think adding more detail here would end up duplicative, which I want to avoid. And, I don't think this is the right place to document that behavior.
There was a problem hiding this comment.
This comment and my other comment above are getting at the same underlying issue: I spent maybe half an hour trying to understand the changes to this function well enough to e.g. be able to decide when to pass true vs false, including looking quite closely at the comments and implementation of TentativeOutputScope, and I'm still confused. Some of that is attributable to my unfamiliarity with this part of the formatter, but I suspect that a few comments in key places could have reduced that time substantially.
There was a problem hiding this comment.
Maybe good to bring up on #toolchain? But, does it need to block this PR?
There was a problem hiding this comment.
To be sure -- my perspective is I'm pretty much just moving the ShouldFormatInst call from inside FormatInst to instead put it at the caller. And, if ShouldFormatInst is false, there's really no reason to put it in a TentativeOutputScope. I believe the tests show the changed code is working pretty much as expected, and not changing what's in output.
There was a problem hiding this comment.
Maybe good to bring up on #toolchain?
Sure, will do.
But, does it need to block this PR?
In principle maybe not, but in practice I don't feel like I can review the code if I don't understand it.
There was a problem hiding this comment.
We'd discussed a TODO, but given discussion on #toolchain, maybe it's better to just do the condition swap in this PR and see how it works out? ("flow" commit)
There was a problem hiding this comment.
That change looks great, but I would've expected it to have some effect on the test outputs (e.g. starting to print more constants and imports). Am I misunderstanding something?
There was a problem hiding this comment.
This sample has no constants or imports. See #5455 for a more complex example.
There was a problem hiding this comment.
n/m, fixed. But still, note #5455 is the more complex case.
This is making two inter-related changes:
fileto reuse the formatter logic ofconstantsandimports, meaning emptyfilescopes will be omitted<elided>sections in blocks (not in non-block scopes, because they're not as sequential)