Custom Error Handling Prototype - #145
Conversation
|
@ianjosephwilson Just saw you flipped this out of draft. Will take some time to work through it, but... wow! |
|
@davepeck Yeah I think we want to start wading through it before I add even more LOC... I ended up having to follow the concept all the way through the processor to determine how things played out to then backtrack that to the parser to get a working design.. working. It is a lot of parts but I find it easier to see all the parts fitting together (or not fitting). |
|
@davepeck I'm not sure if it's possible but do you want me to try to break this up? |
I've been trying to figure out how to break it apart, but I don't really have great suggestions at the moment. In general, smaller incremental PRs to tackle our bigger projects would be helpful, but I'm not really sure how feasible that is with this particular PR! :-) Maybe there's a meaningful divide between the stuff that's tied to the parser and the stuff that's tied to the processor, not sure... I'm so sorry it's been taking me this long to get through this. |
There was a problem hiding this comment.
Okay!
I've been sitting on a whole bunch of comments for a while, but they were very scattered. I realized I needed to do them in batches if I had any hope of providing useful and timely (okay, too late for that!) feedback.
I'll start with small/point-wise comments and try to cover one corner of the code at the time.
Then I'll work my way up to the bigger picture.
Thanks for hanging in there! :-)
|
|
||
|
|
||
| @dataclass(slots=True, frozen=True) | ||
| class PartPosition: |
There was a problem hiding this comment.
What would it look like if PartPosition included accessor methods and the underlying math necessary to get at specific items in a TemplateRef?
I see us using // 2, % 2 in a few other bits of code that are all effectively trying to interpret a PartPosition — and I worry the "right" math may be easy to miss if we keep repeating it.
Dunno; maybe the math is isolated enough (if slice() is the one true user of PartPosition) or maybe this would just prove to be annoying in practice...
There was a problem hiding this comment.
For now we should probably just try to keep it contained in places that use it internally but stop it from leaking out. For example, if part_pos.index % 2 != 0 and part_pos.offset != 0: in parser_utils.py:ParserPositionTranslator should probably be some sort of function call into template_utils.py. It is in the SourceTracker iterator but that might be able to be refactored eventually.
| ) | ||
| else: | ||
| # @NOTE: No offset OR limit applied to interpolations. | ||
| return TemplateRef(strings=("", ""), i_indexes=((first - 1) // 2,)) |
There was a problem hiding this comment.
This case feels surprising to me. I guess my default assumption for intervals is that they're half open aka start is inclusive and stop is exclusive. When making sure I understood slice(), I tried this example and was surprised by the result:
>>> tr = TemplateRef(strings=("ABC", "DEF", "GHI"), i_indexes=(0, 1))
>>> tr.slice(start=PartPosition(1, 0), stop=PartPosition(1, 0))
TemplateRef(strings=('', ''), i_indexes=(0,)) # actual result
TemplateRef(strings=('', ), i_indexes=()) # what I expected
Going a little further, if I really wanted just the interpolation, I'd do:
>>> tr.slice(start=PartPosition(1, 0), stop=PartPosition(2, 0))
TemplateRef(strings=('', ''), i_indexes=(0,)) # actual result *and* what I expected
There was a problem hiding this comment.
I think the double indexing if you want to call it that makes this more confusing. We cut up to but excluding limit. So if you have a stop then it is inclusive but only up to its limit and excludes after that... For an interpolation setting stop's offset does not make sense so its more like offset=None which is interpreted as "the end of thing". Similarly start's offset for an interpolation would be considered None or "the very start of the thing". So in that case it kind of makes sense that we are doing something like interpolations[start][None:None] which is the interpolation, this parallels the strings version (if it was coded correctly). Although maybe the offset should always be 0 for interpolations?
There is some sort of mental model that seems to be missing here. I might be trying to force a goat to masquerade as a horse by using "slice" instead of fillet, ie. where the parts are kind of awkwardly cut out depending on the structure of things to keep things sane and furthermore they must be re-assembled in a template compatible manner.
I'm more concerned that the overall strategy might not be viable for some reason. It seems to be "working" but how do you feel about the general strategy?
Parser LinePosition --> Template neutral PartPosition --> Template specific LinePosition
There was a problem hiding this comment.
Actually, sorry, what I said was wrong, the default offset is always 0 for interpolations. So what you said makes sense and maybe clears up some confusion. Matching up these different "coordinate" systems is still a little convaluted though. Maybe I need to make a matrix of the different configurations to check the edge cases on this. My point about addressing the overall strategy still stands though.
There was a problem hiding this comment.
I tried to correct this and added assertions. Maybe the offset = 0 is a rules that should apply to all PartLocation's with odd indices (ie. interpolations)?
There was a problem hiding this comment.
I'm more concerned that the overall strategy might not be viable for some reason. It seems to be "working" but how do you feel about the general strategy?
Sorry. I decided to start with very narrowly scoped feedback on individual functions/lines of code, but the big picture really is the important thing to tackle.
I've pretty much convinced myself that your general approach is a good one. In my notes I called it the "three coordinate system" approach: parser coordinates: LinePosition measured against placeholder text, part coordinates: PartPosition which, importantly, can safely sit behind a cache, and source coordinates: LinePosition against the original Python source text. I like that the final translation from part -> source coordinates is lazy, only if there's an error
There are two general things that I noted while reading this PR that seem worth calling out explicitly:
- Nothing that's cached should reference a live
Templateor its values -- akaTTree,TagSourceInfo, etc. This remains true with your PR. - We never look at
Interpolationvalues unless we've already decided there's an error; they are only used to make the messages better
I like both of those as constraints on tdom's code. There's sort of a third I'd like to be true, but I'm not sure it is just yet: error reporting should never hide an original error. I'm still sort of mulling over a bunch of cases here. And looking at a few more possible bugs.
There was a problem hiding this comment.
I think 1 and 2 are good so far. For number 3 I used chaining to build on original errors and I think this is a good way to not lose that information. It can make reading the exception trace almost impossible though so I think eventually there might be a way to configure how things are displayed but as long as we store the right data I think that can all be adjusted.
| # @NOTE: No limit applied to interpolations. | ||
| if index != last: | ||
| i_indexes.append((index - 1) // 2) | ||
| return TemplateRef(strings=tuple(strings), i_indexes=tuple(i_indexes)) |
There was a problem hiding this comment.
I landed on this variant when I was trying to wrap my head around slice():
def slice(
self,
start: PartPosition | None = None,
stop: PartPosition | None = None,
) -> TemplateRef:
"""
Slice template ref based on the given start and stop. `start` is
inclusive; `stop` is exclusive.
"""
size = 2 * len(self.strings) - 1
first = start.index if start else 0
last = stop.index if stop else size - 1
assert 0 <= first < size
assert 0 <= last < size
# If `first` is odd, we need an extra empty string
strings: list[str] = [""] if first % 2 else []
i_indexes: list[int] = []
for index in range(first, last + 1):
if index % 2:
# `offset` is meaningless for interpolations;
# stop sits "before" an interpolation.
if index == last:
break
i_indexes.append((index - 1) // 2)
else:
lo = start.offset if start and index == first else None
hi = stop.offset if stop and index == last else None
strings.append(self.strings[index // 2][lo:hi])
return TemplateRef(strings=tuple(strings), i_indexes=tuple(i_indexes))I think it has the same output on every case, except for the start=PartPosition(1, 0), stop=PartPosition(1, 0) cases.
There was a problem hiding this comment.
This looks more concise but maybe we can keep the sort of "brainless" walking through the different cases version I have now until we know its working correctly or what we define as "correctly". Maybe I should even unroll the offset/limit like you have into the cases to make it even more "written out"?
There was a problem hiding this comment.
I think what's missing here is really a spec for exactly what we expect, and a set of test cases to match.
It's basically "half-open intervals aka start inclusive, stop exclusive; interpolation offsets are always 0".
Here's what I landed on for t"ABC{0}DEF{1}GHI":
| start | stop | shorthand representation of output | changed? |
|---|---|---|---|
| None | None | ['ABC', 0, 'DEF', 1, 'GHI'] | |
| (0,0) | (0,0) | [''] | |
| (0,1) | (0,2) | ['B'] | |
| (0,0) | (0,3) | ['ABC'] | |
| (0,0) | (0,9) | ['ABC'] | |
| (0,1) | (1,0) | ['BC'] | |
| (0,1) | (2,0) | ['BC', 0, ''] | |
| (0,1) | (2,2) | ['BC', 0, 'DE'] | |
| (0,1) | (3,0) | ['BC', 0, 'DEF'] | |
| (0,1) | (4,0) | ['BC', 0, 'DEF', 1, ''] | |
| (0,1) | (4,2) | ['BC', 0, 'DEF', 1, 'GH'] | |
| (1,0) | (1,0) | [''] | (was ['', 0, '']) |
| (1,0) | (2,0) | ['', 0, ''] | |
| (1,0) | (2,2) | ['', 0, 'DE'] | |
| (1,0) | (3,0) | ['', 0, 'DEF'] | |
| (1,0) | (4,0) | ['', 0, 'DEF', 1, ''] | |
| (2,0) | (2,3) | ['DEF'] | |
| (2,3) | (3,0) | [''] | |
| (2,3) | (4,0) | ['', 1, ''] | |
| (3,0) | (3,0) | [''] | (was ['', 1, '']) |
| (3,0) | (4,0) | ['', 1, ''] | |
| (0,1) | None | ['BC', 0, 'DEF', 1, 'GHI'] | |
| (1,0) | None | ['', 0, 'DEF', 1, 'GHI'] | |
| (2,1) | None | ['EF', 1, 'GHI'] | |
| (3,0) | None | ['', 1, 'GHI'] | |
| (4,1) | None | ['HI'] | |
| None | (2,1) | ['ABC', 0, 'D'] |
Hopefully the shorthand is easy to understand, but, for example, ['EF', 1, 'GHI'] means TemplateRef(strings=('EF', 'GHI'), i_indexes=(1,))
| def combine_template_refs(*template_refs: TemplateRef) -> TemplateRef: | ||
| """Concatenate multiple template refs together into a single ref.""" | ||
| # trefs -> naive templates -> naive template -> tref | ||
| return TemplateRef.from_naive_template( |
There was a problem hiding this comment.
(for the future: the from_naive/to_naive dance feels kinda silly here; i realize it's been around for a while, but seems like avoidable frobnication that I think only exists to deal with joining end strings?)
There was a problem hiding this comment.
I tried to remove it but I think we should rename "naive" templates to something else because when you read the usage its easy to miss that template values are being used as indexes. I think when I wrote it that was obvious but coming back later it is not obvious. It is really handy during testing though because you can initialize a TemplateRef right up from a t-string. Maybe something like load_tref_indexes_from_template() or something complicated and dangerous sounding.
| expr_str = ip.expression | ||
| conversion_str = f"!{ip.conversion}" if ip.conversion is not None else "" | ||
| format_spec_str = f":{ip.format_spec}" if ip.format_spec else "" | ||
| return f"{{{expr_str}{conversion_str}{format_spec_str}}}" |
There was a problem hiding this comment.
This is great and doesn't need change.
But because this came up in another context (behavior in annotationlib after 3.14 shipped): there are several things PEP 750 left under- or un-specified. The cpython implementation made its choices. They lead to some super obscure edge cases here:
# cpython handling of the debug specifier (`=`) makes it impossible to reconstruct
>>> interpolation_repr(t"{42=}".interpolations[0])
'{42!r}'
# This one is courtesy of the current cpython parser; might be worth pushing a change
# to cpython; I doubt very many people care about or depend on the current behavior
>>> interpolation_repr(t"{ 42 }".interpolations[0])
'{ 42}'
# I extra shrug my shoulders at this edge case, but:
>>> interpolation_repr(t"{42:{99}}".interpolations[0])
'{42:99}'
Okay, I've lived through my PEP 750 implementation trauma. Carry on. 😅
There was a problem hiding this comment.
I guess we'll have to be careful to try to print the source out with wiggle room so the source can print wrong but the error reporting itself doesn't crash as well. That {99} example is unexpected. I guess it was to make it compatible with f-strings. We can only hope it gets figured out in python 4.
… instead of self-closing.
4e6bc06 to
657e7d1
Compare
| if index != last_index and first_nl_index == -1: | ||
| return PartPosition(index + 1, 0) | ||
| else: | ||
| return PartPosition(last_index, offset_found) |
There was a problem hiding this comment.
Checking my understanding: shouldn't this be return PartPosition(index, offset_found)?
There was a problem hiding this comment.
Yes I think so. Looks like I introduced it by adding the check for first_nl_index == -1 after-the-fact. I'll try to see if I can make a test case for it to see what's happening.
…e away ParserPosition.
|
I started paring this out, starting with #149 , I'm just going to move this back to draft. |
2 participants
SourceTrackerHTMLParser.getpos()could be reconciled with the current string/interpolation index of theTemplatewhile feeding parts but there does not seem to be an easy way to do that.ParserPositionTranslator,LineLocationandPartLocationparser_pos: LineLocationand a current positionpos: MutableLineLocation, and walkposthrough theTemplateparts with the embedded placeholders looping viaindexandoffsetuntilparser_pos == posandsource_pos = PartLocation(index, offset).SourceInfo(andOpenSourceInfo),TTreeandsource_posforTNodesubclassessource_poson each tnode after we translate it from theparser_pos.source_poswe can also store if a tag was self closing (orstartend), the fullstarttag_textasstarttag_refand theendtag_pos(ifstartend==False). This info is inSourceInfo(for accessing via completed nodes) andOpenSourceInfofor incomplete nodesSourceInfowe store it separately from the nodes and pack it into theTTreeso we can unpack when an exception occurs. The reasoning for this was to try to keep most of the error/source details separate from the core processing to keep the core flows clear from debugging/error info churn and also in case that information is not available.SourceReaderSourceTrackerwith the appropriate template to extract the source with the interpolation expressions (when already raising errors) as well as translatePartLocationtoLineLocation(but dependent on the string representations of the interpolations).ParsingError(s)ProcessingError(s)TNodetemplateandTTree(contains the templateTNode root) to the exceptionchainthem withraise ProcessingError() from eTemplateProcessor.process()we should have a singleProcessingErrorthat has a stack of error states that go down until we reach the lowest levelTemplatewhere an error first occurred so we could print something LIKE this (TBD):ProcessingErrororParsingError.