Better row unification error messages

[FredTheDino]

Description of the change

Related to #4413, and part of a potential solution.

The idea was to show which record labels are different. Sometimes, you have typos or a bunch of fields that mostly unify. This could help with that.
I'm hesitant to make more work on this unless there's interest. So let me know if this is a change people want to see. I know this or something that's feature adjacent.

This simple program:

module A where

-- As we can see, Stuff > Thing
type Stuff s = { foo :: Int | s }
type Thing s = { foo :: Int, bar :: Int, baz :: Int | s }

doStuff :: forall s. Stuff s -> Int
doStuff { foo, bar } = bar

doThings :: forall s. Thing s -> Int
doThings thing = thing.bar

Now compiles with the error:

This is a toy example. But it makes it very clear that the bar label is what is causing the unification to fail.
I think it's an improvement which changes the errors for the better.

🔥 Roast me! 🔥

Checklist:

• Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
• Added myself to CONTRIBUTORS.md (if this is my first contribution)
• Linked any existing issues or proposals that this pull request should close
• Updated or added relevant documentation
• Added a test for the contribution (if applicable)

[rhendric]

Thanks for taking a crack at this!

My general objection to changing error messages from

Error:
    AAAAAAAAAA
      BBBBBB
      CCCC
    DDDDDDD
        EEEE

to

Error:
    AAAAAAAAAA
      BBBBBB  <<<LOOKATMELOOKATME
      CCCC
    DDDDDDD
        EEEE

is the following: are lines A, C, D, or E ever the thing a user needs to look at? If not, the better change would be

Error:
  BBBBBB

If so, then this change is not an improvement; it's just moving the confusion around. Some users get directed to the important part of the error for them, and other users get misdirected away from the important part of the error for them.

In this specific instance, consider the following fragment:

f :: forall r. Identity { foo :: Int | r } -> Int
f (Identity v) = v.foo

x :: Identity { foo :: String, bar :: Int }
x = Identity { foo: "hi", bar: 5 }

y :: Int
y = f x

The error currently produced says:

  Could not match type

    String

  with type

    Int


while trying to match type
                             ( bar :: Int
                             , foo :: String
                             ...
                             )

  with type
              ( foo :: Int
              ...
              | t0
              )

IIUC, your PR would make this

while trying to match type
                             (*_bar_ :: Int
                             , foo :: String
                             ...
                             )

  with type
              ( foo :: Int
              ...
              | t0
              )

which makes the hint less helpful, as there's nothing wrong with bar.

[FredTheDino]

Thanks for the feedback @rhendric! :D

I agree it's not the most elegant solution. I also dislike the visual clutter this causes. But I don't agree that the information isn't helpful or moves the issue. The authors intent of this code, is unclear. It could be that the bar field is missing, and that's what is important.
Consider this code:

doStuff :: { a :: Int, b :: Int, c :: Int, d :: Int, e :: Int, f :: Int, h :: Int, j :: Int } -> Int
doStuff { a, b, c, d, e, f, g } = 1

When i look at this code, I think it looks decently right. On my first skim I would not immediately notice the error.

Gives this error:

  Could not match type

    ( a :: Int
    , b :: Int
    , c :: Int
    , d :: Int
    , e :: Int
    , f :: Int
    ,*_h_ :: Int
    ,*_j_ :: Int
    ...
    )

  with type

    ( a :: t7
    , b :: t6
    , c :: t5
    , d :: t4
    , e :: t3
    , f :: t2
    ,*_g_ :: t1
    ...
    | t8
    )

The difference in the fields is clearly given. I've had to manually match these fields where there are upwards of 70 rows and 1 row missing. This very simple tool trivializes the entire task of spotting the difference in the label names.

Compare this error to:

  Could not match type

    ( a :: Int
    , b :: Int
    , c :: Int
    , d :: Int
    , e :: Int
    , f :: Int
    , h :: Int
    , j :: Int
    ...
    )

  with type

    ( a :: t7
    , b :: t6
    , c :: t5
    , d :: t4
    , e :: t3
    , f :: t2
    , g :: t1
    ...
    | t8
    )

And it would already save me as a program writer brain power and thinking. Thinking that should go into the code I'm writing.

