RE: D3754: Refactor uo_thing

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: D3754: Refactor uo_thing

GHC - devs mailing list

Richard

 

I’m on a train, so can’t look at the detail.  But the following thought occurs to me.

 

uo_thing is a Maybe SDoc attached to a constraint.   But a constraint also has a stack of [ErrCtxt], captured in the CtLoc.  That’s what produces the “In the declaration for …” stuff.

 

Idea: instead of uo_thing, could we just push an extra ErrCtxt onto the stack, in the CtLoc for this constraint?  (Or not, in the Nothing case.)

 

Simon

 

From: [hidden email] [mailto:[hidden email]]
Sent: 18 July 2017 19:39
To: Simon Peyton Jones <[hidden email]>
Subject: [Differential] [Request, 533 lines] D3754: Refactor uo_thing

 

goldfire created this revision.
Herald added subscribers: thomie, rwbarton.
Herald added a reviewer: hvr.

View Revision

 

REVISION SUMMARY

The uo_thing field of TypeEqOrigin is used to track the
"thing" (either term or type) that has the type (kind) stored
in the TypeEqOrigin fields. Previously, this was sometimes a
proper Core Type, which needed zonking and tidying. Now, it
is only HsSyn: much simpler, and the error messages now use
the user-written syntax.

But this aspect of uo_thing didn't cause Trac #13819; it was the
sibling field uo_arity that did. uo_arity stored the number
of arguments of uo_thing, useful when reporting something
like "should have written 2 fewer arguments". We wouldn't want
to say that if the thing didn't have two arguments. However,
in practice, GHC was getting this wrong, and this message
didn't seem all that helpful. Furthermore, the calculation
of the number of arguments is what caused Trac #13819 to fall over.
This patch just removes uo_arity. In my opinion, the change
to error messages is a nudge in the right direction.

Test case: typecheck/should_fail/T13819

 

TEST PLAN

./validate

 

REPOSITORY

rGHC Glasgow Haskell Compiler

 

BRANCH

wip/rae

 

 

AFFECTED FILES

compiler/ghci/RtClosureInspect.hs
compiler/typecheck/Inst.hs
compiler/typecheck/TcArrows.hs
compiler/typecheck/TcErrors.hs
compiler/typecheck/TcExpr.hs
compiler/typecheck/TcHsType.hs
compiler/typecheck/TcMType.hs
compiler/typecheck/TcPat.hs
compiler/typecheck/TcRnTypes.hs
compiler/typecheck/TcSigs.hs
compiler/typecheck/TcSplice.hs
compiler/typecheck/TcSplice.hs-boot
compiler/typecheck/TcTyClsDecls.hs
compiler/typecheck/TcType.hs
compiler/typecheck/TcUnify.hs
compiler/typecheck/TcUnify.hs-boot
compiler/types/Type.hs
testsuite/tests/indexed-types/should_fail/T12867.stderr
testsuite/tests/polykinds/T12593.stderr
testsuite/tests/polykinds/T6039.stderr
testsuite/tests/polykinds/T7278.stderr
testsuite/tests/polykinds/T8616.stderr
testsuite/tests/polykinds/T9200b.stderr
testsuite/tests/rename/should_fail/rnfail026.stderr
testsuite/tests/th/T3177a.stderr
testsuite/tests/typecheck/should_fail/T11356.stderr
testsuite/tests/typecheck/should_fail/T11672.stderr
testsuite/tests/typecheck/should_fail/T12785b.stderr
testsuite/tests/typecheck/should_fail/T13819.hs
testsuite/tests/typecheck/should_fail/T13819.stderr
testsuite/tests/typecheck/should_fail/T2994.stderr
testsuite/tests/typecheck/should_fail/T3540.stderr
testsuite/tests/typecheck/should_fail/T4875.stderr
testsuite/tests/typecheck/should_fail/T7609.stderr
testsuite/tests/typecheck/should_fail/T7778.stderr
testsuite/tests/typecheck/should_fail/all.T
testsuite/tests/typecheck/should_fail/tcfail070.stderr
testsuite/tests/typecheck/should_fail/tcfail078.stderr
testsuite/tests/typecheck/should_fail/tcfail113.stderr
testsuite/tests/typecheck/should_fail/tcfail123.stderr
testsuite/tests/typecheck/should_fail/tcfail132.stderr

 

 

To: goldfire, simonpj, austin, bgamari, hvr
Cc: rwbarton, thomie, Mikolaj, fryguybob, carter, mpickering, adamgundry


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: D3754: Refactor uo_thing

Richard Eisenberg-4
Hi Simon,

I suppose this is possible, but it would change lots of error messages. (Currently, the uo_thing -- when reported -- is included inline with other parts of the message.) In my opinion, if we're going to reformat error messages, we could do better than this, anyway. I'm thinking of something like

Type mismatch error:
  `uo_thing` has one type, but I expected a different type.
  type of `uo_thing`: `actual`
  expected type: `expected`

