Mention which row label the type error occurs on #4411

FredTheDino · 2022-10-27T15:59:05Z

Description of the change
Writing code is hard, we make errors. Better errors makes it easier to write code.
This PR makes it clear which row-label type errors occur on.

Example of the previous error message for this code:

a :: { f :: Int }
a = { f: 1 }

b :: { f :: String }
b = a -- Oh noes!

Error found:
...

  Could not match type

    Int

  with type

    String


while checking that type { f :: Int
                         , qa :: Int
                         , qb :: Int
                         , qc :: Int
                         , qd :: Int
                         }
...

Now it reads:

Error found:
...

  Could not match type

    Int

  with type

    String


while matching row label f
while checking that type { f :: Int
                         , qa :: Int
                         , qb :: Int
                         , qc :: Int
                         , qd :: Int
                         }

This is very helpful when you have a lot of labels in your records (150) which I sometimes have and the pretty printer for type errors cuts of the records halfway through or you have to manually differentiate these to find out what's wrong.

Needed guidance

Is the error formatting good?
Haskell formatting and style opinions are welcome

Related to #4005

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)

tests/purs/failing/RecordLabelOnTypeError.out

purefunctor

Just a quick round-up of the changes

src/Language/PureScript/Types.hs

src/Language/PureScript/TypeChecker/Kinds.hs

tests/purs/failing/NestedRecordLabelOnTypeError.out

tests/purs/failing/NestedRecordLabelOnTypeError.purs

MonoidMusician · 2022-11-02T12:20:10Z

I think this is the next place you'll want to hit to add label information for subsumption:

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