I definately understand the visual clutter point. We've discussed that earlier. I think there's more we can to do to remove the clutter. I for one is for removing the entire

where t0 is an unknown type
      t8 is an unknown type
      t7 is an unknown type
      t6 is an unknown type
      t5 is an unknown type
      t4 is an unknown type
      t3 is an unknown type
      t2 is an unknown type
      t1 is an unknown type

part.

But I think the compiler error messages are in a form of local optimum. They
are not the best version they can be in. And to get the best version possible
we might have to make alterations which don't solve all problems we are having right now.

Some other ideas of getting forwards:

• Add this as a special case in the error messages, if all the types are unknown only show the different labels
• Survey the community for bad errors, examples of where the compiler mislead them
• Add this and revert it if people complain
• Maybe someone has a suggestion which makes this change better

It might be that we should put up goals for what we want the errors to be. But
doing something is better than doing nothing. :)

[rhendric]

My point is that for every example you come up with where this change helps the user find the problem, I can come up with an example where this same change makes it harder for the user to find the problem. That's not a global improvement. That's, at best, throwing some users under the bus to make other, hopefully more plentiful users happier—and I'm not yet convinced that the majority of cases are on your side.

You don't have to solve this problem perfectly in order to make progress on it. You just have to do no harm in the process.

For example, just looking at these errors:

  Could not match type

    ( a :: Int
    , b :: Int
    , c :: Int
    , d :: Int
    , e :: Int
    , f :: Int
    , h :: Int
    , j :: Int
    ...
    )

  with type

    ( a :: t7
    , b :: t6
    , c :: t5
    , d :: t4
    , e :: t3
    , f :: t2
    , g :: t1
    ...
    | t8
    )

it seems likely that we could make more progress on unification, matching up the a through f labels, before raising the error. That would result in the error message being smaller (because the ... already hides the part of the row that is equal to the other row) and wouldn't mislead users when one of the types in a through f is not a match—a win for everyone. That may not be a perfect solution either but it would make progress in a way that does no harm, and assuming it works I'd support it.

[FredTheDino]

I don't know the inns and outs of the type checker perfectly. But reading @MonoidMusician s comment:

The tricky part is that unification mismatches can apply in e.g. contravariant contexts, so you don't actually know which labels was missing and which labels were extraneous, you can only see that there was some mismatch. (See also the infamous #3399.)

#4413 (comment)

I figured that is very hard to unify more things in the type checker. This lead me to not investigate those solutions. I'm not sure it would be possible.

I don't know if it's possible to get better error messages without "doing harm", much less so in increments. (relevant XKCD)

I believe adding more information is doing the least harm.

It sounds a lot like you want a complete revolution of the error messages or for them to be left alone. Something that unfortunately won't come out of making low hanging fruit changes in places where users are complaining.

My reasoning went something like this:

• The type errors display label mismatch and type mismatch (which kinda overlap)
• Sometimes the labels are the problem (we cannot know this)
• Sometimes the types are the problem (we cannot know this)
• The current error messages assumes the problem is in the types of the rows
• Annotating which labels differ gives more information for the second kind of problem which is currently under represented

The goal was to be as un-intrusive as possible with *. Maybe a smaller symbol would remove the visual noise for you? Maybe run this for a day and see how it feels? Is the problem really the symbol or that more information is added (which I suspect is unavoidable).

We could add another section to the error saying which labels differ there, if any. Is that a better solution?

[rhendric]

The tricky part is that unification mismatches can apply in e.g. contravariant contexts, so you don't actually know which labels was missing and which labels were extraneous, you can only see that there was some mismatch. (See also the infamous #3399.)

#4413 (comment)

I figured that is very hard to unify more things in the type checker. This lead me to not investigate those solutions. I'm not sure it would be possible.

The thing being highlighted as difficult in that comment is telling the difference between a missing label that's needed or an extraneous label being provided. Simply proceeding with unification to eliminate more unknowns shouldn't run up against any intractable problems, though it might not resolve everything.

I don't know if it's possible to get better error messages without "doing harm", much less so in increments. (relevant XKCD)

