Use layout histories to produce better errors

[goldfirere]

This is still a work in progress, but I want to get performance feedback on this, as perf is the big worry with this direction of exploration.

[goldfirere]

This is now ready for review. @ccasin is the best whole-PR reviewer, though @antalsz may be interested in some of the restructuring in layouts.ml.

I recommend reviewing individual commits. (Individual commits might plausibly build, but don't count on it.)

Commit: Layout histories commit

The first commit actually adds the layout histories. However, the patch does not change many error messages, because I decided to prioritize other activities over carefully crafting beautiful errors with the new infrastructure. I did get far enough to convince myself that turning the new feature on is a good idea, but it's not quite ready yet. The behavior is toggled with Layout.Format_history.display_histories: turning that on adds detail to error messages. Another toggle is Layout.Format_history.flatten_histories: turning that off prints out the full history tree instead of a truncated version. I think this full tree is not very useful to users, but it's quite helpful while debugging! So you might want to use it in the future. We might, in the end, just build the truncated history (instead of truncating during printing) to save space.

Most of this patch is quite boring, with little ~why:Some_reason_or_other added to many lines. (Some lines remove ~reasons, because reasons are now tracked in different places.) Do not spend any time trying to check the reasons or the text printed for correctness. When we enable layout histories in printing, we'll have to go over all of this carefully -- no use investing that time now, without seeing the error messages.

This commit also introduce a new Layout.Violation constructor to track missing cmi files. It thus can remove the flag used for this purpose and thus change Layout.Violation.violation back to Layout.Violation.t.

layout_bound_of_{record,variant}_representation have more allocations than necessary, but these functions are removed in a later commit.

This commit removes the commoning-up of Tvar None nodes during cmi generation, but this did not to make much of a difference.

During this work, I discovered a bug, which now appears at the end of basics_alpha.ml. I think it will be easy to fix, but I did not do so in this patch.

Commit: Commoning up layouts in cmi files

This commit commons up layouts during writing to a cmi file, and also replaces their history with a boring Imported. Ideally, the Imported would carry information about where the layout was imported from. But my first attempt to do this failed, and we can revisit when we actually start printing histories.

This commit surprisingly made a <1% difference in allocations in a memory consumption test. I did not look at build product size.

Commit: Memoizing V1_safety_check

This made a significant improvement to the number of allocations.

Performance impact

In a simple test (compiling one moderate sized file and measuring allocations) this has a negligible impact. This PR is registered to get nightly performance tests, and so we'll see what news that delivers.

It would be sensible to, say, simply not store histories until we want to print them. That will save even more allocations. However, I think we'll want to turn this on someday, and so keeping the histories around will force us to reckon with any performance impacts. So my thought is to keep the histories as they are in this patch, but I could be argued away from this opinion.

[ccasin]

I'll look at this in detail this week, but one quick observation is that the last commit here should really be its own PR. This undoes a key design decision from Stephen's immediacy rework (which moved immediacy from the decl to the kind, see 10041 and 11841), and seems orthogonal to error messages, so it surely merits a separate conversation. (Though it's certainly possible it's an improvement - I was initially surprised to hear it works, but I guess the question about when to trust the stored immediacy information is separate from where to store it).

You mention this commit saves allocations in value_kind, but looking quickly those must come from skipping the call to layout_bound_of_kind, which as you note elsewhere is written suboptimally and can easily be reworked not to allocate. So I think this is just a question about design and correctness, not efficiency.

[goldfirere]

I've broken out the Type_abstract commit into its own PR: #1384. This is now based on that PR. The reason that the change to Type_abstract is so important is that, with histories, every query of a layout has to allocate a new layout. The change in Type_abstract effectively caches the layout so that a new one isn't allocated each time. So it's not that layout_bound_of_kind is slow, but rather that it needs to allocate.

[ccasin]

The reason that the change to Type_abstract is so important is that, with histories, every query of a layout has to allocate a new layout. The change in Type_abstract effectively caches the layout so that a new one isn't allocated each time. So it's not that layout_bound_of_kind is slow, but rather that it needs to allocate.

I am probably just being slow, but I don't understand this remark.

Here's types.ml in the commit that used to come before the one with the Type_abstract change. It looks to me like the only allocations in layout_bound_of_* are the calls to Layout.value and Layout.immediate. These seem to construct a layout without any mutable state in it, so can't we just do something like:

let ext_variant_bound = Layout.value ~why:Extensible_variant

let layout_bound_of_kind : _ type_kind -> _ = function
  | Type_abstract { layout } -> layout
  | Type_open -> ext_variant_bound
  | Type_record (_,rep) -> layout_bound_of_record_representation rep
  | Type_variant (_, rep) -> layout_bound_of_variant_representation rep

To share the values and avoid allocating? (And similarly for the other functions)

[goldfirere]

Chris is right, above, of course. I'm worried about a future where we'll want the reasons to e.g. mention the name of the type it is the reason for, and so the commoning up Chris shows isn't exactly what I'm looking for. In any case, the particular improvement there is now in #1384 and this PR can be reviewed separately. (Perf is still a worry; that is being tracked using Jane-Street-internal mechanisms, and we'll be able to verify that perf is OK before merging.)

[goldfirere]

Just to give an example, here is an error that we currently report:

type s4 = s5 t4
and 'a [@immediate] t4
and s5 = string;;

[%%expect{|
Line 3, characters 0-15:
3 | and s5 = string;;
    ^^^^^^^^^^^^^^^
Error:
       s5 has layout value, which is not a sublayout of immediate.
|}]

That's hard to untangle. With display_histories = true, we get

type s4 = s5 t4
and 'a [@immediate] t4
and s5 = string;;

[%%expect{|
Line 3, characters 0-15:
3 | and s5 = string;;
    ^^^^^^^^^^^^^^^
Error:
       The layout of s5 is value, because
         it equals the primitive value type string.
       But the layout of s5 must be a sublayout of immediate, because
         of the annotation on 'a in the declaration of the type t4.
|}]

Much better!

[ccasin]

This looks nice. It's clear it will be a big improvement to error messages and debugging. I added a bunch of questions and comments.

I remain worried about the impact on performance and artifact sizes, for the obvious reasons. Hopefully we will get a good picture of that from the test run.

[ccasin]

This looks nice. It's clear it will be a big improvement to error messages and debugging. I added a bunch of questions and comments.

I remain worried about the impact on performance and artifact sizes, for the obvious reasons. Hopefully we will get a good picture of that from the test run.

[goldfirere]

OK. I've addressed all the comments. Thanks for the review, @ccasin!

[ccasin]

Thanks! I think all that remains is to look at the perf. It looks like it hasn't run again since your push to that branch, but should soon.

[goldfirere]

I've now updated this PR and #1385 so that we can get updated perf numbers soon.

[goldfirere]

OK. If CI is green, I'd like to merge. @ccasin and I agree that the performance is acceptable. @ccasin, are you ready to Approve?

[ccasin]

Done.

[mshinwell]

Only files owned by me