where `expected` and `actual` are vertically aligned, as they are today. (This would require the ability to query the rendered length of an SDoc, which is not currently possible... but I don't think would be all that hard, as long as we were in a context that returned an SDoc.) What I like about this formatting is that it puts `uo_thing` right at the top of the error; this is the chunk of user-written syntax that causes the error.

I've not tried this yet because I haven't the time to sift through the thousand error-message changes that this would cause.

Putting uo_thing in the ErrCtxt would make something like this impossible, I think.

Richard

On Jul 21, 2017, at 3:50 AM, Simon Peyton Jones <[hidden email]> wrote:

Richard
 
I’m on a train, so can’t look at the detail.  But the following thought occurs to me.
 
uo_thing is a Maybe SDoc attached to a constraint.   But a constraint also has a stack of [ErrCtxt], captured in the CtLoc.  That’s what produces the “In the declaration for …” stuff.
 
Idea: instead of uo_thing, could we just push an extra ErrCtxt onto the stack, in the CtLoc for this constraint?  (Or not, in the Nothing case.)
 
Simon
From: [hidden email] [[hidden email]] 
Sent: 18 July 2017 19:39
To: Simon Peyton Jones <[hidden email]>
Subject: [Differential] [Request, 533 lines] D3754: Refactor uo_thing
 

goldfire created this revision.
Herald added subscribers: thomie, rwbarton.
Herald added a reviewer: hvr. 

 

REVISION SUMMARY

The uo_thing field of TypeEqOrigin is used to track the
"thing" (either term or type) that has the type (kind) stored
in the TypeEqOrigin fields. Previously, this was sometimes a
proper Core Type, which needed zonking and tidying. Now, it
is only HsSyn: much simpler, and the error messages now use
the user-written syntax.

But this aspect of uo_thing didn't cause Trac #13819; it was the
sibling field uo_arity that did. uo_arity stored the number
of arguments of uo_thing, useful when reporting something
like "should have written 2 fewer arguments". We wouldn't want
to say that if the thing didn't have two arguments. However,
in practice, GHC was getting this wrong, and this message
didn't seem all that helpful. Furthermore, the calculation
of the number of arguments is what caused Trac #13819 to fall over.
This patch just removes uo_arity. In my opinion, the change
to error messages is a nudge in the right direction.

Test case: typecheck/should_fail/T13819

 
TEST PLAN

./validate

 
REPOSITORY
rGHC Glasgow Haskell Compiler
 
BRANCH
wip/rae
 
 
AFFECTED FILES
compiler/ghci/RtClosureInspect.hs
compiler/typecheck/Inst.hs
compiler/typecheck/TcArrows.hs
compiler/typecheck/TcErrors.hs
compiler/typecheck/TcExpr.hs
compiler/typecheck/TcHsType.hs
compiler/typecheck/TcMType.hs
compiler/typecheck/TcPat.hs
compiler/typecheck/TcRnTypes.hs
compiler/typecheck/TcSigs.hs
compiler/typecheck/TcSplice.hs
compiler/typecheck/TcSplice.hs-boot
compiler/typecheck/TcTyClsDecls.hs
compiler/typecheck/TcType.hs
compiler/typecheck/TcUnify.hs
compiler/typecheck/TcUnify.hs-boot
compiler/types/Type.hs
testsuite/tests/indexed-types/should_fail/T12867.stderr
testsuite/tests/polykinds/T12593.stderr
testsuite/tests/polykinds/T6039.stderr
testsuite/tests/polykinds/T7278.stderr
testsuite/tests/polykinds/T8616.stderr
testsuite/tests/polykinds/T9200b.stderr
testsuite/tests/rename/should_fail/rnfail026.stderr
testsuite/tests/th/T3177a.stderr
testsuite/tests/typecheck/should_fail/T11356.stderr
testsuite/tests/typecheck/should_fail/T11672.stderr
testsuite/tests/typecheck/should_fail/T12785b.stderr
testsuite/tests/typecheck/should_fail/T13819.hs
testsuite/tests/typecheck/should_fail/T13819.stderr
testsuite/tests/typecheck/should_fail/T2994.stderr
testsuite/tests/typecheck/should_fail/T3540.stderr
testsuite/tests/typecheck/should_fail/T4875.stderr
testsuite/tests/typecheck/should_fail/T7609.stderr
testsuite/tests/typecheck/should_fail/T7778.stderr
testsuite/tests/typecheck/should_fail/all.T
testsuite/tests/typecheck/should_fail/tcfail070.stderr
testsuite/tests/typecheck/should_fail/tcfail078.stderr
testsuite/tests/typecheck/should_fail/tcfail113.stderr
testsuite/tests/typecheck/should_fail/tcfail123.stderr
testsuite/tests/typecheck/should_fail/tcfail132.stderr
 
 
To: goldfire, simonpj, austin, bgamari, hvr
Cc: rwbarton, thomie, Mikolaj, fryguybob, carter, mpickering, adamgundry


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Loading...