That's not as relevant as you think it is. I'm not talking about ‘breaking workflows’; we've done plenty of major releases where we've changed quite a lot about how users' workflows work, and when we're working towards a better world that is a decision we're comfortable making. That isn't this. We're talking about an error message that, conceptually, is saying, ‘Hey user, you might have problem A or you might have problem B.’ You're proposing adding information that biases the user toward looking at problem A. That disadvantages the users who have problem B. That's not a better world to be worked toward, unless you're myopically focused on the users with problem A.

It sounds a lot like you want a complete revolution of the error messages or for them to be left alone.

No idea where you're getting that but that's definitely not what I want. A complete revolution is neither necessary nor desirable here. Leaving things alone is tolerable but not ideal either. I'm glad you're stepping up to work on the problem; I want things to improve, and I want to help you contribute!

Working on the problem, however, involves considering the full space of cases that might trigger this error message, and making sure that in improving parts of the space you aren't making the message less helpful in others. It can be a difficult balancing act, and that's probably the biggest reason why nobody has fixed this yet.

The goal was to be as un-intrusive as possible with *. Maybe a smaller symbol would remove the visual noise for you? Maybe run this for a day and see how it feels? Is the problem really the symbol or that more information is added (which I suspect is unavoidable).

We could add another section to the error saying which labels differ there, if any. Is that a better solution?

In this case I'm not concerned about how noisy the indicator is. I'm concerned that the indicator in the error message makes it read as if that's the thing the user needs to look at to solve their problem. If you can figure out a way to format the diff so that it's obvious that you're just highlighting some rows that are different and that this isn't necessarily what the user needs to focus on, I could be okay with that. Maybe a second section would work. Just keep in mind that when open rows are involved, frequently extra or missing labels are totally fine and users—particularly beginners—shouldn't be misled by the error message into adding properties to their objects that they don't need, for example. For this reason, I'm not very optimistic about this approach, but you're welcome to take a stab at it and see if your creativity exceeds my imagination.

Edit: Actually, maybe a side-by-side diff of rows (without any additional highlighting or indicators) would do the trick here?

[JordanMartinez]

Actually, maybe a side-by-side diff of rows (without any additional highlighting or indicators) would do the trick here?

That's what I was thinking after reading through the above comments. The issue isn't the error per say but that jumping back and forth between really long rows makes it hard to see what is different about them. Consider the following examples of the same error shown in the comment, #4421 (comment):

Side-by-Side

Could not match type on left with type on right

    ( a :: Int      ( a :: t7
    , b :: Int      , b :: t6
    , c :: Int      , c :: t5
    , d :: Int      , d :: t4
    , e :: Int      , e :: t3
    , f :: Int      , f :: t2
                    , g :: t1
    , h :: Int
    , j :: Int
    ...             ...
                    | t8
    )               )

Downsides:

• when the labels and/or their types are really long, this won't display well. For example, if one of the label-type associations in the row was aReallyLongNameForSomeReason :: SomeNewtypeOverSomething (Array (NonEmptyArray (Tuple a (Either b (Maybe c))))).

Row comparison

  Could not match rows in type

    ( a :: Int
    ( a :: t7
    
    , b :: Int
    , b :: t6
    
    , c :: Int
    , c :: t5
    
    , d :: Int
    , d :: t4
    
    , e :: Int
    , e :: t3
    
    , f :: Int
    , f :: t2
    
    <no row>
    , g :: t1
    
    , h :: Int
    <no row>

    , j :: Int
    <no row>

    ...
    
    <no tail>
    | t8
    )

Downsides:

• the error itself may span quite a large number of lines in cases where one type has a large number of rows. Considering that this already happens, maybe that's ok?

[FredTheDino]

Great to have more input!

I've been thinking about this quite a bit for the last days. I you're both right that a more diff like approach is better. Particularly the second example that Jordan pointed out. I'm gonna look at implementing that.

But I see some perils on the road to better error messages still. Switching to more of a "diff approach", might make it hard for new users to read the error message. Maybe some extra indentation could help, or maybe color coding the types differently might help.

A more spread out type error might cause confusion of which row is part of which type. Spreading the type out might make it hard to see what belongs to what. Changing the shape of the error so drastically might also make it harder for people more used to reading the old errors.

I'm gonna try to implement the more diff-like approach and we'll see how that turns out. I'm sure we're missing something there as well. :)

[FredTheDino]