Lines 105 to 116 in b7e0596

    
           subsumes' mode (TypeApp _ f1 r1) (TypeApp _ f2 r2) | eqType f1 tyRecord && eqType f2 tyRecord = do 
        
               let (common, ((ts1', r1'), (ts2', r2'))) = alignRowsWith (subsumes' SNoElaborate) r1 r2 
        
               -- For { ts1 | r1 } to subsume { ts2 | r2 } when r1 is empty (= we're working with a closed row), 
        
               -- every property in ts2 must appear in ts1. If not, then the candidate expression is missing a required property. 
        
               -- Conversely, when r2 is empty, every property in ts1 must appear in ts2, or else the expression has 
        
               -- an additional property which is not allowed. 
        
               when (isREmpty r1') 
        
                 (for_ (firstMissingProp ts2' ts1') (throwError . errorMessage . PropertyIsMissing . rowListLabel)) 
        
               when (isREmpty r2') 
        
                 (for_ (firstMissingProp ts1' ts2') (throwError . errorMessage . AdditionalProperty . rowListLabel)) 
        
               -- Check subsumption for common labels 
        
               sequence_ common

Probably need to add a alignRowsWithLabel helper next to alignRowsWith, or something:

purescript/src/Language/PureScript/Types.hs

Lines 438 to 449 in b7e0596

    
           -- | Align two rows of types, splitting them into three parts: 
        
           -- 
        
           -- * Those types which appear in both rows 
        
           -- * Those which appear only on the left 
        
           -- * Those which appear only on the right 
        
           -- 
        
           -- Note: importantly, we preserve the order of the types with a given label. 
        
           alignRowsWith 
        
             :: (Type a -> Type a -> r) 
        
             -> Type a 
        
             -> Type a 
        
             -> ([r], (([RowListItem a], Type a), ([RowListItem a], Type a)))

FredTheDino · 2022-11-03T16:02:53Z

I think this is the next place you'll want to hit to add label information for subsumption:

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

Lines 105 to 116 in b7e0596

subsumes' mode (TypeApp _ f1 r1) (TypeApp _ f2 r2) | eqType f1 tyRecord && eqType f2 tyRecord = do

let (common, ((ts1', r1'), (ts2', r2'))) = alignRowsWith (subsumes' SNoElaborate) r1 r2

-- For { ts1 | r1 } to subsume { ts2 | r2 } when r1 is empty (= we're working with a closed row),

-- every property in ts2 must appear in ts1. If not, then the candidate expression is missing a required property.

-- Conversely, when r2 is empty, every property in ts1 must appear in ts2, or else the expression has

-- an additional property which is not allowed.

when (isREmpty r1')

(for_ (firstMissingProp ts2' ts1') (throwError . errorMessage . PropertyIsMissing . rowListLabel))

when (isREmpty r2')

(for_ (firstMissingProp ts1' ts2') (throwError . errorMessage . AdditionalProperty . rowListLabel))

-- Check subsumption for common labels

sequence_ common

Probably need to add a alignRowsWithLabel helper next to alignRowsWith, or something:

purescript/src/Language/PureScript/Types.hs

Lines 438 to 449 in b7e0596

-- | Align two rows of types, splitting them into three parts:

--

-- * Those types which appear in both rows

-- * Those which appear only on the left

-- * Those which appear only on the right

--

-- Note: importantly, we preserve the order of the types with a given label.

alignRowsWith

:: (Type a -> Type a -> r)

-> Type a

-> Type a

-> ([r], (([RowListItem a], Type a), ([RowListItem a], Type a)))

I think I've already changed that, and I think you're referencing into a branch. Currently it is:

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

Lines 105 to 116 in 2c90056

    
           subsumes' mode (TypeApp _ f1 r1) (TypeApp _ f2 r2) | eqType f1 tyRecord && eqType f2 tyRecord = do 
        
               let goWithLabel l t1 t2 = withErrorMessageHint (ErrorInRowLabel l) $ subsumes' SNoElaborate t1 t2 
        
               let (common, ((ts1', r1'), (ts2', r2'))) = alignRowsWith goWithLabel r1 r2 
        
               -- For { ts1 | r1 } to subsume { ts2 | r2 } when r1 is empty (= we're working with a closed row), 
        
               -- every property in ts2 must appear in ts1. If not, then the candidate expression is missing a required property. 
        
               -- Conversely, when r2 is empty, every property in ts1 must appear in ts2, or else the expression has 
        
               -- an additional property which is not allowed. 
        
               when (isREmpty r1') 
        
                 (for_ (firstMissingProp ts2' ts1') (throwError . errorMessage . PropertyIsMissing . rowListLabel)) 
        
               when (isREmpty r2') 
        
                 (for_ (firstMissingProp ts1' ts2') (throwError . errorMessage . AdditionalProperty . rowListLabel)) 
        
               -- Check subsumption for common labels

purescript/src/Language/PureScript/Types.hs

Lines 438 to 449 in 2c90056

    
           -- | Align two rows of types, splitting them into three parts: 
        
           -- 
        
           -- * Those types which appear in both rows 
        
           -- * Those which appear only on the left 
        
           -- * Those which appear only on the right 
        
           -- 
        
           -- Note: importantly, we preserve the order of the types with a given label. 
        
           alignRowsWith 
        
             :: (Label -> Type a -> Type a -> r) 
        
             -> Type a 
        
             -> Type a 
        
             -> ([r], (([RowListItem a], Type a), ([RowListItem a], Type a)))

But I changed the alignRowsWith function so the first given function also takes a label. I don't know which other places to poke in to get more of these row merge errors. But I can probably add some more error messages in other places. I feel like it's smart to start with the low hanging fruit.

What more is required for this to get merged?

MonoidMusician · 2022-11-03T16:33:53Z

You're right, my bad! Since we're checking that a record literal ({ a: "a" }) has a known type ({ a :: Int }), we stay in checking mode, we are not doing subsumption yet. So the place that happens is actually here:

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

Lines 816 to 827 in 2c90056

    
           -- | 
        
           -- Check the type of a collection of named record fields 
        
           -- 
        
           -- The @lax@ parameter controls whether or not every record member has to be provided. For object updates, this is not the case. 
        
           -- 
        
           checkProperties 
        
             :: (MonadSupply m, MonadState CheckState m, MonadError MultipleErrors m, MonadWriter MultipleErrors m) 
        
             => Expr 
        
             -> [(PSString, Expr)] 
        
             -> SourceType 
        
             -> Bool 
        
             -> m [(PSString, Expr)]

I think if we tag those errors there with your ErrorInRowLabel hint, your NestedRecordLabelOnTypeError test should have that label information now too.

Other than that I think this is looking good. I'd like to give it some testing locally of course.

rhendric · 2022-11-03T16:36:42Z

I'm not sure we want the hint there. The error already points to the precise location in the source where the issue is, which is better than a hint.

rhendric · 2022-11-03T16:41:24Z

src/Language/PureScript/TypeChecker/Kinds.hs

@@ -419,6 +419,7 @@ unifyKindsWithFailure
  -> m ()
 unifyKindsWithFailure onFailure = go
  where
+  goWithLabel l t1 t2 = withErrorMessageHint (ErrorInRowLabel l) $ go t1 t2


Instead of defining these local helpers in three different places, could you add a helper next to alignRowsWith called

alignRowsWithHinted :: (MonadState CheckState m, MonadError MultipleErrors m) => (Type a -> Type a -> m ()) -> Type a -> Type a -> (m (), (([RowListItem a], Type a), ([RowListItem a], Type a)))

(note the absence of Label in this type), which simply delegates to your updated alignRowsWith and adds the hint? (I'd also factor the sequence_ call that co-occurs with all of these call sites into this helper, which is why m () and not [m ()].)

I've finally had enough time to look at this. But this is not really possible to do. We get a dependency cycle:

Module imports form a cycle: module ‘Language.PureScript.Types’ (src/Language/PureScript/Types.hs) imports module ‘Language.PureScript.Errors’ (src/Language/PureScript/Errors.hs) which imports module ‘Language.PureScript.Types’ (src/Language/PureScript/Types.hs)

I'm not sure I want to touch this much code and if this is the better change. It would have been great if it worked though. I'm all for exposing less and not moving complexity to the call sites. Curse your cyclic dependencies!

Oh, that's unfortunate. Fine to leave as-is, then.

MonoidMusician · 2022-11-03T16:45:48Z

Perhaps, but you still need to manually keep track of how you got there since it doesn't tell you where the type is coming from. If the field is hidden behind a lot of type synonyms, I think it would be helpful to just see what label is failing at first glance.

I guess with verbose errors we see a little bit more context, but it's pretty rare that I actually enable that flag:

  Could not match type
          
    String
          
  (defined in module Prim)
  with type
       
    Int
       
  (defined in module Prim)

while trying to match type String
  with type Int
while checking that type String
  is at least as general as type Int
while checking that expression "a"
  has type Int
while checking that expression { a: "a"
                               }       
  has type { a :: Int
           }         
in value declaration record

rhendric · 2022-11-03T17:13:34Z

Not sure I understand you. If we're checking a record literal, the thing that gets underlined in an IDE will be the field in the record literal; the label is right there in the expression, so it doesn't matter if the label in the type is hidden behind a synonym. If we're checking a non-literal term, the compiler will attempt to unify the type of that term with the expected type and the code path that already has hints will be taken.

MonoidMusician · 2022-11-03T17:45:30Z

Well I think the root of my dissatisfaction is that the non-verbose hints jump straight to “checking expression "a"”:

while checking that expression "a"
  has type Int
in value declaration record

without an indication of how the typechecker got there from checking record = …. I think there should be some form of breadcrumbs there even in non-verbose mode. Otherwise it looks like it just picked the type Int out of thin air. I know you can reconstruct it from the source, but still, adding a hint “while matching label a” looks awfully convenient to me …

FredTheDino · 2022-11-04T10:54:58Z

I'm not sure I understand the reasoning from @rhendric.
Is your reasoning that:

We point to the source location of the error
From this information we can deduce the label by looking at the code and the error
It's better to not touch code
If these are your arguments I wonder why you don't think these warnings are useful information?

On one level I feel like this adds a bit of noise to the output of the error messages. But I also think it adds helpful information for understanding the type errors quickly and easily.

Adding a bit of extra information can probably help them find the errors quicker and easier even if they're using an IDE.

TL;DR:
I really agree with @MonoidMusician that informing users of how we got to the error is convenient and helpful. It aids understanding, troubleshooting and makes purescript a friendlier language.

rhendric · 2022-11-04T14:31:40Z

I'm not making any sort of argument about touching code. Certainly the amount of code that would be involved here would be justified by improving the error message. I only disagree that, in the narrow case currently under discussion, a label hint is an improvement.

Your code is going to look like:

blah blah blah {
  label1: blah blah,
  label2: blah blah,
  label3: this is a problem,
  label4: blah blah
}

The error message is going to tell you that this is a problem is not the correct type. It'll point you to line and column, which one can visit manually or see highlighted in an IDE.

What value is added by adding a line to the error message that the problem was detected while checking label3? Of course we're checking label3! You'd hardly expect to see an error about this is a problem while checking label1!

The cost is that the more noise we add to our hints (which, IMO, are already on the noisy side), the less useful they are in aggregate for pointing users at what they need to know.

When we aren't talking about record literals, the hints you're adding in this PR certainly add value, because the error message no longer points to a specific label. I have no issue whatsoever with adding hints in those cases; the cost is justified.

I am all for making our error messages more convenient and more helpful. This PR does that, and I approve. More information is not always more helpful, though; one has to consider whether the information is necessary or redundant.

rhendric

I am unimpressed with the latest two commits. I think the functionality added there is unnecessary, as I've previously argued, but I'm willing to accept being outvoted on that. However, the side effect is that actively misleading hints have been added to other scenarios, as demonstrated in the other outputs that needed to change, and this I'm going to hold my ground on as a bad thing. Please at the very least make sure that these cases aren't changed by what you want to do.

tests/purs/failing/1175.out

tests/purs/failing/2616.out

tests/purs/failing/ExtraRecordField.out

tests/purs/failing/FoldableInstance4.out

tests/purs/failing/InstanceSigsBodyIncorrect.out

tests/purs/failing/InstanceSigsDifferentTypes.out

tests/purs/failing/Superclasses1.out

tests/purs/failing/Superclasses5.out

MonoidMusician · 2022-11-08T16:34:48Z

src/Language/PureScript/TypeChecker/Types.hs

-        ps'' <- go ps' (delete (Label p, ty) ts) r
-        return $ (p, v') : ps''
+  go ((p,v):ps') ts r = 
+    withErrorMessageHint (ErrorInRowLabel (Label p))


This is currently applying recursively for each call of go, resulting in extra labels showing up. It just needs to apply to infer/check below.

I made these changes, it didn't fix all of the comments rhendic mentioned earlier. If you want to look for yourself you can see them here: e136c2a

I think the right decision is to not add the information there. I'd rather see this merged and then move to cleaning up the error messages in general. :D

Thanks for the input! :)

MonoidMusician · 2022-11-09T01:00:16Z

I looked into better errors for the instances. The problem is that by the time they're typechecked, they're just tagged as value declarations not as instance declarations, so we aren't getting good hints for them in the first place. If we want to do anything better wrt to this patch (e.g. hiding these new hints for instance declarations, or changing them to say “while checking member compare”) we would need to fix that first. Personally I'm less concerned that the internal desugaring details are leaking through given that this was already happening (#dict Su and Su$Dict), it's out of scope for this PR to fix all of that.

rhendric · 2022-11-09T01:24:47Z

In that case I would prefer to merge this without the most recent commits. The goal is to make the error messages more helpful, which this PR achieves even without adding hints on the case where hints are least necessary. If the PR adds hints that are sometimes helpful and sometimes misleading, I think that's a worse outcome. (It trains users to ignore the hint, and we have too many hints that, speaking only for myself, I already reflexively ignore.)

This reverts commit 102ba80.

This reverts commit 4b67f95.

FredTheDino · 2022-11-12T15:10:10Z

In that case I would prefer to merge this without the most recent commits. The goal is to make the error messages more helpful, which this PR achieves even without adding hints on the case where hints are least necessary. If the PR adds hints that are sometimes helpful and sometimes misleading, I think that's a worse outcome. (It trains users to ignore the hint, and we have too many hints that, speaking only for myself, I already reflexively ignore.)

I agree. Some of these hints are confusing. Errors overall can use some cleaning. I've reverted the last two commits. It's better we add something that is a clear improvement - if we want to add them we add them later.
Thanks for all the feedback @rhendric and @MonoidMusician!

Is there anything more that anyone wants to see regarding this PR. Otherwise I'd love to see this get merged. :D

rhendric

You have my approval ~~conditional on addressing the minor feedback on the last test~~ ☑.

tests/purs/failing/RecordLabelOnTypeErrorImediate.purs

FredTheDino · 2023-01-09T07:50:46Z

I'd love to see this merged. What more do you want me to do?

rhendric · 2023-01-09T20:57:29Z

Appreciate your engagement! Currently, what this PR needs is a second maintainer to review it.

JordanMartinez · 2023-01-20T19:45:03Z

I haven't forgotten about this PR. I anticipate re-reviewing this sometime in the next two weeks. Sorry for the delay. I've been focusing my free time on porting a JS library to PureScript.

JordanMartinez

LGTM. I'm grateful for all the others who reviewed this as it feels like I'm more rubber-stamping this than anything else.

JordanMartinez · 2023-01-31T03:01:47Z

I don't have enough time to write up a cleaner commit message that summarizes the work down here and reasons for why other work was NOT done here, so I'm not going to merge this yet. Perhaps another core team member can do that if I don't get to it sooner.

MonoidMusician

Hey @FredTheDino, sorry I haven't had a chance to get back to this earlier! Been busy with things especially work on the registry.

I gave it a test run and it's working well so I'm happy to merge it. There is a small issue with the non-verbose hints still being a bit too verbose that I will look at addressing in a later PR (we could trim out some while matching type X with type Y in between row labels, it only shows up in some circumstances). I will start looking at your other PR again soon.

Thanks for your efforts!

FredTheDino added 6 commits October 27, 2022 12:03

Add a row-label error

5f5baeb

Inject the labels in the error messages

813783b

Change the error label a bit

0d56135

Remove stray trace

74b5848

Add a test

ec1225f

Do the tasks in the PR

2e7d90e

purefunctor reviewed Oct 28, 2022

View reviewed changes

tests/purs/failing/RecordLabelOnTypeError.out Outdated Show resolved Hide resolved

Fix the test output

e642e12

purefunctor reviewed Oct 30, 2022

View reviewed changes

src/Language/PureScript/Types.hs Outdated Show resolved Hide resolved

src/Language/PureScript/TypeChecker/Kinds.hs Outdated Show resolved Hide resolved

FredTheDino added 4 commits November 1, 2022 10:27

Refactor alignRowsWith to use a case

9712abc

Extract lambdas into bindings

73c49c3

Update the errors to be clearer

26cb0e1

Merge branch 'master' into mention-row-tags-in-type-errors

461393a

JordanMartinez reviewed Nov 1, 2022

View reviewed changes

tests/purs/failing/NestedRecordLabelOnTypeError.out Outdated Show resolved Hide resolved

MonoidMusician reviewed Nov 2, 2022

View reviewed changes

tests/purs/failing/NestedRecordLabelOnTypeError.purs Outdated Show resolved Hide resolved

FredTheDino added 3 commits November 2, 2022 09:13

Update the erroneous source file

5859372

Add yet another case for the error files

560e5ed

Update the test outputs

2c90056

rhendric reviewed Nov 3, 2022

View reviewed changes

FredTheDino added 2 commits November 8, 2022 14:34

Update comppiler tests

4b67f95

Other outputs that changed

102ba80

rhendric requested changes Nov 8, 2022

View reviewed changes

MonoidMusician reviewed Nov 8, 2022

View reviewed changes

FredTheDino added 2 commits November 12, 2022 16:09

Revert "Other outputs that changed"

f9a0aff

This reverts commit 102ba80.

Revert "Update comppiler tests"

24542d4

This reverts commit 4b67f95.

FredTheDino requested review from rhendric and MonoidMusician and removed request for rhendric and MonoidMusician November 12, 2022 15:10

rhendric approved these changes Nov 12, 2022

View reviewed changes

tests/purs/failing/RecordLabelOnTypeErrorImediate.purs Outdated Show resolved Hide resolved

FredTheDino added 2 commits November 12, 2022 21:12

Fix typo and update comment

56bfdde

Rename the module in the test as well

7cd2c20

FredTheDino mentioned this pull request Dec 19, 2022

Better row unification error messages #4421

Open

5 tasks

FredTheDino requested review from JordanMartinez, MonoidMusician and rhendric and removed request for JordanMartinez, MonoidMusician and rhendric January 9, 2023 07:48

JordanMartinez approved these changes Jan 31, 2023

View reviewed changes

MonoidMusician approved these changes Feb 1, 2023

View reviewed changes

MonoidMusician merged commit 7a5b2b8 into purescript:master Feb 1, 2023

Mention which row label the type error occurs on #4411

Mention which row label the type error occurs on #4411

Uh oh!

Conversation

FredTheDino commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Needed guidance

Uh oh!

Uh oh!

purefunctor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MonoidMusician commented Nov 2, 2022

Uh oh!

FredTheDino commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MonoidMusician commented Nov 3, 2022

Uh oh!

rhendric commented Nov 3, 2022

Uh oh!

rhendric Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

FredTheDino Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

rhendric Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

MonoidMusician commented Nov 3, 2022

Uh oh!

rhendric commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MonoidMusician commented Nov 3, 2022

Uh oh!

FredTheDino commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhendric commented Nov 4, 2022

Uh oh!

rhendric left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MonoidMusician Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

FredTheDino Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

MonoidMusician commented Nov 9, 2022

Uh oh!

rhendric commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FredTheDino commented Nov 12, 2022

Uh oh!

rhendric left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FredTheDino commented Jan 9, 2023

Uh oh!

rhendric commented Jan 9, 2023

Uh oh!

FredTheDino commented Oct 27, 2022 •

edited

Loading

FredTheDino commented Nov 3, 2022 •

edited

Loading

rhendric commented Nov 3, 2022 •

edited

Loading

FredTheDino commented Nov 4, 2022 •

edited

Loading

rhendric left a comment •

edited

Loading

rhendric commented Nov 9, 2022 •

edited

Loading

rhendric left a comment •

edited

Loading