Improve AST-level source position tracking. #2076
Conversation
sauclovian-g
added
topics: error-messages
|
I would appreciate fairly close attention to 10dbb25, because even though it's fairly simple it's the kind of thing where if you botch it up you'll still be finding problems three years later. (Recommend looking at that commit by itself for this.) |
|
The case in question is: Sticking it in the test suite raises some other questions though (mostly about how to prepare for #2075); not opposed to doing it but those questions probably ought to get answered first. See other message elsewhere... |
|
I propose one way to handle adding test cases of this sort in #2075 (comment). |
There was a problem hiding this comment.
Great work! Do make sure to rename the type_errors subdirectory to something like test_type_errors so that SAW's test suite driver picks it up (see here).
| # Prune the timestamps from the log since they'll never match. | ||
| # We also need to shorten pathnames that contain the current | ||
| # directory, because saw feels the need to expand pathnames | ||
| # (see also saw-script #2082). | ||
| sed < $TEST.rawlog > $TEST.log ' | ||
| /^\[[0-9][0-9]:[0-9][0-9]:[0-9][0-9]\.[0-9][0-9][0-9]\] /{ | ||
| s/^..............// | ||
| } | ||
| s,'"$CURDIR"'/,, | ||
| ' |
There was a problem hiding this comment.
Ideally, we'd introduce a settings into SAW that:
- Omits timestamps
- Shorten path names
So that we don't have to strip them out with sed afterwards. Can you file issues about these? (No need to hold up this PR, but I want to make sure the ideas are recorded.)
Add positions to all expression and pattern nodes. Kill off the LExpr
and LPattern expression and pattern cases that held just a position.
The idea had been to always use the first enclosing such position.
This doesn't provide accurate enough positions for type errors (in
general for a type error you always want the exact expression you're
complaining about, especially if you're printing spans of positions
and not just line numbers), and, worse, with this approach it's hard
to tell if any given position is the one you actually ought to be
using.
Fix up some of the position usage where prompted by the ensuing typing
changes.
Kill off the currentExprPos field of the local state monad in MGU.hs,
which was for collecting the enclosing LExpr and is no longer needed.
In pursuit of all this, add two ops to Position.hs:
- leadingPos, which gets the empty (no span) position at the beginning
of the position of something else, which can be used for deriving the
position of implicit elements;
- maxSpan', which is like maxSpan but works for (exactly) two Positioned
objects of potentially heterogeneous type. This allows dropping a
number of explicit calls to getPos in places where that's verbose and
ugly.
These contain source positions, so derived Eq instances aren't in general going to produce the desired behavior. Leave the Eq instance on Type for the moment because it's used in the typechecker. This is going to require further attention.
There were only two uses of it; provide matching-based alternatives for both, on the grounds that if we don't need an explicit Eq instance that specifically ignores position information it's better not to have to write one. The use in MGU handled peeling off any LType nodes; the one in Interpreter did not and was therefore potentially wrong...
We concluded after discussion that using the more precise position from the pattern involved is preferable to using PosREPL, even though the name "it" is predicated on this coming from the repl.
Also add a bunch of infrastructure -- the driver script for this is massively overkill for just this test but we expect this test to grow a lot of siblings in the not-too-distant future.
sauclovian-g
force-pushed
the
2071-dholland-positioning
branch
from
ad9b127 to
2f5bb08
Compare
|
Force-push was to rebase on master -- the only things in master since this branch started were submodule bumps, which are extremely unlikely to interact with the things I've been doing. (merged with master first so the force-push comparison gh offers will come out identical) |
|
(careful examination will also reveal that I squashed the "rename test dir" commit; there's no point preserving that glitch, plus git's shortcomings with rename handlings make it better to avoid having any when the opportunity arises) |
This is the first chunk of work on #2071.
Track positions explicitly in every expression and pattern constructor (but not types, yet); kill off the Eq instances this renders problematic (or were already problematic). Kill off the Eq instance for Type too and clean up the things that were trying to use it.