I've rewritten the errors to now display a diff. Should be easier to see typos. This is what it looks like one commit back:

(ce2fa58)

I made some alterations to what Jordan suggested, it is a different solution but maybe it has something in it that is relevant or interesting.

But after the last change it's more inline with what Jordan wrote.

(cd79ba0)

I'm not entirely sure about the unified view of the error. It makes it kinda hard to know which type is which. I think the indentation helped a bit but it also added a bit of noise. I don't think either of these are perfect and I do prefer the formatting that Jordan suggested.

And this solution is not complete. Tails aren't handled and the record/row open and closing brace aren't correct. Just to name a few. I'll let this sit for a few days and see what people feel about it before I do more work on it.

[JordanMartinez]

I made some alterations to what Jordan suggested, it is a different solution but maybe it has something in it that is relevant or interesting.

Could you clarify what those alterations were?

I'm not entirely sure about the unified view of the error. It makes it kinda hard to know which type is which.

There's another issue open for better indicating what was the 'expected' type and what was the 'actual type'. But, I do think this error is overall better than what we currently have.

Also, I think there should be a few golden tests:

• one row has a label whereas another one doesn't
• all labels are the same but...
  • one label's type is Int whereas another is String
  • one label's type is a really long type while the other is a similarly long type and both don't unify (i.e. does the rendering function properly account for long type names, and does the overall look still look good?)
  • one label's type is Int whereas another is { foo :: String } (i.e. some initial record nesting occurs)
  • one label's type is { bar :: Int, foo :: String } whereas another is { foo :: String } (i.e. in a nested record, a label is missing)
  • one label's type is { foo :: Int } whereas another is { foo :: String } (i.e. in a nested record, types in the same label don't unify)

You can look at Writing tests if you need clarification on how to do that.

[FredTheDino]

TL;DR: I'll gladly implement this more thoroughly. But is this likely to get merged?

Could you clarify what those alterations were?
I removed one of the parenthesis in the start. I also changed some of the layout a bit. I also haven't added suppor t for the RowType tails. But most of this is just "this was faster to implement and I don't want to write code that isn't used".

There's another issue open for better indicating what was the 'expected' type and what was the 'actual type'. But, I do think this error is overall better than what we currently have.

I might be overly sceptic of the changes, but I'm not sure this change will go over well with everyone. Some of the things I've noted:

• The extra whitespace will make the rows take more space then previous
• The new errors are not easily copy-pastable if you want to use this to e.g. generate a type
• New users that use other typed languages might be more used to "traditional" errors where the types aren't interlaced, this might make the learning curve steeper for an already tough language
• The 2 developers I've shown this too didn't understand the error at first glance

There are of course some pros to this new way of showing.

• Easier to see what fields differ
• Very apparent when you make a typo
• It's cool

This doesn't convince me that we:

make progress in a way that does no harm

Maybe my list of cons is too long. I'm all for digging deeper. I'm personally almost at the point where I consider doing a survey in the Discord. I'd love to see changes and I'll happily do them, but I want them to be changes that are to the benefit of everyone.

I'll add as many tests as we can think of, but is there value in me working more on this?

[rhendric]

Can't speak for Jordan, but in my opinion work on design is less likely to be wasted than work on tests at this stage.

Though I still think the most straightforwardly beneficial approach would be allowing row unification to continue past the first mismatch, and then seeing whether work on changing error rendering is still needed.

[rhendric]

In fact, doing nothing more than changing

purescript/src/Language/PureScript/TypeChecker/Unify.hs

Lines 179 to 180 in df5fcff

	unifyTails _ _ =
	throwError . errorMessage $ TypesDoNotUnify r1 r2

to the following:

  unifyTails r1' r2' =
    throwError . errorMessage $ TypesDoNotUnify (rowFromList r1') (rowFromList r2')

makes the error generated in #4421 (comment) become the following:

         Could not match type
                     
           ( h :: Int
           , j :: Int
           ...       
           )         
                     
         with type
                    
           ( g :: t1
           ...      
           | t2     
           )        

