Repository navigation
Improve TypesDoNotUnify message for record types #4413
Description
Summary
We should call out what difference in the records caused the match to fail in addition to the current output.
Motivation
When making heavy use of row polymorphism in a large codebase one often ends up with functions on record types that expect some number of labels with a generic row tail, which turns the error messages for two row types not matching into a bit of a spot the differences game, and this gets worse and worse the more stuff you add to these, or if you start nesting records.
Proposal
Call out what didn't match in the error message, i.e. something like 'expected the label "label :: Type"', or "type cannot have the label "label :: Type"'.
Examples
Type Stuff s =
{ foo: Foo
, bar: Bar
, baz: Baz
| s
}
Let's say you have a function with the following type
doThingWithStuff :: forall s. Stuff s -> SomeOtherType
Suppose also that you have a type
type Stuff2 s =
{ foo: Foo
, bar: Bar
, qux: Qux
, quaz: Quaz
| s
}
but which is missing the bar :: Bar label, and a value stuff2 :: Stuff2 (). Now say you pass stuff2 to doThingWithStuff. The error message isn't directly helpful at finding the actual problem, which is that Stuff2 is missing the label bar :: Bar. The error message should mention that the types failed to unify because the first has a label bar :: Bar and the second doesn't.
Now let's say you also have a function mkStuff :: StuffPrecursor -> Stuff () and you put in an extra label. It would be helpful if the compiler told you that your record has the extra label directly instead of giving a generic row TypesDoNotUnify message.
Activity
MonoidMusician commentedon Nov 2, 2022
There are already errors for this, but it looks like they are only used during subsumption, not during unification (see the
MissingRecordFieldandExtraRecordFieldtests):purescript/src/Language/PureScript/TypeChecker/Subsumption.hs
Lines 105 to 114 in b7e0596
purescript/src/Language/PureScript/TypeChecker/Unify.hs
Lines 163 to 180 in b7e0596
The error message would need some tweaking, since it isn't saying the right thing for unification, but I think it is a good idea to do what we can to improve these errors.
The difference between subsumption and unification is that the compiler is allowed to apply typeclass instances during subsumption, but not during unification. The compiler would use unification instead of subsumption if you have even something as simple as the
Identitytype constructor in the way:This is because the compiler will not do subsumption through type constructors, it resorts to unification then (* except function types? maybe? I don't actually know). (The reason it won't do that is that it would require taking apart and putting back together the datatype to apply the instances, and it doesn't even keep track of which parameters are covariant or contravariant.)
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.)
rpolley commentedon Nov 2, 2022
Honestly even just calling out what's actually different between the two types would probably be helpful.
FredTheDino commentedon Nov 15, 2022
I also hate when this happens (#same). But I'm also scared of the type-checker. I'm very interested in looking at this.
But I'm not sure we have to go digging in the type-checker. When we write out the errors, we know what is in each record (at least parts of it). We could do a super simple diff on those records and simply display that information. It could possibly remove some noise and make it easier to see the actual difference.
Here's a toy example:
E.g.
Here we could make the compiler do parts of the diffing for us in the error message. Then we could present it as something like:
This would solve most of the problems I've experienced. I also truly hate "spot the differences games".
This won't solve all of the problem. But I think it's quite an easy fix that gets us some of the way there. Because the information is there, it's just really poorly presented.
Another alternative is color coding the labels that don't exist in the other output as being red or something. But I kinda like the idea of removing some of the text from the type errors, since they are way to big as they are.
JordanMartinez commentedon Jul 22, 2023
Looking at #4421, a few thoughts come to mind.
First, this issue is complicated because making a change that makes an error message more understandable for one user may make a different error message less understandable for a different user. I think this is the main issue with that PR.
Second, when developers hit compiler errors, I think there are two generally kinds of workflows they take:
Perhaps a better way forward is to allow the user more control when they hit Workflow 2 above?
If a user could use some kind of mechanism in these ad-hoc cases, they could build their code multiple times with different row-error-reporting strategies, each of which further informs them about the issue. This would deal with the first problem raised above about harming user A to benefit user B: such "harm" to user A only applies if they use the mechanism intended for user B.
If such a mechanism was supported, then the approach currently taken in #4421 (i.e. the
fromRowListapproach) could be opted-into by the user. However, Ryan's second suggestion (i.e. thesubstituteTypeapproach) could also be opted-into by the user. I don't know if those are the only two cases, but it would allow benefits to user B without harming user A and thus make progress on the issue.As for the mechanism, we could use a CLI flag, but I think that's undesirable. Keeping the CLI surface small is a net win IMO. The mechanism I'm considering is using an annotation (i.e. #4482). It's something I can quickly add to my code for debugging purposes and then immediately remove thereafter. Moreover, I can imagine a scenario where I annotate multiple pieces of my code and the compiler uses that information to inform its error-reporting. Perhaps I know that some type alias, some type class, and a particular usage of the two is what is involved in the problem. Annotating all of them would cause the compiler to produce "better errors" for that particular ad-hoc case. And since these are meant for debugging purposes, any such usage of annotations would produce a compiler warning to encourage the user to remove such annotations once they have finished debugging the problem.
In this way, these error-reporting annotations are like temporary in-source-code error-reporting CLI flags/arguments that don't increase the CLI surface.
Thoughts?
rhendric commentedon Jul 23, 2023
I think #4421 is pretty close to mergable; it isn't stuck on a design consideration, just on someone with enough on-the-ground experience with this particular frustration to be able to sign off on what is and isn't an improvement who is willing to answer the remaining questions and take the time to get it over the finish line.
That said, annotations to control the manner in which error messages are presented are an interesting idea and, if another part of this design space ends up being contentious and if #4482 actually happens, it would be a good possibility to consider.
JordanMartinez commentedon Oct 28, 2023
Continuing from the discussion from #4421 (comment), where the focus changed to improving row errors when
Row.Unionconstraints are used, Ryan indicated how we could move forward via:Using
react-basic-hooks-dom'sdivcomponent's API as an example, I added the following failing test locally. I updated various type signatures to reduce the number of imports needed. Then, I ran it on bothmasterand #4421. Both versions produce the exact same output.UnionRowErrors.purs
Error message output
In this specific case, I think the ideal solution is to print the type alias everywhere where the long row appears. Due to desugaring our AST before typechecking it, we lose that information.
I think the next best thing we could do is deduplicate the rows. In other words, look through the resulting error message, find all rows, and if they are the same, extract that out to a variable of sorts and reference that variable instead. For example, rather than the above error, we might get:
rhendric commentedon Oct 31, 2023
Deduplication might not be all that helpful in less trivial cases. If the offending code was
div { id: "my-div", classNameBad: "foo" }, then the first type that you've replaced with$$RowTypewould be all ofProps_divwithout theidlabel, and the second one would still be all ofProps_div. Maybe we could make the deduplicator smart enough to present the latter as(id :: String | $$RowType )?Here's another proposal: when a
TypesDoNotUnifybetween an open row (row with variable tail) and a closed row is presented, and the closed row is above a certain size, and verbose messages are off, only show the labels in the closed row that are alphabetically nearby (±2, or something?) to the labels present in the open row. In this case, that would be something like:(and then the types present in the hints remain super ugly, but is that important?)
JordanMartinez commentedon Nov 9, 2023
That's better than what I came up. To clarify, the 'alphabetically nearby' approach is based on the first few characters in the label, correct? And if someone happened to be deleting the prefix part of a label (e.g.
pineapple->eapple), this approach would show a different set of labels (e.g.e-staring labels rather thanapple-starting labels), correct?rhendric commentedon Nov 10, 2023
Yeah, that's what I had in mind. It's not ideal if the error is in the initial letters.
If the user did a typo (
lcassNameorlassName), hopefully seeing the error is enough to nudge them toward correcting it without needing to see the correct label.The somewhat more concerning case is if the user has misremembered an initial component (they think it's
elementClassName, for example); here this approach doesn't help, and maybe we'd want to add a did-you-mean hint when applicable. (If this is the motivating case for that hint, though, I think we wouldn't want to use per-character edit distance to select the did-you-mean; I think it'd make more sense to split on word boundaries and look for common words among the expected labels.)