I suspect that doesn't cover 100% of the cases you're concerned about, but I recommend investigating that sort of improvement first (you may need to make a few other similarly-sized tweaks in that area; there's a sequence_ in there that I suspect may want to be a parU) and seeing how close it gets to your ideal.

[JordanMartinez]

I might be overly sceptic of the changes, but I'm not sure this change will go over well with everyone. Some of the things I've noted:

Hmm... those are all good points, and it further highlights why resolving issues like this are hard.

Can't speak for Jordan, but in my opinion work on design is less likely to be wasted than work on tests at this stage.

I agree with this. My comment above was more mentioning tests that should be added at some point / cases to consider when designing the final error. However, @rhendric is right in that writing tests shouldn't be done until this design is better flushed out.

[FredTheDino]

I have now made the changes that @rhendric suggested. The tests noted a change in one place, and I would say it's equally good or an improvement. This change doesn't solve my entire problem, but it goes part of the way there.

I would propose merging this change (I can make a new PR if that's easier) and then look into potential changes to error messages given the better unification.

I also looked through all other errorMessage calls to see if there was any other low-hanging fruit - but since I don't know what I'm doing, I didn't find anything notable.

[rhendric]

At this point you need tests. What cases were you looking at that you wanted to see improvements on? In particular, did you have a case that didn't look good without the sequence_ to parU change but did look good afterwards? (You shouldn't blindly follow my hunch without testing it. I could commit and push my own hunches if that's all that we needed to make progress! 😄)

Also, are there any downsides to making this change? Can you explain what they are or why they don't exist?

[FredTheDino]

Also, are there any downsides to making this change? Can you explain what they are or why they don't exist?

TL;DR: These changes improve the error messages somewhat.

From how I understand the code - which is probably faulty - I see this as a safe change. Sending out other type information than the original types should not change the correctness of the compiler. The error messages I've seen, given all the unit tests in this code and from all programs I've run I've never found a "worse error message". That said, there are differences. Now the compiler exposes less type information displayed in certain errors. The more focused errors makes it easier for me to understand the type error at a glance, but someone else might prefer the verbosity.

We should also consider the harm of this change. It is a change, there will be friction. It's less friction than the previous changes. I think it's less likely to mislead someone.

Changing the returned error
I cannot reason about all error outputs for all programs. But I can guess. My guess is that most people who are reading error messages want to know the difference between the type and the type the compiler expects. Showing the unified rows creates a more focused error.

Changing the sequence into a parU
Again, this doesn't change correctness, finding an error in parallel or in sequence still finds the error. But this is the more controversial of the changes. But finding errors in parallel could potentially show more errors earlier, saving the developer a recompile and giving context about other errors. I would expect the compiler to notice all errors in a row if it's been bothered to check it. That said, it also produces more output. I still think this error is better than the previous.

In particular, did you have a case that didn't look good without the sequence_ to parU change but did look good afterwards?

Yes, see tests.

I could commit and push my own hunches if that's all that we needed to make progress!

I'm well aware. I see this as a learning opportunity. Not every day you get to poke around in a compiler. Thanks for all the help, hope I haven't been too frustrating. <3

To finish this in a truly scientific manner, I still think more work needs to be done in this area. But it's hard to see these changes in isolation. So I'm not going to open another PR until #4421 and #4411 are merged. 🍡

[JordanMartinez]

Hm.... I would have expected the recent changelog entry edit to not trigger CI. Looks like #4502 didn't work...?

Edit: seems like this behavior is expected and we'd have to move to push to workaround it: https://github.com/orgs/community/discussions/25767#discussioncomment-4397713

[rhendric]

Yeah, I would not have expected #4502 to do what you wanted but you guys merged it before I took a look and then I figured it wouldn't be worth a fuss. I don't think push would be an acceptable alternative because it wouldn't trigger on pushes to other people's forks, which is where most PRs come from. We could instead put this logic in the workflow itself—check if the last successful run differs only from the current commit in irrelevant files and skip all actions if so.

[rhendric]

I think this could use some eyeballs belonging to someone who regularly bumps up against errors involving large rows, but pending their approval I'm happy with where we've landed. I don't think it's perfect but I think it's better in every way on the cases we've touched.

[JordanMartinez]

Given that no one has responded in the past few days and that this was a highly-desired PR on the survey, there's three ways we could move forward:

1. we continue waiting until someone has a good example on which to try this PR, hope they build this PR locally, and then tell us of the results. Building this PR locally is not exactly convenient and that inconvenience may lead them to just spending their time trying to resolve the problem rather than building this PR and using that version of the compiler on the problem.
2. merge this PR, making a pre-release. The pre-release removes the need to build the PR locally. so we're more likely to get feedback that way. However, being 1.5 weeks out from the next scheduled release, we may not have enough time for someone to come across a problem where they get to see how this version handles things.
3. We make an official release now for 0.5.11, then merge this PR, making a prelease 0.15.12-0, which gives this work a 6-week time period to garner feedback.

I don't think it's perfect

What do you consider to still be imperfect about it?

[rhendric]

Probably not everyone who indicated interest in this PR on the survey is subscribed to it. We could maybe stir up some interest on Discourse. I think it's important for requests like this one to have at least one ‘customer’ verifying that the change meets the objective before it's merged; I apply this policy to things like ‘better error messages’ and ‘make go faster’, as opposed to ‘here is a bug where the documentation or common sense says one thing should happen but another thing happens instead’. If this is such an in-demand feature, finding one single user who has this problem and can build the compiler shouldn't be hard. And conversely, we don't need to rush it out if we can't.

What do you consider to still be imperfect about it?

I think there are some inconsistencies in what errors we present as rows versus records, and I think there are many more places where the expected/found distinction could be drawn but where the current code shrugs (in fact, I somewhat suspect that, given that we highlight a particular subexpression, we can always draw an expected/found distinction from the perspective of the highlighted subexpression, and the greatest barrier to doing so is that there are still places where highlighting is off because source spans aren't being applied in one or two spots). I'm pretty comfortable with all that being worked out later as needed, though.

[JordanMartinez]

We could maybe stir up some interest on Discourse.

Would you mind typing up such a Discourse post then?

Edit: Ryan's corresponding post is here: https://discourse.purescript.org/t/upcoming-changes-to-error-messages-for-large-records-rows/3696/1

I think there are some inconsistencies in what errors we present as rows versus records, and I think there are many more places where the expected/found distinction could be drawn but where the current code shrugs (in fact, I somewhat suspect that, given that we highlight a particular subexpression, we can always draw an expected/found distinction from the perspective of the highlighted subexpression, and the greatest barrier to doing so is that there are still places where highlighting is off because source spans aren't being applied in one or two spots). I'm pretty comfortable with all that being worked out later as needed, though.

Makes sense. Thanks for clarifying.

[f-f]

I had a look at the patch and it seems good!

I am approving this so that we can get it merged and test it more widely through the prerelease - we could keep this waiting and ask people to try it out, but I think that's asking for unrealistic conditions: I do certainly see terrible row mismatch errors, but not at predictable times, and when that happens I'm usually in the middle of something complex so I want to just go past it rather than compile a custom build of the compiler.

While it might not be perfect, the improvements to error messages seem good enough to warrant getting this mainlined and iterating on it.

[rhendric]

Based on the current state of the Discourse thread, I actually think this PR misses the mark wide. The test cases we were focused on all along are not relevant to what seems to be the primary way these error messages come up in the wild—namely, through Union constraints instead of through unification with rows having unknowns in their tails.

Please let's have a little more time to consider whether anything in this PR is relevant to handling that use case before we run ahead and merge it.

[JordanMartinez]

The test cases we were focused on all along are not relevant to what seems to be the primary way these error messages come up in the wild—namely, through Union constraints instead of through unification with rows having unknowns in their tails.

I somewhat disagree with this interpretation of people's responses so far. While Union constraints are another aspect of this problem we have missed, I'm not sure I would consider them to be the primary way such errors occur. (We've only had 2 people respond so far, and one of them said "normal" errors were better). So, I still think this PR does address some issues, just not all of them, and only two responses is not a clear indicator of that.

[rhendric]

I still think this PR does address some issues, just not all of them

I guess it depends on your definition of ‘issue’. I would take the perspective that an issue is something that a user encounters that makes their day worse. I think this PR makes some changes, I think it makes some of our error messages better, but I have yet to see any evidence that it resolves any issues. All we have is evidence that when users poke the thing we changed, it responds with the error messages we designed for that type of poking. We could have the best error messages imaginable for some scenario that users never naturally encounter in the wild, and it wouldn't improve things at all.

In exchange for that no-benefit, the PR adds several confusingly-named functions (we did our best, but we should also acknowledge that where we ended up isn't great) to the project and possibly impacts the performance of the type checker. It removes three constructors from our SimpleErrorMessage type and adds a parameter to another one—kind of a draw, there, in terms of long-term impact, but a net negative if we end up wanting those constructors back soon.

Most PRs are written by people who are invested in the particular issue that they resolve. This PR, at this point, is mostly written by you and me; and speaking for myself, I have never had problems with the error messages I get related to rows. But I also don't use react-basic for anything. For most PRs, having an author who knows the problem space and believes that the solution improves things is enough to bias me towards making changes on the scale we're making them here. But we don't even have that—we have authors who (again speaking for myself) are interested in the abstract in improving our error messages, but who don't have experience with the specifics of this type of error message—so from my perspective we run quite a high risk of churning the code for no end benefit to anyone. I object to the let's-just-merge-and-see approach if we don't even have a single stakeholder telling us that it resolves one of their issues.

The Union thing does genuinely give me pause. I haven't sat down to take the time to think it through but I would like to, and I think there's a decent chance that dealing with it will mean yet a different approach than the one we took here; it might not be just another function that we need to bring into the unifyishRows family. Also, while yes we've only had two respondents, from their comments I think that Union is the main course here, not a side dish.

[drathier]

Huge reply from the author on discourse https://discourse.purescript.org/t/upcoming-changes-to-error-messages-for-large-records-rows/3696/19 says he's available on discord but rarely checks github issues or discourse. Lots of input on this problem too, which I definitely second.

[JordanMartinez]

I guess it depends on your definition of ‘issue’....

@rhendric I would quote the rest of what you said above, but that's a lot of text 😆.

Fair points! From my perspective, this does make things better, but I don't think I can provide evidence proving that, at least not enough to convince you. I'm also finding it difficult to come up with a logical response to your arguments above. However, part of the difficulty of this issue is coming across reproducible errors that can be collected into a publicly-visible place that would inform us on how to best improve errors. There's a lot of upfront cost to do that.

Given the responses on the Discourse thread, what do you propose is the way to move forward? Given both my initial response and @f-f's response to merge this, it may be worthwhile to close this PR and refocus on the original problem. So long as the branch isn't deleted, none of the work done so far is lost.

[rhendric]

From my perspective, this does make things better, but I don't think I can provide evidence proving that, at least not enough to convince you. I'm also finding it difficult to come up with a logical response to your arguments above.

It matters to me what hat you're wearing when you say this makes things better. If it's your user hat—if you encounter the previous iteration of these messages in your own PureScript work and would prefer the new ones—that's all the convincing I need. If it's your maintainer hat—if you think the code is better with these changes than without them, independent of what users experience—then I disagree but there's no reason for my opinion to be weighed above yours; let's keep talking and see if we can agree or get a third maintainer's opinion. If it's your business analyst hat—if you have reason to believe users would want to see these changes merged—that's the tougher hill to climb, unless you can produce the users who want this. Otherwise it's your imaginary users against my imaginary users and that's hard to discuss logically, as you say.

what do you propose is the way to move forward?

From a process-management perspective, we could close; maybe it'd be better to mark as draft, since I don't think we're giving up on the concept, but I don't feel strongly either way.

From a getting-things-done perspective, whichever one of us gets cycles first should make a test case that mimics the react-basic API and start sketching out ways to make that error more digestible. It might be special-case logic for handling Union constraints; it might involve increasing the tracking already started in this PR of which types are expected and which are found; it might be a general-purpose change to the row-diffing code if only one side of the diff is above a size threshold; it might be something I haven't brainstormed yet. I think I at least have a pretty good idea of the conditions that cause the huge-row errors that react-basic users are encountering in the wild. I have no idea about the conditions that produce FredTheDino's errors but if they can't share details and won't participate in implementation then there's only so much we can expect to do about that. So I would focus on react-basic and making those errors look like things I'd want to see in a terminal, somehow, details TBD because that's the work. And if that approach ends up intersecting with what we already did here, great, that's however many person-days you and I put into this already not wasted after all.